Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB

[Laszlo Ersek <lersek@redhat.com>]

On 07/11/18 19:10, Carsey, Jaben wrote:
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, July 11, 2018 5:16 AM
>> To: rbacik@gmail.com; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Vladimir Olovyannikov
>> <vladimir.olovyannikov@broadcom.com>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Carsey,
>> Jaben <jaben.carsey@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
>> Kinney, Michael D <michael.d.kinney@intel.com>; Zhang, Chao B
>> <chao.b.zhang@intel.com>
>> Subject: Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from
>> eMMC/SD/USB
>> Importance: High
>>
>> On 07/11/18 14:05, Laszlo Ersek wrote:
>>
>>> - The OpenFileByDevicePath() function is duplicated in the following
>>> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
>>> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c".
>> With the
>>> implication that the alignment issue you found affects all three drivers!
>>
>> Note that I've mutually diffed the three function definitions between
>> each other (with "diff -b"), and there are only whitespace and comment
>> wrapping differences.
>>
>> So, again, this function looks like a prime suspect for UefiLib.
>>
>> I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE
>> format"); I think that's simply stale, and should be removed.
> 
> Looks like the stale reference may be from origination as derived from ShellOpenFileByDevicePath.  I agree this should be moved to a lib.  Any way we could merge the shell's only slightly different version also into the mix?

Wow, that's incredible -- ShellOpenFileByDevicePath() contains the
*exact same* alignment fix that Roman is contributing with this patch;
from commit 0b6cb335fa82 ("Fixed some alignment faults in IPF platform",
2013-01-25).

ShellOpenFileByDevicePath() -- including the alignment fix -- also has
the same kind of resource leak (regarding "Handle1").

So, code duplication has bitten us again. We have four instances of
basically the same logic in the tree, all four are affected by the same
resource leak, and the alignment fix that was discovered in or before
2013 was applied to only one of the four.

So, yes. The top of the ShellOpenFileByDevicePath() function (the "UEFI
Shell 2.0 method") should be preserved, but then the last part ("use old
shell method") should be replaced with a call to the new UefiLib API.

And, the final "weak spot" comment remains valid, but only for
ShellOpenFileByDevicePath(), not for the new EfiOpenFileByDevicePath() API.

Thank you Jaben for finding this!
Laszlo