public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Laszlo Ersek <lersek@redhat.com>
Cc: rbacik@gmail.com,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	 Jiewen Yao <jiewen.yao@intel.com>,
	 Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>,
	Chao Zhang <chao.b.zhang@intel.com>,
	 "Jordan Justen (Intel address)" <jordan.l.justen@intel.com>,
	Michael Kinney <michael.d.kinney@intel.com>,
	 "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Date: Wed, 11 Jul 2018 16:16:30 +0200	[thread overview]
Message-ID: <CAKv+Gu-pOHf6zjKeKLkD1nVV5P5DjdvR0r191HyaUXtz0fCa5Q@mail.gmail.com> (raw)
In-Reply-To: <5bca3f43-7c23-dca6-03cd-2d647d8fe253@redhat.com>

On 11 July 2018 at 14:05, 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.
>

Yes, please.

> 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);
>>      }
>>
>


  parent reply	other threads:[~2018-07-11 14:16 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-10 22:51 [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB rbacik
2018-07-11 12:05 ` Laszlo Ersek
2018-07-11 12:15   ` Laszlo Ersek
2018-07-11 17:10     ` Carsey, Jaben
2018-07-11 17:24       ` Laszlo Ersek
2018-07-11 14:16   ` Ard Biesheuvel [this message]
2018-07-11 15:44   ` Roman Bacik
2018-07-11 16:06     ` Laszlo Ersek
2018-07-11 21:06       ` Laszlo Ersek
2018-07-12 12:07         ` Yao, Jiewen
2018-07-12 21:42           ` Laszlo Ersek
2018-07-11 15:43 ` Roman Bacik
2018-07-16 15:09 ` Zhang, Chao B
2018-07-16 15:50   ` Yao, Jiewen
2018-07-17  4: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=CAKv+Gu-pOHf6zjKeKLkD1nVV5P5DjdvR0r191HyaUXtz0fCa5Q@mail.gmail.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