From: Laszlo Ersek <lersek@redhat.com>
To: Roman Bacik <roman.bacik@broadcom.com>,
Jiewen Yao <jiewen.yao@intel.com>,
Chao Zhang <chao.b.zhang@intel.com>
Cc: Roman Bacik <rbacik@gmail.com>,
edk2-devel@lists.01.org,
Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>,
"Jordan Justen (Intel address)" <jordan.l.justen@intel.com>,
"Gao, Liming" <liming.gao@intel.com>,
Michael Kinney <michael.d.kinney@intel.com>
Subject: Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Date: Wed, 11 Jul 2018 18:06:12 +0200 [thread overview]
Message-ID: <fa31fa72-59f5-3e47-026b-3673787b2b81@redhat.com> (raw)
In-Reply-To: <CAGQAs7wNF4KXiHGJmZzKZYCBDLm-dFbJktxQ2nX5Nihk4EdUdA@mail.gmail.com>
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>
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
>>
>
next prev parent reply other threads:[~2018-07-11 16:06 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
2018-07-11 15:44 ` Roman Bacik
2018-07-11 16:06 ` Laszlo Ersek [this message]
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=fa31fa72-59f5-3e47-026b-3673787b2b81@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