* [PATCH v2 1/6] SecurityPkg/TPM: measure UEFI images without associated device paths again
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
@ 2020-04-16 7:33 ` Guomin Jiang
2020-04-16 7:33 ` [PATCH v2 2/6] SecurityPkg/DxeImageAuth: Mark the File parameter as option Guomin Jiang
` (6 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Guomin Jiang @ 2020-04-16 7:33 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
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.
Fixes: 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.c | 20 +++++++++----------
.../DxeTpmMeasureBootLib.c | 20 +++++++++----------
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
index f0e95e5ec0..92eac71580 100644
--- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
+++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
@@ -384,8 +384,6 @@ Finish:
and other exception operations. The File parameter allows for possible logging
within the SAP of the driver.
- 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.
@@ -398,6 +396,8 @@ Finish:
might be possible to use it at a future time, then EFI_SECURITY_VIOLATION is
returned.
+ If check image specified by FileBuffer and File is NULL meanwhile, return EFI_ACCESS_DENIED.
+
@param[in] AuthenticationStatus This is the authentication status returned
from the securitymeasurement services for the
input file.
@@ -416,7 +416,7 @@ EFI_STATUS
EFIAPI
DxeTpm2MeasureBootHandler (
IN UINT32 AuthenticationStatus,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
@@ -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..d990eb2ad3 100644
--- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
+++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
@@ -678,8 +678,6 @@ Finish:
and other exception operations. The File parameter allows for possible logging
within the SAP of the driver.
- 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.
@@ -692,6 +690,8 @@ Finish:
might be possible to use it at a future time, then EFI_SECURITY_VIOLATION is
returned.
+ If check image specified by FileBuffer and File is NULL meanwhile, return EFI_ACCESS_DENIED.
+
@param[in] AuthenticationStatus This is the authentication status returned
from the securitymeasurement services for the
input file.
@@ -710,7 +710,7 @@ EFI_STATUS
EFIAPI
DxeTpmMeasureBootHandler (
IN UINT32 AuthenticationStatus,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
@@ -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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 2/6] SecurityPkg/DxeImageAuth: Mark the File parameter as option
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
2020-04-16 7:33 ` [PATCH v2 1/6] SecurityPkg/TPM: measure UEFI images without associated device paths again Guomin Jiang
@ 2020-04-16 7:33 ` Guomin Jiang
2020-04-16 7:33 ` [PATCH v2 3/6] SecurityPkg/DxeImageVerificationLib: Mark the File parameter as OPTIONAL Guomin Jiang
` (5 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Guomin Jiang @ 2020-04-16 7:33 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
According to the File description, The File is optional and can be NULL.
Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
---
.../DxeImageAuthenticationStatusLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageAuthenticationStatusLib/DxeImageAuthenticationStatusLib.c b/SecurityPkg/Library/DxeImageAuthenticationStatusLib/DxeImageAuthenticationStatusLib.c
index e4ddff21b5..ec77151c9c 100644
--- a/SecurityPkg/Library/DxeImageAuthenticationStatusLib/DxeImageAuthenticationStatusLib.c
+++ b/SecurityPkg/Library/DxeImageAuthenticationStatusLib/DxeImageAuthenticationStatusLib.c
@@ -32,7 +32,7 @@ EFI_STATUS
EFIAPI
DxeImageAuthenticationStatusHandler (
IN UINT32 AuthenticationStatus,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 3/6] SecurityPkg/DxeImageVerificationLib: Mark the File parameter as OPTIONAL
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
2020-04-16 7:33 ` [PATCH v2 1/6] SecurityPkg/TPM: measure UEFI images without associated device paths again Guomin Jiang
2020-04-16 7:33 ` [PATCH v2 2/6] SecurityPkg/DxeImageAuth: Mark the File parameter as option Guomin Jiang
@ 2020-04-16 7:33 ` Guomin Jiang
2020-04-16 7:33 ` [PATCH v2 4/6] MdeModulePkg/SecurityManagementLib: " Guomin Jiang
` (4 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Guomin Jiang @ 2020-04-16 7:33 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
According to the File description, the File is optional and can be NULL.
Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
---
.../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index b7fa8ea8c5..36b87e16d5 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1630,7 +1630,7 @@ EFI_STATUS
EFIAPI
DxeImageVerificationHandler (
IN UINT32 AuthenticationStatus,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 4/6] MdeModulePkg/SecurityManagementLib: Mark the File parameter as OPTIONAL
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
` (2 preceding siblings ...)
2020-04-16 7:33 ` [PATCH v2 3/6] SecurityPkg/DxeImageVerificationLib: Mark the File parameter as OPTIONAL Guomin Jiang
@ 2020-04-16 7:33 ` Guomin Jiang
2020-04-20 11:31 ` [edk2-devel] " Laszlo Ersek
2020-04-16 7:33 ` [PATCH v2 5/6] MdeModulePkg/SecurityStubDxe: " Guomin Jiang
` (3 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Guomin Jiang @ 2020-04-16 7:33 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Dandan Bi, Liming Gao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
According to the File description, the File is optional and can be NULL.
Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
MdeModulePkg/Include/Library/SecurityManagementLib.h | 2 +-
.../Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Include/Library/SecurityManagementLib.h b/MdeModulePkg/Include/Library/SecurityManagementLib.h
index c6ef8c38b9..9bb29121af 100644
--- a/MdeModulePkg/Include/Library/SecurityManagementLib.h
+++ b/MdeModulePkg/Include/Library/SecurityManagementLib.h
@@ -261,7 +261,7 @@ EFIAPI
ExecuteSecurity2Handlers (
IN UINT32 AuthenticationOperation,
IN UINT32 AuthenticationStatus,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
diff --git a/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c b/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
index b541dbab3c..de1eb53183 100644
--- a/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
+++ b/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
@@ -473,7 +473,7 @@ EFIAPI
ExecuteSecurity2Handlers (
IN UINT32 AuthenticationOperation,
IN UINT32 AuthenticationStatus,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH v2 4/6] MdeModulePkg/SecurityManagementLib: Mark the File parameter as OPTIONAL
2020-04-16 7:33 ` [PATCH v2 4/6] MdeModulePkg/SecurityManagementLib: " Guomin Jiang
@ 2020-04-20 11:31 ` Laszlo Ersek
0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2020-04-20 11:31 UTC (permalink / raw)
To: devel, guomin.jiang; +Cc: Jian J Wang, Hao A Wu, Dandan Bi, Liming Gao
On 04/16/20 09:33, Guomin Jiang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
>
> According to the File description, the File is optional and can be NULL.
>
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
> MdeModulePkg/Include/Library/SecurityManagementLib.h | 2 +-
> .../Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Include/Library/SecurityManagementLib.h b/MdeModulePkg/Include/Library/SecurityManagementLib.h
> index c6ef8c38b9..9bb29121af 100644
> --- a/MdeModulePkg/Include/Library/SecurityManagementLib.h
> +++ b/MdeModulePkg/Include/Library/SecurityManagementLib.h
> @@ -261,7 +261,7 @@ EFIAPI
> ExecuteSecurity2Handlers (
> IN UINT32 AuthenticationOperation,
> IN UINT32 AuthenticationStatus,
> - IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
> + IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
> IN VOID *FileBuffer,
> IN UINTN FileSize,
> IN BOOLEAN BootPolicy
> diff --git a/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c b/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
> index b541dbab3c..de1eb53183 100644
> --- a/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
> +++ b/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
> @@ -473,7 +473,7 @@ EFIAPI
> ExecuteSecurity2Handlers (
> IN UINT32 AuthenticationOperation,
> IN UINT32 AuthenticationStatus,
> - IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
> + IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
> IN VOID *FileBuffer,
> IN UINTN FileSize,
> IN BOOLEAN BootPolicy
>
The documentation of the ExecuteSecurity2Handlers() function again mixes
"File" and "DevicePath". All of those should be in sync.
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 5/6] MdeModulePkg/SecurityStubDxe: Mark the File parameter as OPTIONAL
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
` (3 preceding siblings ...)
2020-04-16 7:33 ` [PATCH v2 4/6] MdeModulePkg/SecurityManagementLib: " Guomin Jiang
@ 2020-04-16 7:33 ` Guomin Jiang
2020-04-16 7:33 ` [PATCH v2 6/6] MdePkg/Security2: " Guomin Jiang
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Guomin Jiang @ 2020-04-16 7:33 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Dandan Bi, Liming Gao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
According to the description, the File is optional and can be NULL
Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c b/MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c
index 3ed9527561..79f98b28e5 100644
--- a/MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c
+++ b/MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c
@@ -129,7 +129,7 @@ EFI_STATUS
EFIAPI
Security2StubAuthenticate (
IN CONST EFI_SECURITY2_ARCH_PROTOCOL *This,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *File,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *File, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
` (4 preceding siblings ...)
2020-04-16 7:33 ` [PATCH v2 5/6] MdeModulePkg/SecurityStubDxe: " Guomin Jiang
@ 2020-04-16 7:33 ` Guomin Jiang
2020-04-20 11:27 ` [edk2-devel] " Laszlo Ersek
2020-04-21 14:21 ` Liming Gao
2020-04-20 11:39 ` [edk2-devel] [PATCH v2 0/6] " Laszlo Ersek
2020-04-21 8:52 ` Wang, Jian J
7 siblings, 2 replies; 16+ messages in thread
From: Guomin Jiang @ 2020-04-16 7:33 UTC (permalink / raw)
To: devel; +Cc: Michael D Kinney, Liming Gao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
According to the description, the File is OPTIONAL and can be NULL.
Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
MdePkg/Include/Protocol/Security2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdePkg/Include/Protocol/Security2.h b/MdePkg/Include/Protocol/Security2.h
index 2d85b4ba9f..6a63009956 100644
--- a/MdePkg/Include/Protocol/Security2.h
+++ b/MdePkg/Include/Protocol/Security2.h
@@ -80,7 +80,7 @@ typedef struct _EFI_SECURITY2_ARCH_PROTOCOL EFI_SECURITY2_ARCH_PROTOCOL;
**/
typedef EFI_STATUS (EFIAPI *EFI_SECURITY2_FILE_AUTHENTICATION) (
IN CONST EFI_SECURITY2_ARCH_PROTOCOL *This,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath,
+ IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath, OPTIONAL
IN VOID *FileBuffer,
IN UINTN FileSize,
IN BOOLEAN BootPolicy
--
2.25.1.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
2020-04-16 7:33 ` [PATCH v2 6/6] MdePkg/Security2: " Guomin Jiang
@ 2020-04-20 11:27 ` Laszlo Ersek
2020-04-21 14:21 ` Liming Gao
1 sibling, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2020-04-20 11:27 UTC (permalink / raw)
To: devel, guomin.jiang; +Cc: Michael D Kinney, Liming Gao
On 04/16/20 09:33, Guomin Jiang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
>
> According to the description, the File is OPTIONAL and can be NULL.
>
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
> MdePkg/Include/Protocol/Security2.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdePkg/Include/Protocol/Security2.h b/MdePkg/Include/Protocol/Security2.h
> index 2d85b4ba9f..6a63009956 100644
> --- a/MdePkg/Include/Protocol/Security2.h
> +++ b/MdePkg/Include/Protocol/Security2.h
> @@ -80,7 +80,7 @@ typedef struct _EFI_SECURITY2_ARCH_PROTOCOL EFI_SECURITY2_ARCH_PROTOCOL;
> **/
> typedef EFI_STATUS (EFIAPI *EFI_SECURITY2_FILE_AUTHENTICATION) (
> IN CONST EFI_SECURITY2_ARCH_PROTOCOL *This,
> - IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath,
> + IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath, OPTIONAL
> IN VOID *FileBuffer,
> IN UINTN FileSize,
> IN BOOLEAN BootPolicy
>
(1) I think writing
DevicePath OPTIONAL,
is a bit more idiomatic than
DevicePath, OPTIONAL
However, the UEFI spec contains examples for both styles of comma
placement in parameter lists; and so I think this change is valid,
albeit somewhat unusual. So that's good.
(2) Unfortunately, I don't think the patch is complete. If you look at
the entire function declaration in the header file, the documentation
*inconsistently* calls the parameter both "File" and "DevicePath". If
you search the leading comment, there are both "File" and "DevicePath"
references.
Therefore the reference in the commit message, "According to the
description, the File is OPTIONAL", is actually a dangling reference,
because *that* particular part of the documentation calls the parameter
"File".
Please rework this patch so that the documentation and the parameter
list fully agree on the parameter's name.
According to the platform init spec (1.7),
EFI_SECURITY2_ARCH_PROTOCOL.FileAuthentication(), the name of the
parameter is "DevicePath". So please replace all relevant "File"
references in the leading comment with "DevicePath".
(3) Also, I think it would be prudent to file a ticket in Mantis, to
mark "DevicePath" as OPTIONAL in the spec too.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
2020-04-16 7:33 ` [PATCH v2 6/6] MdePkg/Security2: " Guomin Jiang
2020-04-20 11:27 ` [edk2-devel] " Laszlo Ersek
@ 2020-04-21 14:21 ` Liming Gao
1 sibling, 0 replies; 16+ messages in thread
From: Liming Gao @ 2020-04-21 14:21 UTC (permalink / raw)
To: devel@edk2.groups.io, Jiang, Guomin; +Cc: Kinney, Michael D
Guomin:
I find this parameter name doesn't match the one in function description. Can you make them be same? With this change, Reviewed-by: Liming Gao <liming.gao@intel.com>
@param File A pointer to the device path of the file that is
being dispatched. This will optionally be used for logging.
Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin Jiang
> Sent: Thursday, April 16, 2020 3:34 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
>
> According to the description, the File is OPTIONAL and can be NULL.
>
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
> MdePkg/Include/Protocol/Security2.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdePkg/Include/Protocol/Security2.h b/MdePkg/Include/Protocol/Security2.h
> index 2d85b4ba9f..6a63009956 100644
> --- a/MdePkg/Include/Protocol/Security2.h
> +++ b/MdePkg/Include/Protocol/Security2.h
> @@ -80,7 +80,7 @@ typedef struct _EFI_SECURITY2_ARCH_PROTOCOL EFI_SECURITY2_ARCH_PROTOCOL;
> **/
>
> typedef EFI_STATUS (EFIAPI *EFI_SECURITY2_FILE_AUTHENTICATION) (
>
> IN CONST EFI_SECURITY2_ARCH_PROTOCOL *This,
>
> - IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath,
>
> + IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath, OPTIONAL
>
> IN VOID *FileBuffer,
>
> IN UINTN FileSize,
>
> IN BOOLEAN BootPolicy
>
> --
> 2.25.1.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#57460): https://edk2.groups.io/g/devel/message/57460
> Mute This Topic: https://groups.io/mt/73050539/1759384
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming.gao@intel.com]
> -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
` (5 preceding siblings ...)
2020-04-16 7:33 ` [PATCH v2 6/6] MdePkg/Security2: " Guomin Jiang
@ 2020-04-20 11:39 ` Laszlo Ersek
2020-04-21 2:15 ` Guomin Jiang
2020-04-21 8:52 ` Wang, Jian J
7 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2020-04-20 11:39 UTC (permalink / raw)
To: devel, guomin.jiang
On 04/16/20 09:33, Guomin Jiang wrote:
> File paramter should be optional according to the descritpion.
>
> Guomin Jiang (6):
> SecurityPkg/TPM: measure UEFI images without associated device paths
> again
> SecurityPkg/DxeImageAuth: Mark the File parameter as option
> SecurityPkg/DxeImageVerificationLib: Mark the File parameter as
> OPTIONAL
> MdeModulePkg/SecurityManagementLib: Mark the File parameter as
> OPTIONAL
> MdeModulePkg/SecurityStubDxe: Mark the File parameter as OPTIONAL
> MdePkg/Security2: Mark the File parameter as OPTIONAL.
>
> .../Include/Library/SecurityManagementLib.h | 2 +-
> .../DxeSecurityManagementLib.c | 2 +-
> .../Universal/SecurityStubDxe/SecurityStub.c | 2 +-
> MdePkg/Include/Protocol/Security2.h | 2 +-
> .../DxeImageAuthenticationStatusLib.c | 2 +-
> .../DxeImageVerificationLib.c | 2 +-
> .../DxeTpm2MeasureBootLib.c | 20 +++++++++----------
> .../DxeTpmMeasureBootLib.c | 20 +++++++++----------
> 8 files changed, 26 insertions(+), 26 deletions(-)
>
The more I look at the pre-patch code, the more the pre-existent
documentation inconsistencies irritate me.
I withdraw from reviewing this series. Proceed as you and other
reviewers see fit.
If you prefer, go ahead and simply revert 4b026f0d5af3. If you do so, I
will not review that patch either. The existent function-level comments
are broken, so I don't think anyone can really rely on them for guidance.
If you want to do the right thing, the whole comment mess has to be
cleaned up. Up to you, but I'm out.
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL
2020-04-20 11:39 ` [edk2-devel] [PATCH v2 0/6] " Laszlo Ersek
@ 2020-04-21 2:15 ` Guomin Jiang
0 siblings, 0 replies; 16+ messages in thread
From: Guomin Jiang @ 2020-04-21 2:15 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Hi Laszlo,
I'm sad when saw the message. but I still appreciate the advice provided by you.
I think that this is not the only documentation issue, and should discuss in other topic.
Hi Jian, Chao,
Just for this issue, please give some feedback.
Best Regards
Guomin
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, April 20, 2020 7:39 PM
> To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as
> OPTIONAL
>
> On 04/16/20 09:33, Guomin Jiang wrote:
> > File paramter should be optional according to the descritpion.
> >
> > Guomin Jiang (6):
> > SecurityPkg/TPM: measure UEFI images without associated device paths
> > again
> > SecurityPkg/DxeImageAuth: Mark the File parameter as option
> > SecurityPkg/DxeImageVerificationLib: Mark the File parameter as
> > OPTIONAL
> > MdeModulePkg/SecurityManagementLib: Mark the File parameter as
> > OPTIONAL
> > MdeModulePkg/SecurityStubDxe: Mark the File parameter as OPTIONAL
> > MdePkg/Security2: Mark the File parameter as OPTIONAL.
> >
> > .../Include/Library/SecurityManagementLib.h | 2 +-
> > .../DxeSecurityManagementLib.c | 2 +-
> > .../Universal/SecurityStubDxe/SecurityStub.c | 2 +-
> > MdePkg/Include/Protocol/Security2.h | 2 +-
> > .../DxeImageAuthenticationStatusLib.c | 2 +-
> > .../DxeImageVerificationLib.c | 2 +-
> > .../DxeTpm2MeasureBootLib.c | 20 +++++++++----------
> > .../DxeTpmMeasureBootLib.c | 20 +++++++++----------
> > 8 files changed, 26 insertions(+), 26 deletions(-)
> >
>
> The more I look at the pre-patch code, the more the pre-existent
> documentation inconsistencies irritate me.
>
> I withdraw from reviewing this series. Proceed as you and other reviewers
> see fit.
>
> If you prefer, go ahead and simply revert 4b026f0d5af3. If you do so, I will not
> review that patch either. The existent function-level comments are broken,
> so I don't think anyone can really rely on them for guidance.
>
> If you want to do the right thing, the whole comment mess has to be cleaned
> up. Up to you, but I'm out.
>
> Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL
2020-04-16 7:33 [PATCH v2 0/6] Mark the File parameter as OPTIONAL Guomin Jiang
` (6 preceding siblings ...)
2020-04-20 11:39 ` [edk2-devel] [PATCH v2 0/6] " Laszlo Ersek
@ 2020-04-21 8:52 ` Wang, Jian J
7 siblings, 0 replies; 16+ messages in thread
From: Wang, Jian J @ 2020-04-21 8:52 UTC (permalink / raw)
To: devel@edk2.groups.io, Jiang, Guomin
For the patch series,
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Regards,
Jian
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
> Jiang
> Sent: Thursday, April 16, 2020 3:34 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL
>
> File paramter should be optional according to the descritpion.
>
> Guomin Jiang (6):
> SecurityPkg/TPM: measure UEFI images without associated device paths
> again
> SecurityPkg/DxeImageAuth: Mark the File parameter as option
> SecurityPkg/DxeImageVerificationLib: Mark the File parameter as
> OPTIONAL
> MdeModulePkg/SecurityManagementLib: Mark the File parameter as
> OPTIONAL
> MdeModulePkg/SecurityStubDxe: Mark the File parameter as OPTIONAL
> MdePkg/Security2: Mark the File parameter as OPTIONAL.
>
> .../Include/Library/SecurityManagementLib.h | 2 +-
> .../DxeSecurityManagementLib.c | 2 +-
> .../Universal/SecurityStubDxe/SecurityStub.c | 2 +-
> MdePkg/Include/Protocol/Security2.h | 2 +-
> .../DxeImageAuthenticationStatusLib.c | 2 +-
> .../DxeImageVerificationLib.c | 2 +-
> .../DxeTpm2MeasureBootLib.c | 20 +++++++++----------
> .../DxeTpmMeasureBootLib.c | 20 +++++++++----------
> 8 files changed, 26 insertions(+), 26 deletions(-)
>
> --
> 2.25.1.windows.1
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread