* Re: [edk2-devel] [PATCH] SecurityPkg/MeasureBootLib: Return EFI_ACCESS_DENIED after image check fail
2020-04-01 1:11 [PATCH] SecurityPkg/MeasureBootLib: Return EFI_ACCESS_DENIED after image check fail Guomin Jiang
@ 2020-04-08 10:46 ` Laszlo Ersek
0 siblings, 0 replies; 2+ messages in thread
From: Laszlo Ersek @ 2020-04-08 10:46 UTC (permalink / raw)
To: devel, guomin.jiang; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
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
> //
>
^ permalink raw reply [flat|nested] 2+ messages in thread