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

[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
>