.../DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c | 14 +++++++------- .../DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
If check the File at the begin of function, it will only allow the File is
present and forbid image from buffer.
It is possible that image come from the memory buffer, so make it can run
and check the File after it.
It is improvement for 4b026f0d5af36faf3a3629a3ad49c51b5b3be12f.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
---
.../DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c | 14 +++++++-------
.../DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c | 14 +++++++-------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
index f0e95e5ec0..fdb4758cbe 100644
--- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
+++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
@@ -435,13 +435,6 @@ DxeTpm2MeasureBootHandler (
EFI_PHYSICAL_ADDRESS FvAddress;
UINT32 Index;
- //
- // Check for invalid parameters.
- //
- if (File == NULL) {
- return EFI_ACCESS_DENIED;
- }
-
Status = gBS->LocateProtocol (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol);
if (EFI_ERROR (Status)) {
//
@@ -615,6 +608,13 @@ DxeTpm2MeasureBootHandler (
//
Status = PeCoffLoaderGetImageInfo (&ImageContext);
if (EFI_ERROR (Status)) {
+ //
+ // Check for invalid parameters.
+ //
+ if (File == NULL) {
+ Status = EFI_ACCESS_DENIED;
+ }
+
//
// The information can't be got from the invalid PeImage
//
diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
index d499371e7a..20f7d94d6b 100644
--- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
+++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
@@ -732,13 +732,6 @@ DxeTpmMeasureBootHandler (
EFI_PHYSICAL_ADDRESS FvAddress;
UINT32 Index;
- //
- // Check for invalid parameters.
- //
- if (File == NULL) {
- return EFI_ACCESS_DENIED;
- }
-
Status = gBS->LocateProtocol (&gEfiTcgProtocolGuid, NULL, (VOID **) &TcgProtocol);
if (EFI_ERROR (Status)) {
//
@@ -912,6 +905,13 @@ DxeTpmMeasureBootHandler (
//
Status = PeCoffLoaderGetImageInfo (&ImageContext);
if (EFI_ERROR (Status)) {
+ //
+ // Check for invalid parameters.
+ //
+ if (File == NULL) {
+ return EFI_ACCESS_DENIED;
+ }
+
//
// The information can't be got from the invalid PeImage
//
--
2.25.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56805): https://edk2.groups.io/g/devel/message/56805
Mute This Topic: https://groups.io/mt/72691331/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi, On 04/01/20 03:11, Guomin Jiang wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652 > > If check the File at the begin of function, it will only allow the File is > present and forbid image from buffer. > It is possible that image come from the memory buffer, so make it can run > and check the File after it. Unfortunately, this commit message is unhelpful. The use case you are trying to describe is presumably the following: the DxeTpm2MeasureBootHandler() and DxeTpmMeasureBootHandler() functions may receive a FileBuffer argument that is not associated with any particular device path (e.g., because the UEFI image has not been loaded from any particular device path). Therefore rejecting (File==NULL) at the top of the function is invalid. (1) So please state this clearly in the commit message. The subject line is similarly incomprehensible. It should state the *goal* of the patch. Something like: SecurityPkg/TPM: measure UEFI images without associated device paths again (74 characters) (2) However, this is a use case that currently does not fully conform to the SECURITY2_FILE_AUTHENTICATION_HANDLER specification in "MdeModulePkg/Include/Library/SecurityManagementLib.h": - The documentation for "@param[in] File" does not explain what happens when File is NULL. The description says "This will optionally be used for logging" -- dependent on what? The statement means, "Maybe we'll use File for logging, maybe we won't". It doesn't explain what the decision depends on, and it also doesn't explain what *else* the File parameter could be used to. The generic description is "The pointer to the device path of the file that is being dispatched", and passing NULL for that is not valid. - In the actual parameter list, "File" is not annotated as OPTIONAL. So please fix up the lib class header *first*. If the documentation had been complete when Phil was working on 4b026f0d5af3 ("SecurityPkg: Fix incorrect return value when File is NULL", 2020-02-10), then the use case in question would not have been regressed. (Because then we'd have understood that (File == NULL) is valid, under certain circumstances.) > It is improvement for 4b026f0d5af36faf3a3629a3ad49c51b5b3be12f. In fact 4b026f0d5af3 introduced a regression, so this patch is a regression fix. Please mark TianoCore#2652 with the Regression keyword, and add the following to the commit message here: Fixes: 4b026f0d5af36faf3a3629a3ad49c51b5b3be12f Regarding the logic change, I'll let the SecurityPkg maintainers / reviewers verify that. Thanks Laszlo > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Signed-off-by: Guomin Jiang <guomin.jiang@intel.com> > --- > .../DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c | 14 +++++++------- > .../DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c | 14 +++++++------- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > index f0e95e5ec0..fdb4758cbe 100644 > --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > @@ -435,13 +435,6 @@ DxeTpm2MeasureBootHandler ( > EFI_PHYSICAL_ADDRESS FvAddress; > UINT32 Index; > > - // > - // Check for invalid parameters. > - // > - if (File == NULL) { > - return EFI_ACCESS_DENIED; > - } > - > Status = gBS->LocateProtocol (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol); > if (EFI_ERROR (Status)) { > // > @@ -615,6 +608,13 @@ DxeTpm2MeasureBootHandler ( > // > Status = PeCoffLoaderGetImageInfo (&ImageContext); > if (EFI_ERROR (Status)) { > + // > + // Check for invalid parameters. > + // > + if (File == NULL) { > + Status = EFI_ACCESS_DENIED; > + } > + > // > // The information can't be got from the invalid PeImage > // > diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > index d499371e7a..20f7d94d6b 100644 > --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > +++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > @@ -732,13 +732,6 @@ DxeTpmMeasureBootHandler ( > EFI_PHYSICAL_ADDRESS FvAddress; > UINT32 Index; > > - // > - // Check for invalid parameters. > - // > - if (File == NULL) { > - return EFI_ACCESS_DENIED; > - } > - > Status = gBS->LocateProtocol (&gEfiTcgProtocolGuid, NULL, (VOID **) &TcgProtocol); > if (EFI_ERROR (Status)) { > // > @@ -912,6 +905,13 @@ DxeTpmMeasureBootHandler ( > // > Status = PeCoffLoaderGetImageInfo (&ImageContext); > if (EFI_ERROR (Status)) { > + // > + // Check for invalid parameters. > + // > + if (File == NULL) { > + return EFI_ACCESS_DENIED; > + } > + > // > // The information can't be got from the invalid PeImage > // > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57063): https://edk2.groups.io/g/devel/message/57063 Mute This Topic: https://groups.io/mt/72691331/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.