Hi Phil, On 02/04/20 23:26, Philippe Mathieu-Daudé wrote: > The DxeTpmMeasureBootHandler and DxeTpm2MeasureBootHandler handlers > are SECURITY2_FILE_AUTHENTICATION_HANDLER prototype. This prototype > can not return EFI_INVALID_PARAMETER. > > The prototype documentation states it returns EFI_ACCESS_DENIED if: > > "The file specified by File and FileBuffer did not authenticate, > and the platform policy dictates that the DXE Foundation may not > use File." > > Note in practice both functions return EFI_SECURITY_VIOLATION: > > "The file specified by DevicePath and FileBuffer did not > authenticate, and the platform policy dictates that the file > should be placed in the untrusted state. The image has been > added to the file execution table." > > Noticed while reviewing commit 6d57592740cdd0b6868baeef7929d6e6fef. > > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Jian J Wang <jian.j.w...@intel.com> > Cc: Chao Zhang <chao.b.zh...@intel.com> > Signed-off-by: Philippe Mathieu-Daude <phi...@redhat.com> > --- > .../Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c | 2 +- > SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git > a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > index 04b9b0d7fbf3..0c07f65c6835 100644 > --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > @@ -384,7 +384,7 @@ Finish: > and other exception operations. The File parameter allows for possible > logging > within the SAP of the driver. > > - If File is NULL, then EFI_INVALID_PARAMETER is returned. > + If File is NULL, then EFI_ACCESS_DENIED is returned. > > If the file specified by File with an authentication status specified by > AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is > returned.
I've tried seeing where this code actually returns EFI_INVALID_PARAMETER, but I can't find it. If File is NULL in DxeTpm2MeasureBootHandler(), then first we seem to do: OrigDevicePathNode = DuplicateDevicePath (File); which I believe will produce a NULL. Then we seem to pass this NULL around a little bit, so I think there's a fair chance of crashing there. I wonder if this code should be fixed, to check "File" in the first place, and then return EFI_ACCESS_DENIED. There are also some other places in the function that could apparently return a wide array of error codes -- FvbProtocol->GetPhysicalAddress() (EFI_UNSUPPORTED?), PeCoffLoaderGetImageInfo(), etc. I do agree this patch is an improvement, however; at least it says what *should* be returned. So with that in mind: Acked-by: Laszlo Ersek <ler...@redhat.com> Thanks Laszlo > diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > index 1f2eed29a1df..0dccbb66eb26 100644 > --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > +++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > @@ -678,7 +678,7 @@ Finish: > and other exception operations. The File parameter allows for possible > logging > within the SAP of the driver. > > - If File is NULL, then EFI_INVALID_PARAMETER is returned. > + If File is NULL, then EFI_ACCESS_DENIED is returned. > > If the file specified by File with an authentication status specified by > AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is > returned. > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53793): https://edk2.groups.io/g/devel/message/53793 Mute This Topic: https://groups.io/mt/70984329/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
