* [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
@ 2018-07-10 22:51 rbacik
2018-07-11 12:05 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: rbacik @ 2018-07-10 22:51 UTC (permalink / raw)
To: edk2-devel; +Cc: Chao Zhang, Jiewen Yao, Laszlo Ersek, Vladimir Olovyannikov
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(-)
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);
+ 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 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 ` (2 more replies) 2018-07-11 15:43 ` Roman Bacik 2018-07-16 15:09 ` Zhang, Chao B 2 siblings, 3 replies; 15+ messages in thread From: Laszlo Ersek @ 2018-07-11 12:05 UTC (permalink / raw) To: rbacik, edk2-devel Cc: Jiewen Yao, Vladimir Olovyannikov, Chao Zhang, Jordan Justen (Intel address), Ard Biesheuvel, Michael Kinney, Gao, Liming 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); > } > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 12:05 ` Laszlo Ersek @ 2018-07-11 12:15 ` Laszlo Ersek 2018-07-11 17:10 ` Carsey, Jaben 2018-07-11 14:16 ` Ard Biesheuvel 2018-07-11 15:44 ` Roman Bacik 2 siblings, 1 reply; 15+ messages in thread From: Laszlo Ersek @ 2018-07-11 12:15 UTC (permalink / raw) To: rbacik, edk2-devel Cc: Jiewen Yao, Vladimir Olovyannikov, Chao Zhang, Jordan Justen (Intel address), Ard Biesheuvel, Michael Kinney, Gao, Liming, Carsey, Jaben, Ruiyu Ni On 07/11/18 14:05, Laszlo Ersek wrote: > - 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! Note that I've mutually diffed the three function definitions between each other (with "diff -b"), and there are only whitespace and comment wrapping differences. So, again, this function looks like a prime suspect for UefiLib. I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE format"); I think that's simply stale, and should be removed. Thanks Laszlo ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 12:15 ` Laszlo Ersek @ 2018-07-11 17:10 ` Carsey, Jaben 2018-07-11 17:24 ` Laszlo Ersek 0 siblings, 1 reply; 15+ messages in thread From: Carsey, Jaben @ 2018-07-11 17:10 UTC (permalink / raw) To: Laszlo Ersek, rbacik@gmail.com, edk2-devel@lists.01.org Cc: Ni, Ruiyu, Vladimir Olovyannikov, Justen, Jordan L, Gao, Liming, Yao, Jiewen, Kinney, Michael D, Zhang, Chao B > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Wednesday, July 11, 2018 5:16 AM > To: rbacik@gmail.com; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Vladimir Olovyannikov > <vladimir.olovyannikov@broadcom.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Carsey, > Jaben <jaben.carsey@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Kinney, Michael D <michael.d.kinney@intel.com>; Zhang, Chao B > <chao.b.zhang@intel.com> > Subject: Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from > eMMC/SD/USB > Importance: High > > On 07/11/18 14:05, Laszlo Ersek wrote: > > > - 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! > > Note that I've mutually diffed the three function definitions between > each other (with "diff -b"), and there are only whitespace and comment > wrapping differences. > > So, again, this function looks like a prime suspect for UefiLib. > > I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE > format"); I think that's simply stale, and should be removed. Looks like the stale reference may be from origination as derived from ShellOpenFileByDevicePath. I agree this should be moved to a lib. Any way we could merge the shell's only slightly different version also into the mix? -Jaben > > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 17:10 ` Carsey, Jaben @ 2018-07-11 17:24 ` Laszlo Ersek 0 siblings, 0 replies; 15+ messages in thread From: Laszlo Ersek @ 2018-07-11 17:24 UTC (permalink / raw) To: Carsey, Jaben, rbacik@gmail.com, edk2-devel@lists.01.org Cc: Ni, Ruiyu, Vladimir Olovyannikov, Justen, Jordan L, Gao, Liming, Yao, Jiewen, Kinney, Michael D, Zhang, Chao B On 07/11/18 19:10, Carsey, Jaben wrote: > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, July 11, 2018 5:16 AM >> To: rbacik@gmail.com; edk2-devel@lists.01.org >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Vladimir Olovyannikov >> <vladimir.olovyannikov@broadcom.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Carsey, >> Jaben <jaben.carsey@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; >> Kinney, Michael D <michael.d.kinney@intel.com>; Zhang, Chao B >> <chao.b.zhang@intel.com> >> Subject: Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from >> eMMC/SD/USB >> Importance: High >> >> On 07/11/18 14:05, Laszlo Ersek wrote: >> >>> - 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! >> >> Note that I've mutually diffed the three function definitions between >> each other (with "diff -b"), and there are only whitespace and comment >> wrapping differences. >> >> So, again, this function looks like a prime suspect for UefiLib. >> >> I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE >> format"); I think that's simply stale, and should be removed. > > Looks like the stale reference may be from origination as derived from ShellOpenFileByDevicePath. I agree this should be moved to a lib. Any way we could merge the shell's only slightly different version also into the mix? Wow, that's incredible -- ShellOpenFileByDevicePath() contains the *exact same* alignment fix that Roman is contributing with this patch; from commit 0b6cb335fa82 ("Fixed some alignment faults in IPF platform", 2013-01-25). ShellOpenFileByDevicePath() -- including the alignment fix -- also has the same kind of resource leak (regarding "Handle1"). So, code duplication has bitten us again. We have four instances of basically the same logic in the tree, all four are affected by the same resource leak, and the alignment fix that was discovered in or before 2013 was applied to only one of the four. So, yes. The top of the ShellOpenFileByDevicePath() function (the "UEFI Shell 2.0 method") should be preserved, but then the last part ("use old shell method") should be replaced with a call to the new UefiLib API. And, the final "weak spot" comment remains valid, but only for ShellOpenFileByDevicePath(), not for the new EfiOpenFileByDevicePath() API. Thank you Jaben for finding this! Laszlo ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 12:05 ` Laszlo Ersek 2018-07-11 12:15 ` Laszlo Ersek @ 2018-07-11 14:16 ` Ard Biesheuvel 2018-07-11 15:44 ` Roman Bacik 2 siblings, 0 replies; 15+ messages in thread From: Ard Biesheuvel @ 2018-07-11 14:16 UTC (permalink / raw) To: Laszlo Ersek Cc: rbacik, edk2-devel@lists.01.org, Jiewen Yao, Vladimir Olovyannikov, Chao Zhang, Jordan Justen (Intel address), Michael Kinney, Gao, Liming 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); >> } >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 12:05 ` Laszlo Ersek 2018-07-11 12:15 ` Laszlo Ersek 2018-07-11 14:16 ` Ard Biesheuvel @ 2018-07-11 15:44 ` Roman Bacik 2018-07-11 16:06 ` Laszlo Ersek 2 siblings, 1 reply; 15+ messages in thread From: Roman Bacik @ 2018-07-11 15:44 UTC (permalink / raw) To: Laszlo Ersek Cc: Roman Bacik, edk2-devel, Vladimir Olovyannikov, Jordan Justen (Intel address), Gao, Liming, Jiewen Yao, Michael Kinney, Chao Zhang Hi Laszlo, Thank you very much for your review and help. I would prefer the option 2b. 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 15:44 ` Roman Bacik @ 2018-07-11 16:06 ` Laszlo Ersek 2018-07-11 21:06 ` Laszlo Ersek 0 siblings, 1 reply; 15+ messages in thread From: Laszlo Ersek @ 2018-07-11 16:06 UTC (permalink / raw) To: Roman Bacik, Jiewen Yao, Chao Zhang Cc: Roman Bacik, edk2-devel, Vladimir Olovyannikov, Jordan Justen (Intel address), Gao, Liming, Michael Kinney 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 >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 16:06 ` Laszlo Ersek @ 2018-07-11 21:06 ` Laszlo Ersek 2018-07-12 12:07 ` Yao, Jiewen 0 siblings, 1 reply; 15+ messages in thread From: Laszlo Ersek @ 2018-07-11 21:06 UTC (permalink / raw) To: Roman Bacik, Jiewen Yao, Chao Zhang Cc: Vladimir Olovyannikov, Jordan Justen (Intel address), edk2-devel, Gao, Liming, Michael Kinney On 07/11/18 18:06, Laszlo Ersek wrote: > 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> Here's the TianoCore BZ that I plan to reference in the updated commit message, as "further known problems": https://bugzilla.tianocore.org/show_bug.cgi?id=1008 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 >>> >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-11 21:06 ` Laszlo Ersek @ 2018-07-12 12:07 ` Yao, Jiewen 2018-07-12 21:42 ` Laszlo Ersek 0 siblings, 1 reply; 15+ messages in thread From: Yao, Jiewen @ 2018-07-12 12:07 UTC (permalink / raw) To: Laszlo Ersek Cc: Roman Bacik, Zhang, Chao B, Vladimir Olovyannikov, Justen, Jordan L, edk2-devel@lists.01.org, Gao, Liming, Kinney, Michael D, Yao, Jiewen thanks laszlo I like your idea to eliminate the code duplication and fix all the problem by adding a new api in uefi lib. If there is urgency to fix this specific issue, I have no problem on the enhancement. Reviewed by : Jiewen.yao@intel.com thank you! Yao, Jiewen > 在 2018年7月12日，上午5:06，Laszlo Ersek <lersek@redhat.com> 写道： > >> On 07/11/18 18:06, Laszlo Ersek wrote: >>> 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> > > Here's the TianoCore BZ that I plan to reference in the updated commit > message, as "further known problems": > > https://bugzilla.tianocore.org/show_bug.cgi?id=1008 > > 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 >>>> >>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-12 12:07 ` Yao, Jiewen @ 2018-07-12 21:42 ` Laszlo Ersek 0 siblings, 0 replies; 15+ messages in thread From: Laszlo Ersek @ 2018-07-12 21:42 UTC (permalink / raw) To: Yao, Jiewen Cc: Vladimir Olovyannikov, Justen, Jordan L, edk2-devel@lists.01.org, Gao, Liming, Kinney, Michael D, Zhang, Chao B On 07/12/18 14:07, Yao, Jiewen wrote: > thanks laszlo > I like your idea to eliminate the code duplication and fix all the problem by adding a new api in uefi lib. > > If there is urgency to fix this specific issue, I have no problem on the enhancement. > > Reviewed by : Jiewen.yao@intel.com > > thank you! > Yao, Jiewen Commit 79b10d4ce4f0. Thanks all! Laszlo ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 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 15:43 ` Roman Bacik 2018-07-16 15:09 ` Zhang, Chao B 2 siblings, 0 replies; 15+ messages in thread From: Roman Bacik @ 2018-07-11 15:43 UTC (permalink / raw) To: Roman Bacik Cc: edk2-devel, Laszlo Ersek, Jiewen Yao, Vladimir Olovyannikov, Chao Zhang Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> On Tue, Jul 10, 2018 at 3:51 PM, <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(-) > > 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); > + 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 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 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 15:43 ` Roman Bacik @ 2018-07-16 15:09 ` Zhang, Chao B 2018-07-16 15:50 ` Yao, Jiewen 2 siblings, 1 reply; 15+ messages in thread From: Zhang, Chao B @ 2018-07-16 15:09 UTC (permalink / raw) To: rbacik@gmail.com, edk2-devel@lists.01.org Cc: Yao, Jiewen, Laszlo Ersek, Vladimir Olovyannikov 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/SecureBootConfigFileExplorer.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) 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/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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-16 15:09 ` Zhang, Chao B @ 2018-07-16 15:50 ` Yao, Jiewen 2018-07-17 4:30 ` Roman Bacik 0 siblings, 1 reply; 15+ messages in thread From: Yao, Jiewen @ 2018-07-16 15:50 UTC (permalink / raw) To: Zhang, Chao B, rbacik@gmail.com, edk2-devel@lists.01.org Cc: Laszlo Ersek, Vladimir Olovyannikov 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB 2018-07-16 15:50 ` Yao, Jiewen @ 2018-07-17 4:30 ` Roman Bacik 0 siblings, 0 replies; 15+ messages in thread From: Roman Bacik @ 2018-07-17 4:30 UTC (permalink / raw) To: Yao, Jiewen Cc: Zhang, Chao B, rbacik@gmail.com, edk2-devel@lists.01.org, Laszlo Ersek, Vladimir Olovyannikov Yes, it is being taking care of. On Mon, Jul 16, 2018 at 8:50 AM, Yao, Jiewen <jiewen.yao@intel.com> wrote: > 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 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-07-17 4:31 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox