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

[Laszlo Ersek <lersek@redhat.com>]

I'm not officially a reviewer for SecurityPkg, so just some light comments:

(1) Can you send out the patch with git-send-email? Currently it looks
like the patch was pasted into a desktop or web mail client, and that
makes it hard to apply the patch (it's wrapped etc).

On 07/10/18 00:11, Roman Bacik wrote:
> 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>
> ---
>  .../SecureBootConfigFileExplorer.c                   | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> index 1b6f88804275..d5338406957c 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;
> +  UINT16                          PathLength;
> 
>    if ((FilePath == NULL || FileHandle == NULL)) {
>      return EFI_INVALID_PARAMETER;
> @@ -173,6 +175,10 @@ OpenFileByDevicePath(
>      //
>      Handle2  = Handle1;
>      Handle1 = NULL;
> +    PathLength = ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[0] |
> +                 ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[1] << 8;

(2) Can we use DevicePathNodeLength() from
"MdePkg/Include/Library/DevicePathLib.h" here? (For that, we should also
switch PathLength to UINTN.)

This module already depends on the DevicePathLib class.

Apologies that I didn't suggest this in the BZ.

> +    PathName = AllocateZeroPool (PathLength);
> +    CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> PathLength);

(3) I think it's not necessary to zero-fill the buffer, we're going to
overwrite it right after the allocation.

There's a convenience function for that: AllocateCopyPool().

(4) The number of bytes is not correct IMO. "PathLength" stands for the
number of bytes in the entire device path node (FILEPATH_DEVICE_PATH),
including Header and PathName. So, for getting the number of bytes in
just PathName, we should subtract the size of Header.

Presently, we over-read the source buffer; it's not causing problems
because PathName is NUL-terminated.

(5) Can you please check whether the allocation succeeds?

If it fails (PathName == NULL), we should return EFI_OUT_OF_RESOURCES.
It seems OK to return this error from the loop body.

> 
>      //
>      // Try to test opening an existing file
> @@ -180,7 +186,7 @@ OpenFileByDevicePath(
>      Status = Handle2->Open (
>                            Handle2,
>                            &Handle1,
> -                          ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> +                          PathName,
>                            OpenMode &~EFI_FILE_MODE_CREATE,
>                            0
>                           );
> @@ -192,7 +198,7 @@ OpenFileByDevicePath(
>        Status = Handle2->Open (
>                              Handle2,
>                              &Handle1,
> -                            ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> +                            PathName,
>                              OpenMode,
>                              Attributes
>                             );
> @@ -202,6 +208,8 @@ OpenFileByDevicePath(
>      //
>      Handle2->Close (Handle2);
> 
> +    FreePool (PathName);
> +
>      if (EFI_ERROR(Status)) {
>        return (Status);
>      }
> 

Right, this logic appears fine to me; no leaks.

Thanks!
Laszlo