public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Roman Bacik <roman.bacik@broadcom.com>, edk2-devel@lists.01.org
Cc: Chao Zhang <chao.b.zhang@intel.com>,
	Jiewen Yao <jiewen.yao@intel.com>,
	Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Subject: Re: [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB
Date: Tue, 10 Jul 2018 02:02:43 +0200	[thread overview]
Message-ID: <4e3b21af-4aeb-1524-df25-a1d7e08fdc94@redhat.com> (raw)
In-Reply-To: <CAGQAs7w8MweELynk1XDHTKWAOs1_2GFLwbtj=4-rAqMaUEH-nQ@mail.gmail.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


  reply	other threads:[~2018-07-10  0:02 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-09 22:11 [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB Roman Bacik
2018-07-10  0:02 ` Laszlo Ersek [this message]
2018-07-10  0:24   ` Laszlo Ersek
2018-07-10  0:30     ` Roman Bacik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4e3b21af-4aeb-1524-df25-a1d7e08fdc94@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox