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

["Yao, Jiewen" <jiewen.yao@intel.com>]

thanks laszlo 
I like your idea to eliminate the code duplication and fix all the problem by adding a new api in uefi lib. 

If there is urgency to fix this specific issue, I have no problem on the enhancement. 

Reviewed by : Jiewen.yao@intel.com

thank you!
Yao, Jiewen


> 在 2018年7月12日，上午5:06，Laszlo Ersek <lersek@redhat.com> 写道：
> 
>> On 07/11/18 18:06, Laszlo Ersek wrote:
>>> On 07/11/18 17:44, Roman Bacik wrote:
>>> Hi Laszlo,
>>> 
>>> Thank you very much for your review and help. I would prefer the option 2b.
>> 
>> Great, thanks! Let's wait for the SecurityPkg maintainers then, to give
>> their R-b's for your patch. Chao Zhang, Jiewen, can you please comment?
>> 
>> From my side, dependent on the pending commit message and patch
>> whitespace corrections (which I'm willing to implement myself, at push):
>> 
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Here's the TianoCore BZ that I plan to reference in the updated commit
> message, as "further known problems":
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> 
> Thanks
> Laszlo
> 
> 
>>> Thanks,
>>> 
>>> Roman
>>> 
>>>> On Wed, Jul 11, 2018 at 5:05 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> 
>>>> Hi Roman,
>>>> 
>>>>> On 07/11/18 00:51, rbacik@gmail.com wrote:
>>>>> From: Roman Bacik <roman.bacik@broadcom.com>
>>>>> 
>>>>> When secure boot is enabled, if one loads keys from a FAT formatted
>>>>> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
>>>>> an assert in StrLen() occurs.
>>>>> This is because the filename starts on odd address, which is not a uint16
>>>>> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
>>>>> 
>>>>> Cc: Chao Zhang <chao.b.zhang@intel.com>
>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
>>>>> ---
>>>>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>>>> | 13 +++++++++++--
>>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>> 
>>>> Thank you for sending a well-formed patch.
>>>> 
>>>> I notice that you sent this email from <rbacik@gmail.com>, which is not
>>>> the same as the Signed-off-by line. I realize you posted from
>>>> <rbacik@gmail.com> for technical reasons, and it should be no problem.
>>>> 
>>>> However, I *think* in such cases we usually request the following:
>>>> 
>>>> - Using your broadcom.com email address, please respond to this patch
>>>> (not my present email, but your original git posting), keeping full
>>>> context, and just repeat your Signed-off-by line (referencing the
>>>> broadcom address).
>>>> 
>>>> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
>>>> we've done in the past, in cases when submitters had to post their work
>>>> from private addresses due to company email issues.
>>>> 
>>>> Technical comments below:
>>>> 
>>>>> diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>>>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>>>> SecureBootConfigFileExplorer.c
>>>>> index 1b6f88804275..19b13a5569a6 100644
>>>>> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>>>> SecureBootConfigFileExplorer.c
>>>>> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>>>> SecureBootConfigFileExplorer.c
>>>>> @@ -123,6 +123,8 @@ OpenFileByDevicePath(
>>>>>   EFI_FILE_PROTOCOL               *Handle1;
>>>>>   EFI_FILE_PROTOCOL               *Handle2;
>>>>>   EFI_HANDLE                      DeviceHandle;
>>>>> +  CHAR16                          *PathName;
>>>>> +  UINTN                           PathLength;
>>>>> 
>>>>>   if ((FilePath == NULL || FileHandle == NULL)) {
>>>>>     return EFI_INVALID_PARAMETER;
>>>>> @@ -173,6 +175,11 @@ OpenFileByDevicePath(
>>>>>     //
>>>>>     Handle2  = Handle1;
>>>>>     Handle1 = NULL;
>>>>> +    PathLength = DevicePathNodeLength(*FilePath) -
>>>> sizeof(EFI_DEVICE_PATH_PROTOCOL);
>>>>> +    PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*
>>>> FilePath)->PathName);
>>>> 
>>>> (1) On both lines above, space characters are missing after:
>>>> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
>>>> I think we can fix this up for you when we push the patch. (I'm willing
>>>> to help with that, but we need SecurityPkg maintainer review first.)
>>>> 
>>>> 
>>>>> +    if (PathName == NULL) {
>>>>> +      return EFI_OUT_OF_RESOURCES;
>>>>> +    }
>>>> 
>>>> (2) I have now reviewed the original state of the function more
>>>> carefully, and, while the above "return" branch introduces a leak
>>>> *path*, it does not introduce a leak that doesn't already exist!
>>>> 
>>>> In fact, the original function has multiple issues:
>>>> 
>>>> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
>>>> useless; the intent is obviously to set (*FileHandle) to NULL.
>>>> 
>>>> - At the top of the "while" loop body, "Handle1" stands for an open
>>>> EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
>>>> body returns EFI_INVALID_PARAMETER, then it (a) performs the same
>>>> useless assignment to "FileHandle" as described above, and (b) fails to
>>>> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
>>>> branch introduces no new leak, just a new path to the existent leak.
>>>> 
>>>> - 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!
>>>> 
>>>> 
>>>> Roman, I realize this could be more than what you signed up for; so
>>>> please pick one:
>>>> 
>>>> (2a) you could submit a patch series:
>>>> 
>>>> * Write a patch that sets (*FilePath) to NULL right after the
>>>> (FileHandle==NULL) check, in preparation for failure, and removes all
>>>> the bogus FileHandle=NULL assignments.
>>>> 
>>>> * Write another patch that plugs the leak when the device path type
>>>> check fails -- introduce a "CloseHandle1" label at the end of the
>>>> function, and jump to it when the devpath type check fails, so that we
>>>> close "Handle1". This patch should also invert the meanings of Handle2
>>>> and Handle1 -- the reassignment to Handle1 should only occur *after* we
>>>> successfully open Handle2. "Handle1" should *always* remain suitable for
>>>> closing through the "CloseHandle1" error path.
>>>> 
>>>> * Include your current patch, for fixing the alignment issue.
>>>> 
>>>> * Write another patch that moves the OpenFileByDevicePath() function to
>>>> UefiLib in MdePkg -- under the name EfiOpenFileByDevicePath() -- from
>>>> SecureBootConfigDxe.
>>>> 
>>>> * write two more patches, namely for TlsAuthConfigDxe and RamDiskDxe, in
>>>> order to consume EfiOpenFileByDevicePath() from UefiLib. Both of those
>>>> modules already depend on UefiLib.
>>>> 
>>>> (2b) Alternatively:
>>>> 
>>>> * we can report a new TianoCore BZ about the issues I list above,
>>>> 
>>>> * we can commit this patch of yours as-is, just additionally reference
>>>> the *new* BZ in the commit message, as "further known issues",
>>>> 
>>>> * I can work on the rest of the issues.
>>>> 
>>>> 
>>>> If you pick (2b), then I can
>>>> - file the new BZ,
>>>> - update the commit message for you,
>>>> - update the patch for you, as described in (1),
>>>> - ACK this patch (as updated above),
>>>> - push the patch (if SecurityPkg maintainers agree),
>>>> - take on the new BZ as well.
>>>> 
>>>> Thanks!
>>>> Laszlo
>>>> 
>>>>> 
>>>>>     //
>>>>>     // Try to test opening an existing file
>>>>> @@ -180,7 +187,7 @@ OpenFileByDevicePath(
>>>>>     Status = Handle2->Open (
>>>>>                           Handle2,
>>>>>                           &Handle1,
>>>>> -                          ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
>>>>> +                          PathName,
>>>>>                           OpenMode &~EFI_FILE_MODE_CREATE,
>>>>>                           0
>>>>>                          );
>>>>> @@ -192,7 +199,7 @@ OpenFileByDevicePath(
>>>>>       Status = Handle2->Open (
>>>>>                             Handle2,
>>>>>                             &Handle1,
>>>>> -                            ((FILEPATH_DEVICE_PATH*)*
>>>> FilePath)->PathName,
>>>>> +                            PathName,
>>>>>                             OpenMode,
>>>>>                             Attributes
>>>>>                            );
>>>>> @@ -202,6 +209,8 @@ OpenFileByDevicePath(
>>>>>     //
>>>>>     Handle2->Close (Handle2);
>>>>> 
>>>>> +    FreePool (PathName);
>>>>> +
>>>>>     if (EFI_ERROR(Status)) {
>>>>>       return (Status);
>>>>>     }
>>>>> 
>>>> 
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>> 
>>> 
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>> 
>