From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.8912.1586342779412504770 for ; Wed, 08 Apr 2020 03:46:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FfMsR4/r; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586342778; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/1HqeLztckWda9gPyl+ICpcLZ8+TnEfD06JCO3ixqtg=; b=FfMsR4/rcz23wM5XgF/TwSgiHjF8PcW615OUoV3+4+p8dlq5rbV3tJXOrSHP1cNrfCexo3 ZchipCOGzae3uVqYImc/psEdJi2pDUJecY6xNKgngXbV8LyVdxXaDn+LzCYzJLnCfh4XT6 jj5jSIeJX4nr/QZLYZcvdQZCNPoC294= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-386-iUjMr_1kOEmp8Jw6A6KTiA-1; Wed, 08 Apr 2020 06:46:07 -0400 X-MC-Unique: iUjMr_1kOEmp8Jw6A6KTiA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 71E6F8017F5; Wed, 8 Apr 2020 10:46:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-122.ams2.redhat.com [10.36.114.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1A26C7E303; Wed, 8 Apr 2020 10:46:04 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] SecurityPkg/MeasureBootLib: Return EFI_ACCESS_DENIED after image check fail To: devel@edk2.groups.io, guomin.jiang@intel.com Cc: Jiewen Yao , Jian J Wang , Chao Zhang References: <20200401011113.624-1-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: <461c9fd7-b4f4-2920-49e1-7c089bbee78b@redhat.com> Date: Wed, 8 Apr 2020 12:46:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200401011113.624-1-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > Cc: Jian J Wang > Cc: Chao Zhang > Signed-off-by: Guomin Jiang > --- > .../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 > // >