[PATCH v2 0/6] Mark the File parameter as OPTIONAL

[PATCH v2 0/6] Mark the File parameter as OPTIONAL

[Guomin Jiang]

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

[PATCH v2 1/6] SecurityPkg/TPM: measure UEFI images without associated device paths again

[Guomin Jiang]

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

[PATCH v2 2/6] SecurityPkg/DxeImageAuth: Mark the File parameter as option

[Guomin Jiang]

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

[PATCH v2 3/6] SecurityPkg/DxeImageVerificationLib: Mark the File parameter as OPTIONAL

[Guomin Jiang]

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

[PATCH v2 4/6] MdeModulePkg/SecurityManagementLib: Mark the File parameter as OPTIONAL

[Guomin Jiang]

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

[PATCH v2 5/6] MdeModulePkg/SecurityStubDxe: Mark the File parameter as OPTIONAL

[Guomin Jiang]

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

[PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.

[Guomin Jiang]

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

Re: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH v2 4/6] MdeModulePkg/SecurityManagementLib: Mark the File parameter as OPTIONAL

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL

[Guomin Jiang]

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

Re: [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL

[Wang, Jian J]

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
> 
> 
> 

Re: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.

[Liming Gao]

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]
> -=-=-=-=-=-=