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

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

Laszlo already filed one - https://bugzilla.tianocore.org/show_bug.cgi?id=1008

I suggest we add to UefiLib instead of fixing all individual driver.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Zhang, Chao B
> Sent: Monday, July 16, 2018 11:10 PM
> To: rbacik@gmail.com; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Subject: RE: [PATCH v2] SecurityPkg: Fix assert when setting key from
> eMMC/SD/USB
> 
> Hi Bacik:
>    Tks for the fix. Would you please file another report in Bugzilla for RamDisk
> & Tls Configuration driver? They have same issue as SecureBootConfig driver
> 
> -----Original Message-----
> From: rbacik@gmail.com [mailto:rbacik@gmail.com]
> Sent: Wednesday, July 11, 2018 6:51 AM
> To: edk2-devel@lists.01.org
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Vladimir
> Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Subject: [PATCH v2] SecurityPkg: Fix assert when setting key from
> eMMC/SD/USB
> 
> 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/SecureBootConfigFil
> eExplorer.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> FileExplorer.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> FileExplorer.c
> index 1b6f88804275..19b13a5569a6 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> FileExplorer.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> +++ nfigFileExplorer.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);
> +    if (PathName == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> 
>      //
>      // 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);
>      }
> --
> 2.17.1