[PATCH v1 1/7] FmpDevicePkg/FmpDependencyLib: Correct ValidateDependency() documentation

[PATCH v1 1/7] FmpDevicePkg/FmpDependencyLib: Correct ValidateDependency() documentation

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

Modifies the return value documentation to state that the BOOLEAN
value indicates whether a given dependency expression is valid
not a capsule.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 5 +++--
 FmpDevicePkg/Include/Library/FmpDependencyLib.h          | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
index 91dc0b9abd33..28358069950a 100644
--- a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
+++ b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
@@ -1,6 +1,7 @@
 /** @file
   Supports Fmp Capsule Dependency Expression.
 
+  Copyright (c) Microsoft Corporation.<BR>
   Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -433,8 +434,8 @@ EvaluateDependency (
   @param[in]   MaxDepexSize   Max size of the dependency.
   @param[out]  DepexSize      Size of dependency.
 
-  @retval TRUE    The capsule is valid.
-  @retval FALSE   The capsule is invalid.
+  @retval TRUE    The dependency expression is valid.
+  @retval FALSE   The dependency expression is invalid.
 
 **/
 BOOLEAN
diff --git a/FmpDevicePkg/Include/Library/FmpDependencyLib.h b/FmpDevicePkg/Include/Library/FmpDependencyLib.h
index 1110eefa9a54..c732903425b4 100644
--- a/FmpDevicePkg/Include/Library/FmpDependencyLib.h
+++ b/FmpDevicePkg/Include/Library/FmpDependencyLib.h
@@ -2,6 +2,7 @@
   Fmp Capsule Dependency support functions for Firmware Management Protocol based
   firmware updates.
 
+  Copyright (c) Microsoft Corporation.<BR>
   Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -29,8 +30,8 @@ typedef struct {
   @param[in]   MaxDepexSize   Max size of the dependency.
   @param[out]  DepexSize      Size of dependency.
 
-  @retval TRUE    The capsule is valid.
-  @retval FALSE   The capsule is invalid.
+  @retval TRUE    The dependency expression is valid.
+  @retval FALSE   The dependency expression is invalid.
 
 **/
 BOOLEAN
-- 
2.27.0.windows.1

[PATCH v1 2/7] FmpDevicePkg/FmpDependencyLib: Fix "exression" typo

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
index 28358069950a..ba89eb22d9f0 100644
--- a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
+++ b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
@@ -421,7 +421,7 @@ EvaluateDependency (
     Iterator++;
   }
 
-  DEBUG ((DEBUG_ERROR, "EvaluateDependency: No EFI_FMP_DEP_END Opcode in exression!\n"));
+  DEBUG ((DEBUG_ERROR, "EvaluateDependency: No EFI_FMP_DEP_END Opcode in expression!\n"));
 
 Error:
   return FALSE;
-- 
2.27.0.windows.1

[PATCH v1 3/7] FmpDevicePkg/FmpDependencyLib: Handle version string overflow

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

This change recognizes the condition of the DEPEX version string
extending beyond the end of the dependency expression as an error.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---

Notes:
    This is particularly helpful for the user to isolate the issue
    when stepping through the control flow as this case will be the
    last executed before jumping to the Error label to return from
    the function.

 FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
index ba89eb22d9f0..5ef25d2415cf 100644
--- a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
+++ b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
@@ -286,6 +286,7 @@ EvaluateDependency (
       Iterator += AsciiStrnLenS ((CHAR8 *) Iterator, DependenciesSize - (Iterator - Dependencies->Dependencies));
       if (Iterator == (UINT8 *) Dependencies->Dependencies + DependenciesSize) {
         DEBUG ((DEBUG_ERROR, "EvaluateDependency: STRING extends beyond end of dependency expression!\n"));
+        goto Error;
       }
       break;
     case EFI_FMP_DEP_AND:
-- 
2.27.0.windows.1

[PATCH v1 4/7] FmpDevicePkg/FmpDependencyCheckLib: Return unsatisfied on handle failure

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

CheckFmpDependency () will currently return that dependencies are
satisfied if the initial call in the function to locate handles
that have gEfiFirmwareManagementProtocolGuid installed fails.

This change updates the error handling to return FALSE (dependencies
are not satisfied) if this handle search fails.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c b/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
index 5e0241b25957..02ed600e0e95 100644
--- a/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
+++ b/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
@@ -2,6 +2,7 @@
   Provides FMP capsule dependency check services when updating the firmware
   image of a FMP device.
 
+  Copyright (c) Microsoft Corporation.<BR>
   Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -75,6 +76,7 @@ CheckFmpDependency (
                 );
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "CheckFmpDependency: Get Firmware Management Protocol failed. (%r)", Status));
+    IsSatisfied = FALSE;
     goto cleanup;
   }
 
-- 
2.27.0.windows.1

[PATCH v1 5/7] FmpDevicePkg/FmpDxe: Better warn of potential ImageTypeId misconfig

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

A user may fall through to the case they depend on the
PcdFmpDeviceImageTypeIdGuid value to get the ImageTypeId GUID
value. The default PCD value is 0 (NULL) so the code would
further fall back on the gEfiCallerIdGuid value.

This change modifies the print error level for the message that
indicates this occurred to DEBUG_WARN from DEBUG_INFO to better
warn the user that this occurred.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---

Notes:
    The PCD documentation indicates it is valid behavior for the user
    to fall back to gEfiCallerIdGuid. Is that really expected?
    
    Would an ASSERT be appropriate?

 FmpDevicePkg/FmpDxe/FmpDxe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index 58841774fee0..14994ce4ee0e 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.c
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
@@ -3,7 +3,7 @@
   image stored in a firmware device with platform and firmware device specific
   information provided through PCDs and libraries.
 
-  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
+  Copyright (c) Microsoft Corporation.<BR>
   Copyright (c) 2018 - 2020, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -179,7 +179,7 @@ GetImageTypeIdGuid (
     if (ImageTypeIdGuidSize == sizeof (EFI_GUID)) {
       FmpDeviceLibGuid = (EFI_GUID *)PcdGetPtr (PcdFmpDeviceImageTypeIdGuid);
     } else {
-      DEBUG ((DEBUG_INFO, "FmpDxe(%s): Fall back to ImageTypeIdGuid of gEfiCallerIdGuid\n", mImageIdName));
+      DEBUG ((DEBUG_WARN, "FmpDxe(%s): Fall back to ImageTypeIdGuid of gEfiCallerIdGuid\n", mImageIdName));
       FmpDeviceLibGuid = &gEfiCallerIdGuid;
     }
   }
-- 
2.27.0.windows.1

[PATCH v1 6/7] FmpDevicePkg/FmpDxe: Indicate ESRT GUID on invalid ImageIdName

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

Updates the debug error message to include the GUID of the FMP
instance that encountered the issue to help the user better
isolate the problem.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 FmpDevicePkg/FmpDxe/FmpDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index 14994ce4ee0e..a3e342591936 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.c
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
@@ -1679,7 +1679,7 @@ FmpDxeEntryPoint (
     //
     // PcdFmpDeviceImageIdName must be set to a non-empty Unicode string
     //
-    DEBUG ((DEBUG_ERROR, "FmpDxe: PcdFmpDeviceImageIdName is an empty string.\n"));
+    DEBUG ((DEBUG_ERROR, "FmpDxe(%g): PcdFmpDeviceImageIdName is an empty string.\n", &gEfiCallerIdGuid));
     ASSERT (FALSE);
     return EFI_UNSUPPORTED;
   }
-- 
2.27.0.windows.1

[PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2869

Makes some minor improvements to function parameter validation
in FmpDxe, in particular to externally exposed functions such
as those that back EFI_FIRMWARE_MANAGEMENT_PROTOCOL.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 FmpDevicePkg/FmpDxe/FmpDxe.c | 56 +++++++++++++++++---
 FmpDevicePkg/FmpDxe/FmpDxe.h | 10 ++--
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index a3e342591936..958d9b394b71 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.c
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
@@ -278,6 +278,11 @@ PopulateDescriptor (
   EFI_STATUS  Status;
   UINT32      DependenciesSize;
 
+  if (Private == NULL) {
+    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): PopulateDescriptor() - Private is NULL.\n", mImageIdName));
+    return;
+  }
+
   if (Private->DescriptorPopulated) {
     return;
   }
@@ -429,7 +434,7 @@ PopulateDescriptor (
   @retval EFI_SUCCESS                The device was successfully updated with the new image.
   @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
                                      needed to hold the image(s) information is returned in ImageInfoSize.
-  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
+  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
   @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
 
 **/
@@ -451,6 +456,12 @@ GetTheImageInfo (
 
   Status = EFI_SUCCESS;
 
+  if (This == NULL) {
+    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - This is NULL.\n", mImageIdName));
+    Status = EFI_INVALID_PARAMETER;
+    goto cleanup;
+  }
+
   //
   // Retrieve the private context structure
   //
@@ -536,7 +547,7 @@ GetTheImageInfo (
   @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
                                  image. The current buffer size needed to hold the image is returned
                                  in ImageSize.
-  @retval EFI_INVALID_PARAMETER  The Image was NULL.
+  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
   @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
   @retval EFI_UNSUPPORTED        The operation is not supported.
   @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
@@ -561,6 +572,12 @@ GetTheImage (
 
   Status = EFI_SUCCESS;
 
+  if (This == NULL) {
+    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImage() - This is NULL.\n", mImageIdName));
+    Status = EFI_INVALID_PARAMETER;
+    goto cleanup;
+  }
+
   //
   // Retrieve the private context structure
   //
@@ -615,7 +632,8 @@ GetTheImage (
   @param[in]   Image                 Pointer to the image.
   @param[in]   ImageSize             Size of the image.
   @param[in]   AdditionalHeaderSize  Size of any headers that cannot be calculated by this function.
-  @param[out]  PayloadSize
+  @param[out]  PayloadSize           An optional pointer to a UINTN that holds the size of the payload
+                                     (image size minus headers)
 
   @retval  !NULL  Valid pointer to the header.
   @retval  NULL   Structure is bad and pointer cannot be found.
@@ -626,7 +644,7 @@ GetFmpHeader (
   IN  CONST EFI_FIRMWARE_IMAGE_AUTHENTICATION  *Image,
   IN  CONST UINTN                              ImageSize,
   IN  CONST UINTN                              AdditionalHeaderSize,
-  OUT UINTN                                    *PayloadSize
+  OUT UINTN                                    *PayloadSize OPTIONAL
   )
 {
   //
@@ -640,7 +658,10 @@ GetFmpHeader (
     return NULL;
   }
 
-  *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
+  if (PayloadSize != NULL) {
+    *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
+  }
+
   return (VOID *)((UINT8 *)Image + sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength  + AdditionalHeaderSize);
 }
 
@@ -663,6 +684,10 @@ GetAllHeaderSize (
 {
   UINT32  CalculatedSize;
 
+  if (Image == NULL) {
+    return 0;
+  }
+
   CalculatedSize = sizeof (Image->MonotonicCount) +
                    AdditionalHeaderSize +
                    Image->AuthInfo.Hdr.dwLength;
@@ -698,7 +723,7 @@ GetAllHeaderSize (
 
   @retval EFI_SUCCESS            The image was successfully checked.
   @retval EFI_ABORTED            The operation is aborted.
-  @retval EFI_INVALID_PARAMETER  The Image was NULL.
+  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
   @retval EFI_UNSUPPORTED        The operation is not supported.
   @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
 
@@ -743,6 +768,12 @@ CheckTheImage (
     return EFI_UNSUPPORTED;
   }
 
+  if (This == NULL) {
+    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
+    Status = EFI_INVALID_PARAMETER;
+    goto cleanup;
+  }
+
   //
   // Retrieve the private context structure
   //
@@ -978,7 +1009,7 @@ CheckTheImage (
 
   @retval EFI_SUCCESS            The device was successfully updated with the new image.
   @retval EFI_ABORTED            The operation is aborted.
-  @retval EFI_INVALID_PARAMETER  The Image was NULL.
+  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
   @retval EFI_UNSUPPORTED        The operation is not supported.
   @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
 
@@ -1026,6 +1057,12 @@ SetTheImage (
     return EFI_UNSUPPORTED;
   }
 
+  if (This == NULL) {
+    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
+    Status = EFI_INVALID_PARAMETER;
+    goto cleanup;
+  }
+
   //
   // Retrieve the private context structure
   //
@@ -1382,6 +1419,11 @@ FmpDxeLockEventNotify (
   EFI_STATUS                        Status;
   FIRMWARE_MANAGEMENT_PRIVATE_DATA  *Private;
 
+  if (Context == NULL) {
+    ASSERT (Context != NULL);
+    return;
+  }
+
   Private = (FIRMWARE_MANAGEMENT_PRIVATE_DATA *)Context;
 
   if (!Private->FmpDeviceLocked) {
diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
index 30754dea495e..4dfec316a558 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.h
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
@@ -3,7 +3,7 @@
   image stored in a firmware device with platform and firmware device specific
   information provided through PCDs and libraries.
 
-  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
+  Copyright (c) Microsoft Corporation.<BR>
   Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -132,7 +132,7 @@ DetectTestKey (
   @retval EFI_SUCCESS                The device was successfully updated with the new image.
   @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
                                      needed to hold the image(s) information is returned in ImageInfoSize.
-  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
+  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
   @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
 
 **/
@@ -166,7 +166,7 @@ GetTheImageInfo (
   @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
                                  image. The current buffer size needed to hold the image is returned
                                  in ImageSize.
-  @retval EFI_INVALID_PARAMETER  The Image was NULL.
+  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
   @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
   @retval EFI_UNSUPPORTED        The operation is not supported.
   @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
@@ -198,7 +198,7 @@ GetTheImage (
 
   @retval EFI_SUCCESS            The image was successfully checked.
   @retval EFI_ABORTED            The operation is aborted.
-  @retval EFI_INVALID_PARAMETER  The Image was NULL.
+  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
   @retval EFI_UNSUPPORTED        The operation is not supported.
   @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
 
@@ -254,7 +254,7 @@ CheckTheImage (
 
   @retval EFI_SUCCESS            The device was successfully updated with the new image.
   @retval EFI_ABORTED            The operation is aborted.
-  @retval EFI_INVALID_PARAMETER  The Image was NULL.
+  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
   @retval EFI_UNSUPPORTED        The operation is not supported.
   @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
 
-- 
2.27.0.windows.1

Re: [PATCH v1 2/7] FmpDevicePkg/FmpDependencyLib: Fix "exression" typo

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: michael.kubacki@outlook.com
> <michael.kubacki@outlook.com>
> Sent: Thursday, July 30, 2020 8:15 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 2/7] FmpDevicePkg/FmpDependencyLib:
> Fix "exression" typo
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki
> <michael.kubacki@microsoft.com>
> ---
> 
> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
> | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git
> a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> index 28358069950a..ba89eb22d9f0 100644
> ---
> a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> +++
> b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> @@ -421,7 +421,7 @@ EvaluateDependency (
>      Iterator++;
>    }
> 
> -  DEBUG ((DEBUG_ERROR, "EvaluateDependency: No
> EFI_FMP_DEP_END Opcode in exression!\n"));
> +  DEBUG ((DEBUG_ERROR, "EvaluateDependency: No
> EFI_FMP_DEP_END Opcode in expression!\n"));
> 
>  Error:
>    return FALSE;
> --
> 2.27.0.windows.1

Re: [PATCH v1 3/7] FmpDevicePkg/FmpDependencyLib: Handle version string overflow

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: michael.kubacki@outlook.com
> <michael.kubacki@outlook.com>
> Sent: Thursday, July 30, 2020 8:15 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 3/7] FmpDevicePkg/FmpDependencyLib:
> Handle version string overflow
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> This change recognizes the condition of the DEPEX
> version string
> extending beyond the end of the dependency expression as
> an error.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki
> <michael.kubacki@microsoft.com>
> ---
> 
> Notes:
>     This is particularly helpful for the user to isolate
> the issue
>     when stepping through the control flow as this case
> will be the
>     last executed before jumping to the Error label to
> return from
>     the function.
> 
> 
> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
> | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git
> a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> index ba89eb22d9f0..5ef25d2415cf 100644
> ---
> a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> +++
> b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> @@ -286,6 +286,7 @@ EvaluateDependency (
>        Iterator += AsciiStrnLenS ((CHAR8 *) Iterator,
> DependenciesSize - (Iterator - Dependencies-
> >Dependencies));
>        if (Iterator == (UINT8 *) Dependencies-
> >Dependencies + DependenciesSize) {
>          DEBUG ((DEBUG_ERROR, "EvaluateDependency:
> STRING extends beyond end of dependency
> expression!\n"));
> +        goto Error;
>        }
>        break;
>      case EFI_FMP_DEP_AND:
> --
> 2.27.0.windows.1

Re: [PATCH v1 4/7] FmpDevicePkg/FmpDependencyCheckLib: Return unsatisfied on handle failure

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: michael.kubacki@outlook.com
> <michael.kubacki@outlook.com>
> Sent: Thursday, July 30, 2020 8:15 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 4/7]
> FmpDevicePkg/FmpDependencyCheckLib: Return unsatisfied
> on handle failure
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> CheckFmpDependency () will currently return that
> dependencies are
> satisfied if the initial call in the function to locate
> handles
> that have gEfiFirmwareManagementProtocolGuid installed
> fails.
> 
> This change updates the error handling to return FALSE
> (dependencies
> are not satisfied) if this handle search fails.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki
> <michael.kubacki@microsoft.com>
> ---
> 
> FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependency
> CheckLib.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git
> a/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependen
> cyCheckLib.c
> b/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependen
> cyCheckLib.c
> index 5e0241b25957..02ed600e0e95 100644
> ---
> a/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependen
> cyCheckLib.c
> +++
> b/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependen
> cyCheckLib.c
> @@ -2,6 +2,7 @@
>    Provides FMP capsule dependency check services when
> updating the firmware
>    image of a FMP device.
> 
> +  Copyright (c) Microsoft Corporation.<BR>
>    Copyright (c) 2020, Intel Corporation. All rights
> reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -75,6 +76,7 @@ CheckFmpDependency (
>                  );
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "CheckFmpDependency: Get
> Firmware Management Protocol failed. (%r)", Status));
> +    IsSatisfied = FALSE;
>      goto cleanup;
>    }
> 
> --
> 2.27.0.windows.1

Re: [PATCH v1 5/7] FmpDevicePkg/FmpDxe: Better warn of potential ImageTypeId misconfig

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: michael.kubacki@outlook.com
> <michael.kubacki@outlook.com>
> Sent: Thursday, July 30, 2020 8:15 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 5/7] FmpDevicePkg/FmpDxe: Better warn
> of potential ImageTypeId misconfig
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> A user may fall through to the case they depend on the
> PcdFmpDeviceImageTypeIdGuid value to get the ImageTypeId
> GUID
> value. The default PCD value is 0 (NULL) so the code
> would
> further fall back on the gEfiCallerIdGuid value.
> 
> This change modifies the print error level for the
> message that
> indicates this occurred to DEBUG_WARN from DEBUG_INFO to
> better
> warn the user that this occurred.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki
> <michael.kubacki@microsoft.com>
> ---
> 
> Notes:
>     The PCD documentation indicates it is valid behavior
> for the user
>     to fall back to gEfiCallerIdGuid. Is that really
> expected?
> 
>     Would an ASSERT be appropriate?
> 
>  FmpDevicePkg/FmpDxe/FmpDxe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c
> b/FmpDevicePkg/FmpDxe/FmpDxe.c
> index 58841774fee0..14994ce4ee0e 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> @@ -3,7 +3,7 @@
>    image stored in a firmware device with platform and
> firmware device specific
>    information provided through PCDs and libraries.
> 
> -  Copyright (c) 2016, Microsoft Corporation. All rights
> reserved.<BR>
> +  Copyright (c) Microsoft Corporation.<BR>
>    Copyright (c) 2018 - 2020, Intel Corporation. All
> rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -179,7 +179,7 @@ GetImageTypeIdGuid (
>      if (ImageTypeIdGuidSize == sizeof (EFI_GUID)) {
>        FmpDeviceLibGuid = (EFI_GUID *)PcdGetPtr
> (PcdFmpDeviceImageTypeIdGuid);
>      } else {
> -      DEBUG ((DEBUG_INFO, "FmpDxe(%s): Fall back to
> ImageTypeIdGuid of gEfiCallerIdGuid\n", mImageIdName));
> +      DEBUG ((DEBUG_WARN, "FmpDxe(%s): Fall back to
> ImageTypeIdGuid of gEfiCallerIdGuid\n", mImageIdName));
>        FmpDeviceLibGuid = &gEfiCallerIdGuid;
>      }
>    }
> --
> 2.27.0.windows.1

Re: [edk2-devel] [PATCH v1 6/7] FmpDevicePkg/FmpDxe: Indicate ESRT GUID on invalid ImageIdName

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Michael Kubacki
> Sent: Thursday, July 30, 2020 8:15 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [edk2-devel] [PATCH v1 6/7]
> FmpDevicePkg/FmpDxe: Indicate ESRT GUID on invalid
> ImageIdName
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Updates the debug error message to include the GUID of
> the FMP
> instance that encountered the issue to help the user
> better
> isolate the problem.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki
> <michael.kubacki@microsoft.com>
> ---
>  FmpDevicePkg/FmpDxe/FmpDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c
> b/FmpDevicePkg/FmpDxe/FmpDxe.c
> index 14994ce4ee0e..a3e342591936 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> @@ -1679,7 +1679,7 @@ FmpDxeEntryPoint (
>      //
>      // PcdFmpDeviceImageIdName must be set to a non-
> empty Unicode string
>      //
> -    DEBUG ((DEBUG_ERROR, "FmpDxe:
> PcdFmpDeviceImageIdName is an empty string.\n"));
> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%g):
> PcdFmpDeviceImageIdName is an empty string.\n",
> &gEfiCallerIdGuid));
>      ASSERT (FALSE);
>      return EFI_UNSUPPORTED;
>    }
> --
> 2.27.0.windows.1
> 
> 
> 

Re: [PATCH v1 1/7] FmpDevicePkg/FmpDependencyLib: Correct ValidateDependency() documentation

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: michael.kubacki@outlook.com
> <michael.kubacki@outlook.com>
> Sent: Thursday, July 30, 2020 8:15 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 1/7] FmpDevicePkg/FmpDependencyLib:
> Correct ValidateDependency() documentation
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Modifies the return value documentation to state that
> the BOOLEAN
> value indicates whether a given dependency expression is
> valid
> not a capsule.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki
> <michael.kubacki@microsoft.com>
> ---
> 
> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
> | 5 +++--
>  FmpDevicePkg/Include/Library/FmpDependencyLib.h
> | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git
> a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> index 91dc0b9abd33..28358069950a 100644
> ---
> a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> +++
> b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib
> .c
> @@ -1,6 +1,7 @@
>  /** @file
>    Supports Fmp Capsule Dependency Expression.
> 
> +  Copyright (c) Microsoft Corporation.<BR>
>    Copyright (c) 2020, Intel Corporation. All rights
> reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -433,8 +434,8 @@ EvaluateDependency (
>    @param[in]   MaxDepexSize   Max size of the
> dependency.
>    @param[out]  DepexSize      Size of dependency.
> 
> -  @retval TRUE    The capsule is valid.
> -  @retval FALSE   The capsule is invalid.
> +  @retval TRUE    The dependency expression is valid.
> +  @retval FALSE   The dependency expression is invalid.
> 
>  **/
>  BOOLEAN
> diff --git
> a/FmpDevicePkg/Include/Library/FmpDependencyLib.h
> b/FmpDevicePkg/Include/Library/FmpDependencyLib.h
> index 1110eefa9a54..c732903425b4 100644
> --- a/FmpDevicePkg/Include/Library/FmpDependencyLib.h
> +++ b/FmpDevicePkg/Include/Library/FmpDependencyLib.h
> @@ -2,6 +2,7 @@
>    Fmp Capsule Dependency support functions for Firmware
> Management Protocol based
>    firmware updates.
> 
> +  Copyright (c) Microsoft Corporation.<BR>
>    Copyright (c) 2020, Intel Corporation. All rights
> reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -29,8 +30,8 @@ typedef struct {
>    @param[in]   MaxDepexSize   Max size of the
> dependency.
>    @param[out]  DepexSize      Size of dependency.
> 
> -  @retval TRUE    The capsule is valid.
> -  @retval FALSE   The capsule is invalid.
> +  @retval TRUE    The dependency expression is valid.
> +  @retval FALSE   The dependency expression is invalid.
> 
>  **/
>  BOOLEAN
> --
> 2.27.0.windows.1

Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation

[Michael D Kinney]

Hi Michael,

A few minor comments included below.  With those updates,

Reviewed-bv: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
> Sent: Thursday, July 30, 2020 8:15 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2869
> 
> Makes some minor improvements to function parameter validation
> in FmpDxe, in particular to externally exposed functions such
> as those that back EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  FmpDevicePkg/FmpDxe/FmpDxe.c | 56 +++++++++++++++++---
>  FmpDevicePkg/FmpDxe/FmpDxe.h | 10 ++--
>  2 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
> index a3e342591936..958d9b394b71 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> @@ -278,6 +278,11 @@ PopulateDescriptor (
>    EFI_STATUS  Status;
>    UINT32      DependenciesSize;
> 
> +  if (Private == NULL) {
> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): PopulateDescriptor() - Private is NULL.\n", mImageIdName));
> +    return;
> +  }
> +
>    if (Private->DescriptorPopulated) {
>      return;
>    }
> @@ -429,7 +434,7 @@ PopulateDescriptor (
>    @retval EFI_SUCCESS                The device was successfully updated with the new image.
>    @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>                                       needed to hold the image(s) information is returned in ImageInfoSize.
> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>    @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
> 
>  **/
> @@ -451,6 +456,12 @@ GetTheImageInfo (
> 
>    Status = EFI_SUCCESS;
> 
> +  if (This == NULL) {
> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - This is NULL.\n", mImageIdName));
> +    Status = EFI_INVALID_PARAMETER;
> +    goto cleanup;
> +  }
> +
>    //
>    // Retrieve the private context structure
>    //
> @@ -536,7 +547,7 @@ GetTheImageInfo (
>    @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>                                   image. The current buffer size needed to hold the image is returned
>                                   in ImageSize.
> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>    @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>    @retval EFI_UNSUPPORTED        The operation is not supported.
>    @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> @@ -561,6 +572,12 @@ GetTheImage (
> 
>    Status = EFI_SUCCESS;
> 
> +  if (This == NULL) {
> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImage() - This is NULL.\n", mImageIdName));
> +    Status = EFI_INVALID_PARAMETER;
> +    goto cleanup;
> +  }
> +
>    //
>    // Retrieve the private context structure
>    //
> @@ -615,7 +632,8 @@ GetTheImage (
>    @param[in]   Image                 Pointer to the image.
>    @param[in]   ImageSize             Size of the image.
>    @param[in]   AdditionalHeaderSize  Size of any headers that cannot be calculated by this function.
> -  @param[out]  PayloadSize
> +  @param[out]  PayloadSize           An optional pointer to a UINTN that holds the size of the payload
> +                                     (image size minus headers)
> 
>    @retval  !NULL  Valid pointer to the header.
>    @retval  NULL   Structure is bad and pointer cannot be found.
> @@ -626,7 +644,7 @@ GetFmpHeader (
>    IN  CONST EFI_FIRMWARE_IMAGE_AUTHENTICATION  *Image,
>    IN  CONST UINTN                              ImageSize,
>    IN  CONST UINTN                              AdditionalHeaderSize,
> -  OUT UINTN                                    *PayloadSize
> +  OUT UINTN                                    *PayloadSize OPTIONAL
>    )
>  {
>    //
> @@ -640,7 +658,10 @@ GetFmpHeader (
>      return NULL;
>    }
> 
> -  *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
> +  if (PayloadSize != NULL) {
> +    *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
> +  }
> +
>    return (VOID *)((UINT8 *)Image + sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength  + AdditionalHeaderSize);
>  }
> 
> @@ -663,6 +684,10 @@ GetAllHeaderSize (
>  {
>    UINT32  CalculatedSize;
> 
> +  if (Image == NULL) {

This is an internal helper function.  If Image is ever NULL, it must be a bug in the 
FmpDxe driver.  Should we do more than just return 0?  Perhaps a DEBUG_ERROR message too?

> +    return 0;
> +  }
> +
>    CalculatedSize = sizeof (Image->MonotonicCount) +
>                     AdditionalHeaderSize +
>                     Image->AuthInfo.Hdr.dwLength;
> @@ -698,7 +723,7 @@ GetAllHeaderSize (
> 
>    @retval EFI_SUCCESS            The image was successfully checked.
>    @retval EFI_ABORTED            The operation is aborted.
> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.

This function also uses ImageIndex.  Similar to updates above, I think this
@retval line should be:

  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.

>    @retval EFI_UNSUPPORTED        The operation is not supported.
>    @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> 
> @@ -743,6 +768,12 @@ CheckTheImage (
>      return EFI_UNSUPPORTED;
>    }
> 
> +  if (This == NULL) {
> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
> +    Status = EFI_INVALID_PARAMETER;
> +    goto cleanup;
> +  }
> +
>    //
>    // Retrieve the private context structure
>    //
> @@ -978,7 +1009,7 @@ CheckTheImage (
> 
>    @retval EFI_SUCCESS            The device was successfully updated with the new image.
>    @retval EFI_ABORTED            The operation is aborted.
> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.

This function also uses ImageIndex.  Similar to updates above, I think this
@retval line should be:

  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.

>    @retval EFI_UNSUPPORTED        The operation is not supported.
>    @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> 
> @@ -1026,6 +1057,12 @@ SetTheImage (
>      return EFI_UNSUPPORTED;
>    }
> 
> +  if (This == NULL) {
> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
> +    Status = EFI_INVALID_PARAMETER;
> +    goto cleanup;
> +  }
> +
>    //
>    // Retrieve the private context structure
>    //
> @@ -1382,6 +1419,11 @@ FmpDxeLockEventNotify (
>    EFI_STATUS                        Status;
>    FIRMWARE_MANAGEMENT_PRIVATE_DATA  *Private;
> 
> +  if (Context == NULL) {
> +    ASSERT (Context != NULL);
> +    return;
> +  }
> +
>    Private = (FIRMWARE_MANAGEMENT_PRIVATE_DATA *)Context;
> 
>    if (!Private->FmpDeviceLocked) {
> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
> index 30754dea495e..4dfec316a558 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.h
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
> @@ -3,7 +3,7 @@
>    image stored in a firmware device with platform and firmware device specific
>    information provided through PCDs and libraries.
> 
> -  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
> +  Copyright (c) Microsoft Corporation.<BR>
>    Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -132,7 +132,7 @@ DetectTestKey (
>    @retval EFI_SUCCESS                The device was successfully updated with the new image.
>    @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>                                       needed to hold the image(s) information is returned in ImageInfoSize.
> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>    @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
> 
>  **/
> @@ -166,7 +166,7 @@ GetTheImageInfo (
>    @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>                                   image. The current buffer size needed to hold the image is returned
>                                   in ImageSize.
> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>    @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>    @retval EFI_UNSUPPORTED        The operation is not supported.
>    @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> @@ -198,7 +198,7 @@ GetTheImage (
> 
>    @retval EFI_SUCCESS            The image was successfully checked.
>    @retval EFI_ABORTED            The operation is aborted.
> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.

This function also uses ImageIndex.  Similar to updates above, I think this
@retval line should be:

  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.

>    @retval EFI_UNSUPPORTED        The operation is not supported.
>    @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> 
> @@ -254,7 +254,7 @@ CheckTheImage (
> 
>    @retval EFI_SUCCESS            The device was successfully updated with the new image.
>    @retval EFI_ABORTED            The operation is aborted.
> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.

This function also uses ImageIndex.  Similar to updates above, I think this
@retval line should be:

  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.

>    @retval EFI_UNSUPPORTED        The operation is not supported.
>    @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> 
> --
> 2.27.0.windows.1

Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation

[Michael Kubacki]

Hi Mike,

Some of those functions currently return EFI_SUCCESS if ImageIndex is 
invalid. Example: 
https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L851

Given the request to update the EFI_INVALID_PARAMETER text for those 
other functions, I'm assuming you'd like me to make those return 
EFI_INVALID_PARAMETER like what GetTheImage() currently does - 
https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L573?

Thanks,
Michael

On 8/5/2020 9:51 AM, Kinney, Michael D wrote:
> Hi Michael,
> 
> A few minor comments included below.  With those updates,
> 
> Reviewed-bv: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Mike
> 
>> -----Original Message-----
>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>> Sent: Thursday, July 30, 2020 8:15 PM
>> To: devel@edk2.groups.io
>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2869
>>
>> Makes some minor improvements to function parameter validation
>> in FmpDxe, in particular to externally exposed functions such
>> as those that back EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>   FmpDevicePkg/FmpDxe/FmpDxe.c | 56 +++++++++++++++++---
>>   FmpDevicePkg/FmpDxe/FmpDxe.h | 10 ++--
>>   2 files changed, 54 insertions(+), 12 deletions(-)
>>
>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
>> index a3e342591936..958d9b394b71 100644
>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
>> @@ -278,6 +278,11 @@ PopulateDescriptor (
>>     EFI_STATUS  Status;
>>     UINT32      DependenciesSize;
>>
>> +  if (Private == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): PopulateDescriptor() - Private is NULL.\n", mImageIdName));
>> +    return;
>> +  }
>> +
>>     if (Private->DescriptorPopulated) {
>>       return;
>>     }
>> @@ -429,7 +434,7 @@ PopulateDescriptor (
>>     @retval EFI_SUCCESS                The device was successfully updated with the new image.
>>     @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>>                                        needed to hold the image(s) information is returned in ImageInfoSize.
>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>>     @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
>>
>>   **/
>> @@ -451,6 +456,12 @@ GetTheImageInfo (
>>
>>     Status = EFI_SUCCESS;
>>
>> +  if (This == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - This is NULL.\n", mImageIdName));
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto cleanup;
>> +  }
>> +
>>     //
>>     // Retrieve the private context structure
>>     //
>> @@ -536,7 +547,7 @@ GetTheImageInfo (
>>     @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>>                                    image. The current buffer size needed to hold the image is returned
>>                                    in ImageSize.
>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>     @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>>     @retval EFI_UNSUPPORTED        The operation is not supported.
>>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>> @@ -561,6 +572,12 @@ GetTheImage (
>>
>>     Status = EFI_SUCCESS;
>>
>> +  if (This == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImage() - This is NULL.\n", mImageIdName));
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto cleanup;
>> +  }
>> +
>>     //
>>     // Retrieve the private context structure
>>     //
>> @@ -615,7 +632,8 @@ GetTheImage (
>>     @param[in]   Image                 Pointer to the image.
>>     @param[in]   ImageSize             Size of the image.
>>     @param[in]   AdditionalHeaderSize  Size of any headers that cannot be calculated by this function.
>> -  @param[out]  PayloadSize
>> +  @param[out]  PayloadSize           An optional pointer to a UINTN that holds the size of the payload
>> +                                     (image size minus headers)
>>
>>     @retval  !NULL  Valid pointer to the header.
>>     @retval  NULL   Structure is bad and pointer cannot be found.
>> @@ -626,7 +644,7 @@ GetFmpHeader (
>>     IN  CONST EFI_FIRMWARE_IMAGE_AUTHENTICATION  *Image,
>>     IN  CONST UINTN                              ImageSize,
>>     IN  CONST UINTN                              AdditionalHeaderSize,
>> -  OUT UINTN                                    *PayloadSize
>> +  OUT UINTN                                    *PayloadSize OPTIONAL
>>     )
>>   {
>>     //
>> @@ -640,7 +658,10 @@ GetFmpHeader (
>>       return NULL;
>>     }
>>
>> -  *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
>> +  if (PayloadSize != NULL) {
>> +    *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
>> +  }
>> +
>>     return (VOID *)((UINT8 *)Image + sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength  + AdditionalHeaderSize);
>>   }
>>
>> @@ -663,6 +684,10 @@ GetAllHeaderSize (
>>   {
>>     UINT32  CalculatedSize;
>>
>> +  if (Image == NULL) {
> 
> This is an internal helper function.  If Image is ever NULL, it must be a bug in the
> FmpDxe driver.  Should we do more than just return 0?  Perhaps a DEBUG_ERROR message too?
> 
>> +    return 0;
>> +  }
>> +
>>     CalculatedSize = sizeof (Image->MonotonicCount) +
>>                      AdditionalHeaderSize +
>>                      Image->AuthInfo.Hdr.dwLength;
>> @@ -698,7 +723,7 @@ GetAllHeaderSize (
>>
>>     @retval EFI_SUCCESS            The image was successfully checked.
>>     @retval EFI_ABORTED            The operation is aborted.
>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> 
> This function also uses ImageIndex.  Similar to updates above, I think this
> @retval line should be:
> 
>    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> 
>>     @retval EFI_UNSUPPORTED        The operation is not supported.
>>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>
>> @@ -743,6 +768,12 @@ CheckTheImage (
>>       return EFI_UNSUPPORTED;
>>     }
>>
>> +  if (This == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto cleanup;
>> +  }
>> +
>>     //
>>     // Retrieve the private context structure
>>     //
>> @@ -978,7 +1009,7 @@ CheckTheImage (
>>
>>     @retval EFI_SUCCESS            The device was successfully updated with the new image.
>>     @retval EFI_ABORTED            The operation is aborted.
>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> 
> This function also uses ImageIndex.  Similar to updates above, I think this
> @retval line should be:
> 
>    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> 
>>     @retval EFI_UNSUPPORTED        The operation is not supported.
>>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>
>> @@ -1026,6 +1057,12 @@ SetTheImage (
>>       return EFI_UNSUPPORTED;
>>     }
>>
>> +  if (This == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto cleanup;
>> +  }
>> +
>>     //
>>     // Retrieve the private context structure
>>     //
>> @@ -1382,6 +1419,11 @@ FmpDxeLockEventNotify (
>>     EFI_STATUS                        Status;
>>     FIRMWARE_MANAGEMENT_PRIVATE_DATA  *Private;
>>
>> +  if (Context == NULL) {
>> +    ASSERT (Context != NULL);
>> +    return;
>> +  }
>> +
>>     Private = (FIRMWARE_MANAGEMENT_PRIVATE_DATA *)Context;
>>
>>     if (!Private->FmpDeviceLocked) {
>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
>> index 30754dea495e..4dfec316a558 100644
>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.h
>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
>> @@ -3,7 +3,7 @@
>>     image stored in a firmware device with platform and firmware device specific
>>     information provided through PCDs and libraries.
>>
>> -  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
>> +  Copyright (c) Microsoft Corporation.<BR>
>>     Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
>>
>>     SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -132,7 +132,7 @@ DetectTestKey (
>>     @retval EFI_SUCCESS                The device was successfully updated with the new image.
>>     @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>>                                        needed to hold the image(s) information is returned in ImageInfoSize.
>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>>     @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
>>
>>   **/
>> @@ -166,7 +166,7 @@ GetTheImageInfo (
>>     @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>>                                    image. The current buffer size needed to hold the image is returned
>>                                    in ImageSize.
>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>     @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>>     @retval EFI_UNSUPPORTED        The operation is not supported.
>>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>> @@ -198,7 +198,7 @@ GetTheImage (
>>
>>     @retval EFI_SUCCESS            The image was successfully checked.
>>     @retval EFI_ABORTED            The operation is aborted.
>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> 
> This function also uses ImageIndex.  Similar to updates above, I think this
> @retval line should be:
> 
>    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> 
>>     @retval EFI_UNSUPPORTED        The operation is not supported.
>>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>
>> @@ -254,7 +254,7 @@ CheckTheImage (
>>
>>     @retval EFI_SUCCESS            The device was successfully updated with the new image.
>>     @retval EFI_ABORTED            The operation is aborted.
>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> 
> This function also uses ImageIndex.  Similar to updates above, I think this
> @retval line should be:
> 
>    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> 
>>     @retval EFI_UNSUPPORTED        The operation is not supported.
>>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>
>> --
>> 2.27.0.windows.1
> 

Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation

[Michael D Kinney]

Michael,

That is a good point.  I missed that behavior in some of the APIs.

What I also missed was that these APIs are defined in the UEFI Spec and the
description of the return codes is from there and should match the UEFI Spec.

The implementation should be conformant with the UEFI Spec.  If you notice
behavior that is not conformant, then we need to update the code or potentially
work on spec updates.

For this patch series, let’s make sure the Firmware Management Protocol service
headers match the UEFI Spec.

Mike

> -----Original Message-----
> From: Michael Kubacki <michael.kubacki@outlook.com>
> Sent: Wednesday, August 5, 2020 1:43 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
> 
> Hi Mike,
> 
> Some of those functions currently return EFI_SUCCESS if ImageIndex is
> invalid. Example:
> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L851
> 
> Given the request to update the EFI_INVALID_PARAMETER text for those
> other functions, I'm assuming you'd like me to make those return
> EFI_INVALID_PARAMETER like what GetTheImage() currently does -
> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L573?
> 
> Thanks,
> Michael
> 
> On 8/5/2020 9:51 AM, Kinney, Michael D wrote:
> > Hi Michael,
> >
> > A few minor comments included below.  With those updates,
> >
> > Reviewed-bv: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
> >> Sent: Thursday, July 30, 2020 8:15 PM
> >> To: devel@edk2.groups.io
> >> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> >> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>
> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2869
> >>
> >> Makes some minor improvements to function parameter validation
> >> in FmpDxe, in particular to externally exposed functions such
> >> as those that back EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
> >>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >> ---
> >>   FmpDevicePkg/FmpDxe/FmpDxe.c | 56 +++++++++++++++++---
> >>   FmpDevicePkg/FmpDxe/FmpDxe.h | 10 ++--
> >>   2 files changed, 54 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
> >> index a3e342591936..958d9b394b71 100644
> >> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> >> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> >> @@ -278,6 +278,11 @@ PopulateDescriptor (
> >>     EFI_STATUS  Status;
> >>     UINT32      DependenciesSize;
> >>
> >> +  if (Private == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): PopulateDescriptor() - Private is NULL.\n", mImageIdName));
> >> +    return;
> >> +  }
> >> +
> >>     if (Private->DescriptorPopulated) {
> >>       return;
> >>     }
> >> @@ -429,7 +434,7 @@ PopulateDescriptor (
> >>     @retval EFI_SUCCESS                The device was successfully updated with the new image.
> >>     @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
> >>                                        needed to hold the image(s) information is returned in ImageInfoSize.
> >> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> >> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
> >>     @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
> >>
> >>   **/
> >> @@ -451,6 +456,12 @@ GetTheImageInfo (
> >>
> >>     Status = EFI_SUCCESS;
> >>
> >> +  if (This == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - This is NULL.\n", mImageIdName));
> >> +    Status = EFI_INVALID_PARAMETER;
> >> +    goto cleanup;
> >> +  }
> >> +
> >>     //
> >>     // Retrieve the private context structure
> >>     //
> >> @@ -536,7 +547,7 @@ GetTheImageInfo (
> >>     @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
> >>                                    image. The current buffer size needed to hold the image is returned
> >>                                    in ImageSize.
> >> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>     @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >> @@ -561,6 +572,12 @@ GetTheImage (
> >>
> >>     Status = EFI_SUCCESS;
> >>
> >> +  if (This == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImage() - This is NULL.\n", mImageIdName));
> >> +    Status = EFI_INVALID_PARAMETER;
> >> +    goto cleanup;
> >> +  }
> >> +
> >>     //
> >>     // Retrieve the private context structure
> >>     //
> >> @@ -615,7 +632,8 @@ GetTheImage (
> >>     @param[in]   Image                 Pointer to the image.
> >>     @param[in]   ImageSize             Size of the image.
> >>     @param[in]   AdditionalHeaderSize  Size of any headers that cannot be calculated by this function.
> >> -  @param[out]  PayloadSize
> >> +  @param[out]  PayloadSize           An optional pointer to a UINTN that holds the size of the payload
> >> +                                     (image size minus headers)
> >>
> >>     @retval  !NULL  Valid pointer to the header.
> >>     @retval  NULL   Structure is bad and pointer cannot be found.
> >> @@ -626,7 +644,7 @@ GetFmpHeader (
> >>     IN  CONST EFI_FIRMWARE_IMAGE_AUTHENTICATION  *Image,
> >>     IN  CONST UINTN                              ImageSize,
> >>     IN  CONST UINTN                              AdditionalHeaderSize,
> >> -  OUT UINTN                                    *PayloadSize
> >> +  OUT UINTN                                    *PayloadSize OPTIONAL
> >>     )
> >>   {
> >>     //
> >> @@ -640,7 +658,10 @@ GetFmpHeader (
> >>       return NULL;
> >>     }
> >>
> >> -  *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
> >> +  if (PayloadSize != NULL) {
> >> +    *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
> >> +  }
> >> +
> >>     return (VOID *)((UINT8 *)Image + sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength  + AdditionalHeaderSize);
> >>   }
> >>
> >> @@ -663,6 +684,10 @@ GetAllHeaderSize (
> >>   {
> >>     UINT32  CalculatedSize;
> >>
> >> +  if (Image == NULL) {
> >
> > This is an internal helper function.  If Image is ever NULL, it must be a bug in the
> > FmpDxe driver.  Should we do more than just return 0?  Perhaps a DEBUG_ERROR message too?
> >
> >> +    return 0;
> >> +  }
> >> +
> >>     CalculatedSize = sizeof (Image->MonotonicCount) +
> >>                      AdditionalHeaderSize +
> >>                      Image->AuthInfo.Hdr.dwLength;
> >> @@ -698,7 +723,7 @@ GetAllHeaderSize (
> >>
> >>     @retval EFI_SUCCESS            The image was successfully checked.
> >>     @retval EFI_ABORTED            The operation is aborted.
> >> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >
> > This function also uses ImageIndex.  Similar to updates above, I think this
> > @retval line should be:
> >
> >    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>
> >> @@ -743,6 +768,12 @@ CheckTheImage (
> >>       return EFI_UNSUPPORTED;
> >>     }
> >>
> >> +  if (This == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
> >> +    Status = EFI_INVALID_PARAMETER;
> >> +    goto cleanup;
> >> +  }
> >> +
> >>     //
> >>     // Retrieve the private context structure
> >>     //
> >> @@ -978,7 +1009,7 @@ CheckTheImage (
> >>
> >>     @retval EFI_SUCCESS            The device was successfully updated with the new image.
> >>     @retval EFI_ABORTED            The operation is aborted.
> >> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >
> > This function also uses ImageIndex.  Similar to updates above, I think this
> > @retval line should be:
> >
> >    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>
> >> @@ -1026,6 +1057,12 @@ SetTheImage (
> >>       return EFI_UNSUPPORTED;
> >>     }
> >>
> >> +  if (This == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
> >> +    Status = EFI_INVALID_PARAMETER;
> >> +    goto cleanup;
> >> +  }
> >> +
> >>     //
> >>     // Retrieve the private context structure
> >>     //
> >> @@ -1382,6 +1419,11 @@ FmpDxeLockEventNotify (
> >>     EFI_STATUS                        Status;
> >>     FIRMWARE_MANAGEMENT_PRIVATE_DATA  *Private;
> >>
> >> +  if (Context == NULL) {
> >> +    ASSERT (Context != NULL);
> >> +    return;
> >> +  }
> >> +
> >>     Private = (FIRMWARE_MANAGEMENT_PRIVATE_DATA *)Context;
> >>
> >>     if (!Private->FmpDeviceLocked) {
> >> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
> >> index 30754dea495e..4dfec316a558 100644
> >> --- a/FmpDevicePkg/FmpDxe/FmpDxe.h
> >> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
> >> @@ -3,7 +3,7 @@
> >>     image stored in a firmware device with platform and firmware device specific
> >>     information provided through PCDs and libraries.
> >>
> >> -  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
> >> +  Copyright (c) Microsoft Corporation.<BR>
> >>     Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
> >>
> >>     SPDX-License-Identifier: BSD-2-Clause-Patent
> >> @@ -132,7 +132,7 @@ DetectTestKey (
> >>     @retval EFI_SUCCESS                The device was successfully updated with the new image.
> >>     @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
> >>                                        needed to hold the image(s) information is returned in ImageInfoSize.
> >> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> >> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
> >>     @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
> >>
> >>   **/
> >> @@ -166,7 +166,7 @@ GetTheImageInfo (
> >>     @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
> >>                                    image. The current buffer size needed to hold the image is returned
> >>                                    in ImageSize.
> >> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>     @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >> @@ -198,7 +198,7 @@ GetTheImage (
> >>
> >>     @retval EFI_SUCCESS            The image was successfully checked.
> >>     @retval EFI_ABORTED            The operation is aborted.
> >> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >
> > This function also uses ImageIndex.  Similar to updates above, I think this
> > @retval line should be:
> >
> >    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>
> >> @@ -254,7 +254,7 @@ CheckTheImage (
> >>
> >>     @retval EFI_SUCCESS            The device was successfully updated with the new image.
> >>     @retval EFI_ABORTED            The operation is aborted.
> >> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >
> > This function also uses ImageIndex.  Similar to updates above, I think this
> > @retval line should be:
> >
> >    @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>     @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>
> >> --
> >> 2.27.0.windows.1
> >

Re: [edk2-devel] [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation

[Michael Kubacki]

Hi Mike,

There's quite a few discrepancies at the moment between functions in 
FmpDxe that implement EFI_FIRMWARE_MANAGEMENT_PROTOCOL and the 
corresponding description in the UEFI spec.

For example:

UEFI Spec 2.8B - EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo():

Status Codes Returned

EFI_SUCCESS             The image information was successfully returned.
EFI_BUFFER_TOO_SMALL    The ImageInfo buffer was too small. The current
                         buffer size needed to hold the image(s)
                         information is returned in ImageInfoSize.
EFI_INVALID_PARAMETER   ImageInfoSize is not too small and ImageInfo is
                         NULL.
EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorVersion
                         is NULL.
EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorCount is
                         NULL.
EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorSize is
                         NULL.
EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and PackageVersion is
                         NULL.
EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and PackageVersionName
                         is NULL.
EFI_DEVICE_ERROR        Valid information could not be returned.
                         Possible corrupted image.

Actual - FmpDxe - GetTheImageInfo():

   @retval EFI_SUCCESS                The device was successfully updated
                                      with the new image.
   @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small.
                                      The current buffer size
                                      needed to hold the image(s)
                                      information is returned in
                                      ImageInfoSize.
   @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
   @retval EFI_DEVICE_ERROR           Valid information could not be
                                      returned. Possible corrupted image.

There's cases such as in GetTheImage() where the code describes 
EFI_INVALID_PARAMETER is returned as follows:

   @retval EFI_INVALID_PARAMETER  The Image was NULL.

However, the implementation will actually return the status code under 
other conditions such as an invalid ImageIndex or NULL ImageSize pointer.

I agree these should be cleaned up such that implementation and spec 
match and the descriptions are accurate, but that could warrant its own 
series.

For this series, is the ask to leave the descriptions as-is? If so, I 
can file a BZ to resolve the code/spec discrepancies.

Thanks,
Michael

On 8/5/2020 4:30 PM, Michael D Kinney wrote:
> Michael,
> 
> That is a good point.  I missed that behavior in some of the APIs.
> 
> What I also missed was that these APIs are defined in the UEFI Spec and the
> description of the return codes is from there and should match the UEFI Spec.
> 
> The implementation should be conformant with the UEFI Spec.  If you notice
> behavior that is not conformant, then we need to update the code or potentially
> work on spec updates.
> 
> For this patch series, let’s make sure the Firmware Management Protocol service
> headers match the UEFI Spec.
> 
> Mike
> 
>> -----Original Message-----
>> From: Michael Kubacki <michael.kubacki@outlook.com>
>> Sent: Wednesday, August 5, 2020 1:43 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
>> Cc: Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
>>
>> Hi Mike,
>>
>> Some of those functions currently return EFI_SUCCESS if ImageIndex is
>> invalid. Example:
>> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L851
>>
>> Given the request to update the EFI_INVALID_PARAMETER text for those
>> other functions, I'm assuming you'd like me to make those return
>> EFI_INVALID_PARAMETER like what GetTheImage() currently does -
>> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L573?
>>
>> Thanks,
>> Michael
>>
>> On 8/5/2020 9:51 AM, Kinney, Michael D wrote:
>>> Hi Michael,
>>>
>>> A few minor comments included below.  With those updates,
>>>
>>> Reviewed-bv: Michael D Kinney <michael.d.kinney@intel.com>
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>>>> Sent: Thursday, July 30, 2020 8:15 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>>>> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
>>>>
>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>
>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2869
>>>>
>>>> Makes some minor improvements to function parameter validation
>>>> in FmpDxe, in particular to externally exposed functions such
>>>> as those that back EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
>>>>
>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>> ---
>>>>    FmpDevicePkg/FmpDxe/FmpDxe.c | 56 +++++++++++++++++---
>>>>    FmpDevicePkg/FmpDxe/FmpDxe.h | 10 ++--
>>>>    2 files changed, 54 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
>>>> index a3e342591936..958d9b394b71 100644
>>>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
>>>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
>>>> @@ -278,6 +278,11 @@ PopulateDescriptor (
>>>>      EFI_STATUS  Status;
>>>>      UINT32      DependenciesSize;
>>>>
>>>> +  if (Private == NULL) {
>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): PopulateDescriptor() - Private is NULL.\n", mImageIdName));
>>>> +    return;
>>>> +  }
>>>> +
>>>>      if (Private->DescriptorPopulated) {
>>>>        return;
>>>>      }
>>>> @@ -429,7 +434,7 @@ PopulateDescriptor (
>>>>      @retval EFI_SUCCESS                The device was successfully updated with the new image.
>>>>      @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>>>>                                         needed to hold the image(s) information is returned in ImageInfoSize.
>>>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>>>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>>>>      @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
>>>>
>>>>    **/
>>>> @@ -451,6 +456,12 @@ GetTheImageInfo (
>>>>
>>>>      Status = EFI_SUCCESS;
>>>>
>>>> +  if (This == NULL) {
>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - This is NULL.\n", mImageIdName));
>>>> +    Status = EFI_INVALID_PARAMETER;
>>>> +    goto cleanup;
>>>> +  }
>>>> +
>>>>      //
>>>>      // Retrieve the private context structure
>>>>      //
>>>> @@ -536,7 +547,7 @@ GetTheImageInfo (
>>>>      @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>>>>                                     image. The current buffer size needed to hold the image is returned
>>>>                                     in ImageSize.
>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>      @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>> @@ -561,6 +572,12 @@ GetTheImage (
>>>>
>>>>      Status = EFI_SUCCESS;
>>>>
>>>> +  if (This == NULL) {
>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImage() - This is NULL.\n", mImageIdName));
>>>> +    Status = EFI_INVALID_PARAMETER;
>>>> +    goto cleanup;
>>>> +  }
>>>> +
>>>>      //
>>>>      // Retrieve the private context structure
>>>>      //
>>>> @@ -615,7 +632,8 @@ GetTheImage (
>>>>      @param[in]   Image                 Pointer to the image.
>>>>      @param[in]   ImageSize             Size of the image.
>>>>      @param[in]   AdditionalHeaderSize  Size of any headers that cannot be calculated by this function.
>>>> -  @param[out]  PayloadSize
>>>> +  @param[out]  PayloadSize           An optional pointer to a UINTN that holds the size of the payload
>>>> +                                     (image size minus headers)
>>>>
>>>>      @retval  !NULL  Valid pointer to the header.
>>>>      @retval  NULL   Structure is bad and pointer cannot be found.
>>>> @@ -626,7 +644,7 @@ GetFmpHeader (
>>>>      IN  CONST EFI_FIRMWARE_IMAGE_AUTHENTICATION  *Image,
>>>>      IN  CONST UINTN                              ImageSize,
>>>>      IN  CONST UINTN                              AdditionalHeaderSize,
>>>> -  OUT UINTN                                    *PayloadSize
>>>> +  OUT UINTN                                    *PayloadSize OPTIONAL
>>>>      )
>>>>    {
>>>>      //
>>>> @@ -640,7 +658,10 @@ GetFmpHeader (
>>>>        return NULL;
>>>>      }
>>>>
>>>> -  *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
>>>> +  if (PayloadSize != NULL) {
>>>> +    *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
>>>> +  }
>>>> +
>>>>      return (VOID *)((UINT8 *)Image + sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength  + AdditionalHeaderSize);
>>>>    }
>>>>
>>>> @@ -663,6 +684,10 @@ GetAllHeaderSize (
>>>>    {
>>>>      UINT32  CalculatedSize;
>>>>
>>>> +  if (Image == NULL) {
>>>
>>> This is an internal helper function.  If Image is ever NULL, it must be a bug in the
>>> FmpDxe driver.  Should we do more than just return 0?  Perhaps a DEBUG_ERROR message too?
>>>
>>>> +    return 0;
>>>> +  }
>>>> +
>>>>      CalculatedSize = sizeof (Image->MonotonicCount) +
>>>>                       AdditionalHeaderSize +
>>>>                       Image->AuthInfo.Hdr.dwLength;
>>>> @@ -698,7 +723,7 @@ GetAllHeaderSize (
>>>>
>>>>      @retval EFI_SUCCESS            The image was successfully checked.
>>>>      @retval EFI_ABORTED            The operation is aborted.
>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>
>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>> @retval line should be:
>>>
>>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>
>>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>
>>>> @@ -743,6 +768,12 @@ CheckTheImage (
>>>>        return EFI_UNSUPPORTED;
>>>>      }
>>>>
>>>> +  if (This == NULL) {
>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
>>>> +    Status = EFI_INVALID_PARAMETER;
>>>> +    goto cleanup;
>>>> +  }
>>>> +
>>>>      //
>>>>      // Retrieve the private context structure
>>>>      //
>>>> @@ -978,7 +1009,7 @@ CheckTheImage (
>>>>
>>>>      @retval EFI_SUCCESS            The device was successfully updated with the new image.
>>>>      @retval EFI_ABORTED            The operation is aborted.
>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>
>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>> @retval line should be:
>>>
>>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>
>>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>
>>>> @@ -1026,6 +1057,12 @@ SetTheImage (
>>>>        return EFI_UNSUPPORTED;
>>>>      }
>>>>
>>>> +  if (This == NULL) {
>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
>>>> +    Status = EFI_INVALID_PARAMETER;
>>>> +    goto cleanup;
>>>> +  }
>>>> +
>>>>      //
>>>>      // Retrieve the private context structure
>>>>      //
>>>> @@ -1382,6 +1419,11 @@ FmpDxeLockEventNotify (
>>>>      EFI_STATUS                        Status;
>>>>      FIRMWARE_MANAGEMENT_PRIVATE_DATA  *Private;
>>>>
>>>> +  if (Context == NULL) {
>>>> +    ASSERT (Context != NULL);
>>>> +    return;
>>>> +  }
>>>> +
>>>>      Private = (FIRMWARE_MANAGEMENT_PRIVATE_DATA *)Context;
>>>>
>>>>      if (!Private->FmpDeviceLocked) {
>>>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
>>>> index 30754dea495e..4dfec316a558 100644
>>>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.h
>>>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
>>>> @@ -3,7 +3,7 @@
>>>>      image stored in a firmware device with platform and firmware device specific
>>>>      information provided through PCDs and libraries.
>>>>
>>>> -  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
>>>> +  Copyright (c) Microsoft Corporation.<BR>
>>>>      Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
>>>>
>>>>      SPDX-License-Identifier: BSD-2-Clause-Patent
>>>> @@ -132,7 +132,7 @@ DetectTestKey (
>>>>      @retval EFI_SUCCESS                The device was successfully updated with the new image.
>>>>      @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>>>>                                         needed to hold the image(s) information is returned in ImageInfoSize.
>>>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>>>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>>>>      @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
>>>>
>>>>    **/
>>>> @@ -166,7 +166,7 @@ GetTheImageInfo (
>>>>      @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>>>>                                     image. The current buffer size needed to hold the image is returned
>>>>                                     in ImageSize.
>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>      @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>> @@ -198,7 +198,7 @@ GetTheImage (
>>>>
>>>>      @retval EFI_SUCCESS            The image was successfully checked.
>>>>      @retval EFI_ABORTED            The operation is aborted.
>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>
>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>> @retval line should be:
>>>
>>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>
>>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>
>>>> @@ -254,7 +254,7 @@ CheckTheImage (
>>>>
>>>>      @retval EFI_SUCCESS            The device was successfully updated with the new image.
>>>>      @retval EFI_ABORTED            The operation is aborted.
>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>
>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>> @retval line should be:
>>>
>>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>
>>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>
>>>> --
>>>> 2.27.0.windows.1
>>>
> 
> 
> 

Re: [edk2-devel] [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation

[Michael D Kinney]

Michael,

The description matches UEFI 2.7.  So there appears to be a work item to update the FMP
function descriptions to the latest UEFI 2.8 spec.  

I do recommend you do not change any of these comments in the current patch series.
The update to UEFI 2.8 can be a new BZ.

The UEFI Specifications allows an implementation to return additional error return codes
that are not listed in the API definition.


  Status Codes Returned: A description of any codes returned by the interface. The
    procedure is required to implement any status codes
    listed in this table. Additional error codes may be
    returned, but they will not be tested by standard
    compliance tests, and any software that uses the
    procedure cannot depend on any of the extended error
    codes that an implementation may provide.

Best regards.

Mike

> -----Original Message-----
> From: Michael Kubacki <michael.kubacki@outlook.com>
> Sent: Wednesday, August 5, 2020 5:31 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
> 
> Hi Mike,
> 
> There's quite a few discrepancies at the moment between functions in
> FmpDxe that implement EFI_FIRMWARE_MANAGEMENT_PROTOCOL and the
> corresponding description in the UEFI spec.
> 
> For example:
> 
> UEFI Spec 2.8B - EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo():
> 
> Status Codes Returned
> 
> EFI_SUCCESS             The image information was successfully returned.
> EFI_BUFFER_TOO_SMALL    The ImageInfo buffer was too small. The current
>                          buffer size needed to hold the image(s)
>                          information is returned in ImageInfoSize.
> EFI_INVALID_PARAMETER   ImageInfoSize is not too small and ImageInfo is
>                          NULL.
> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorVersion
>                          is NULL.
> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorCount is
>                          NULL.
> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorSize is
>                          NULL.
> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and PackageVersion is
>                          NULL.
> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and PackageVersionName
>                          is NULL.
> EFI_DEVICE_ERROR        Valid information could not be returned.
>                          Possible corrupted image.
> 
> Actual - FmpDxe - GetTheImageInfo():
> 
>    @retval EFI_SUCCESS                The device was successfully updated
>                                       with the new image.
>    @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small.
>                                       The current buffer size
>                                       needed to hold the image(s)
>                                       information is returned in
>                                       ImageInfoSize.
>    @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>    @retval EFI_DEVICE_ERROR           Valid information could not be
>                                       returned. Possible corrupted image.
> 
> There's cases such as in GetTheImage() where the code describes
> EFI_INVALID_PARAMETER is returned as follows:
> 
>    @retval EFI_INVALID_PARAMETER  The Image was NULL.
> 
> However, the implementation will actually return the status code under
> other conditions such as an invalid ImageIndex or NULL ImageSize pointer.
> 
> I agree these should be cleaned up such that implementation and spec
> match and the descriptions are accurate, but that could warrant its own
> series.
> 
> For this series, is the ask to leave the descriptions as-is? If so, I
> can file a BZ to resolve the code/spec discrepancies.
> 
> Thanks,
> Michael
> 
> On 8/5/2020 4:30 PM, Michael D Kinney wrote:
> > Michael,
> >
> > That is a good point.  I missed that behavior in some of the APIs.
> >
> > What I also missed was that these APIs are defined in the UEFI Spec and the
> > description of the return codes is from there and should match the UEFI Spec.
> >
> > The implementation should be conformant with the UEFI Spec.  If you notice
> > behavior that is not conformant, then we need to update the code or potentially
> > work on spec updates.
> >
> > For this patch series, let’s make sure the Firmware Management Protocol service
> > headers match the UEFI Spec.
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Michael Kubacki <michael.kubacki@outlook.com>
> >> Sent: Wednesday, August 5, 2020 1:43 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> >> Cc: Gao, Liming <liming.gao@intel.com>
> >> Subject: Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
> >>
> >> Hi Mike,
> >>
> >> Some of those functions currently return EFI_SUCCESS if ImageIndex is
> >> invalid. Example:
> >> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L851
> >>
> >> Given the request to update the EFI_INVALID_PARAMETER text for those
> >> other functions, I'm assuming you'd like me to make those return
> >> EFI_INVALID_PARAMETER like what GetTheImage() currently does -
> >> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L573?
> >>
> >> Thanks,
> >> Michael
> >>
> >> On 8/5/2020 9:51 AM, Kinney, Michael D wrote:
> >>> Hi Michael,
> >>>
> >>> A few minor comments included below.  With those updates,
> >>>
> >>> Reviewed-bv: Michael D Kinney <michael.d.kinney@intel.com>
> >>>
> >>> Mike
> >>>
> >>>> -----Original Message-----
> >>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
> >>>> Sent: Thursday, July 30, 2020 8:15 PM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> >>>> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
> >>>>
> >>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>>>
> >>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2869
> >>>>
> >>>> Makes some minor improvements to function parameter validation
> >>>> in FmpDxe, in particular to externally exposed functions such
> >>>> as those that back EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
> >>>>
> >>>> Cc: Liming Gao <liming.gao@intel.com>
> >>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >>>> ---
> >>>>    FmpDevicePkg/FmpDxe/FmpDxe.c | 56 +++++++++++++++++---
> >>>>    FmpDevicePkg/FmpDxe/FmpDxe.h | 10 ++--
> >>>>    2 files changed, 54 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
> >>>> index a3e342591936..958d9b394b71 100644
> >>>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> >>>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> >>>> @@ -278,6 +278,11 @@ PopulateDescriptor (
> >>>>      EFI_STATUS  Status;
> >>>>      UINT32      DependenciesSize;
> >>>>
> >>>> +  if (Private == NULL) {
> >>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): PopulateDescriptor() - Private is NULL.\n", mImageIdName));
> >>>> +    return;
> >>>> +  }
> >>>> +
> >>>>      if (Private->DescriptorPopulated) {
> >>>>        return;
> >>>>      }
> >>>> @@ -429,7 +434,7 @@ PopulateDescriptor (
> >>>>      @retval EFI_SUCCESS                The device was successfully updated with the new image.
> >>>>      @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
> >>>>                                         needed to hold the image(s) information is returned in ImageInfoSize.
> >>>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
> >>>>      @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
> >>>>
> >>>>    **/
> >>>> @@ -451,6 +456,12 @@ GetTheImageInfo (
> >>>>
> >>>>      Status = EFI_SUCCESS;
> >>>>
> >>>> +  if (This == NULL) {
> >>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - This is NULL.\n", mImageIdName));
> >>>> +    Status = EFI_INVALID_PARAMETER;
> >>>> +    goto cleanup;
> >>>> +  }
> >>>> +
> >>>>      //
> >>>>      // Retrieve the private context structure
> >>>>      //
> >>>> @@ -536,7 +547,7 @@ GetTheImageInfo (
> >>>>      @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
> >>>>                                     image. The current buffer size needed to hold the image is returned
> >>>>                                     in ImageSize.
> >>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>>>      @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
> >>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
> >>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>>> @@ -561,6 +572,12 @@ GetTheImage (
> >>>>
> >>>>      Status = EFI_SUCCESS;
> >>>>
> >>>> +  if (This == NULL) {
> >>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImage() - This is NULL.\n", mImageIdName));
> >>>> +    Status = EFI_INVALID_PARAMETER;
> >>>> +    goto cleanup;
> >>>> +  }
> >>>> +
> >>>>      //
> >>>>      // Retrieve the private context structure
> >>>>      //
> >>>> @@ -615,7 +632,8 @@ GetTheImage (
> >>>>      @param[in]   Image                 Pointer to the image.
> >>>>      @param[in]   ImageSize             Size of the image.
> >>>>      @param[in]   AdditionalHeaderSize  Size of any headers that cannot be calculated by this function.
> >>>> -  @param[out]  PayloadSize
> >>>> +  @param[out]  PayloadSize           An optional pointer to a UINTN that holds the size of the payload
> >>>> +                                     (image size minus headers)
> >>>>
> >>>>      @retval  !NULL  Valid pointer to the header.
> >>>>      @retval  NULL   Structure is bad and pointer cannot be found.
> >>>> @@ -626,7 +644,7 @@ GetFmpHeader (
> >>>>      IN  CONST EFI_FIRMWARE_IMAGE_AUTHENTICATION  *Image,
> >>>>      IN  CONST UINTN                              ImageSize,
> >>>>      IN  CONST UINTN                              AdditionalHeaderSize,
> >>>> -  OUT UINTN                                    *PayloadSize
> >>>> +  OUT UINTN                                    *PayloadSize OPTIONAL
> >>>>      )
> >>>>    {
> >>>>      //
> >>>> @@ -640,7 +658,10 @@ GetFmpHeader (
> >>>>        return NULL;
> >>>>      }
> >>>>
> >>>> -  *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
> >>>> +  if (PayloadSize != NULL) {
> >>>> +    *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
> >>>> +  }
> >>>> +
> >>>>      return (VOID *)((UINT8 *)Image + sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength  + AdditionalHeaderSize);
> >>>>    }
> >>>>
> >>>> @@ -663,6 +684,10 @@ GetAllHeaderSize (
> >>>>    {
> >>>>      UINT32  CalculatedSize;
> >>>>
> >>>> +  if (Image == NULL) {
> >>>
> >>> This is an internal helper function.  If Image is ever NULL, it must be a bug in the
> >>> FmpDxe driver.  Should we do more than just return 0?  Perhaps a DEBUG_ERROR message too?
> >>>
> >>>> +    return 0;
> >>>> +  }
> >>>> +
> >>>>      CalculatedSize = sizeof (Image->MonotonicCount) +
> >>>>                       AdditionalHeaderSize +
> >>>>                       Image->AuthInfo.Hdr.dwLength;
> >>>> @@ -698,7 +723,7 @@ GetAllHeaderSize (
> >>>>
> >>>>      @retval EFI_SUCCESS            The image was successfully checked.
> >>>>      @retval EFI_ABORTED            The operation is aborted.
> >>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >>>
> >>> This function also uses ImageIndex.  Similar to updates above, I think this
> >>> @retval line should be:
> >>>
> >>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>>
> >>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
> >>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>>>
> >>>> @@ -743,6 +768,12 @@ CheckTheImage (
> >>>>        return EFI_UNSUPPORTED;
> >>>>      }
> >>>>
> >>>> +  if (This == NULL) {
> >>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
> >>>> +    Status = EFI_INVALID_PARAMETER;
> >>>> +    goto cleanup;
> >>>> +  }
> >>>> +
> >>>>      //
> >>>>      // Retrieve the private context structure
> >>>>      //
> >>>> @@ -978,7 +1009,7 @@ CheckTheImage (
> >>>>
> >>>>      @retval EFI_SUCCESS            The device was successfully updated with the new image.
> >>>>      @retval EFI_ABORTED            The operation is aborted.
> >>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >>>
> >>> This function also uses ImageIndex.  Similar to updates above, I think this
> >>> @retval line should be:
> >>>
> >>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>>
> >>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
> >>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>>>
> >>>> @@ -1026,6 +1057,12 @@ SetTheImage (
> >>>>        return EFI_UNSUPPORTED;
> >>>>      }
> >>>>
> >>>> +  if (This == NULL) {
> >>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
> >>>> +    Status = EFI_INVALID_PARAMETER;
> >>>> +    goto cleanup;
> >>>> +  }
> >>>> +
> >>>>      //
> >>>>      // Retrieve the private context structure
> >>>>      //
> >>>> @@ -1382,6 +1419,11 @@ FmpDxeLockEventNotify (
> >>>>      EFI_STATUS                        Status;
> >>>>      FIRMWARE_MANAGEMENT_PRIVATE_DATA  *Private;
> >>>>
> >>>> +  if (Context == NULL) {
> >>>> +    ASSERT (Context != NULL);
> >>>> +    return;
> >>>> +  }
> >>>> +
> >>>>      Private = (FIRMWARE_MANAGEMENT_PRIVATE_DATA *)Context;
> >>>>
> >>>>      if (!Private->FmpDeviceLocked) {
> >>>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
> >>>> index 30754dea495e..4dfec316a558 100644
> >>>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.h
> >>>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
> >>>> @@ -3,7 +3,7 @@
> >>>>      image stored in a firmware device with platform and firmware device specific
> >>>>      information provided through PCDs and libraries.
> >>>>
> >>>> -  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
> >>>> +  Copyright (c) Microsoft Corporation.<BR>
> >>>>      Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
> >>>>
> >>>>      SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>> @@ -132,7 +132,7 @@ DetectTestKey (
> >>>>      @retval EFI_SUCCESS                The device was successfully updated with the new image.
> >>>>      @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
> >>>>                                         needed to hold the image(s) information is returned in ImageInfoSize.
> >>>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
> >>>>      @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
> >>>>
> >>>>    **/
> >>>> @@ -166,7 +166,7 @@ GetTheImageInfo (
> >>>>      @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
> >>>>                                     image. The current buffer size needed to hold the image is returned
> >>>>                                     in ImageSize.
> >>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>>>      @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
> >>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
> >>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>>> @@ -198,7 +198,7 @@ GetTheImage (
> >>>>
> >>>>      @retval EFI_SUCCESS            The image was successfully checked.
> >>>>      @retval EFI_ABORTED            The operation is aborted.
> >>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >>>
> >>> This function also uses ImageIndex.  Similar to updates above, I think this
> >>> @retval line should be:
> >>>
> >>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>>
> >>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
> >>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>>>
> >>>> @@ -254,7 +254,7 @@ CheckTheImage (
> >>>>
> >>>>      @retval EFI_SUCCESS            The device was successfully updated with the new image.
> >>>>      @retval EFI_ABORTED            The operation is aborted.
> >>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
> >>>
> >>> This function also uses ImageIndex.  Similar to updates above, I think this
> >>> @retval line should be:
> >>>
> >>>     @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
> >>>
> >>>>      @retval EFI_UNSUPPORTED        The operation is not supported.
> >>>>      @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
> >>>>
> >>>> --
> >>>> 2.27.0.windows.1
> >>>
> >
> > 
> >

Re: [edk2-devel] [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation

[Michael Kubacki]

Sounds good, I'll send v3 out soon.

Here's the BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2878

Thanks,
Michael

On 8/6/2020 9:06 AM, Kinney, Michael D wrote:
> Michael,
> 
> The description matches UEFI 2.7.  So there appears to be a work item to update the FMP
> function descriptions to the latest UEFI 2.8 spec.
> 
> I do recommend you do not change any of these comments in the current patch series.
> The update to UEFI 2.8 can be a new BZ.
> 
> The UEFI Specifications allows an implementation to return additional error return codes
> that are not listed in the API definition.
> 
> 
>    Status Codes Returned: A description of any codes returned by the interface. The
>      procedure is required to implement any status codes
>      listed in this table. Additional error codes may be
>      returned, but they will not be tested by standard
>      compliance tests, and any software that uses the
>      procedure cannot depend on any of the extended error
>      codes that an implementation may provide.
> 
> Best regards.
> 
> Mike
> 
>> -----Original Message-----
>> From: Michael Kubacki <michael.kubacki@outlook.com>
>> Sent: Wednesday, August 5, 2020 5:31 PM
>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
>>
>> Hi Mike,
>>
>> There's quite a few discrepancies at the moment between functions in
>> FmpDxe that implement EFI_FIRMWARE_MANAGEMENT_PROTOCOL and the
>> corresponding description in the UEFI spec.
>>
>> For example:
>>
>> UEFI Spec 2.8B - EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo():
>>
>> Status Codes Returned
>>
>> EFI_SUCCESS             The image information was successfully returned.
>> EFI_BUFFER_TOO_SMALL    The ImageInfo buffer was too small. The current
>>                           buffer size needed to hold the image(s)
>>                           information is returned in ImageInfoSize.
>> EFI_INVALID_PARAMETER   ImageInfoSize is not too small and ImageInfo is
>>                           NULL.
>> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorVersion
>>                           is NULL.
>> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorCount is
>>                           NULL.
>> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and DescriptorSize is
>>                           NULL.
>> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and PackageVersion is
>>                           NULL.
>> EFI_INVALID_PARAMETER   ImageInfoSize is non-zero and PackageVersionName
>>                           is NULL.
>> EFI_DEVICE_ERROR        Valid information could not be returned.
>>                           Possible corrupted image.
>>
>> Actual - FmpDxe - GetTheImageInfo():
>>
>>     @retval EFI_SUCCESS                The device was successfully updated
>>                                        with the new image.
>>     @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small.
>>                                        The current buffer size
>>                                        needed to hold the image(s)
>>                                        information is returned in
>>                                        ImageInfoSize.
>>     @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>>     @retval EFI_DEVICE_ERROR           Valid information could not be
>>                                        returned. Possible corrupted image.
>>
>> There's cases such as in GetTheImage() where the code describes
>> EFI_INVALID_PARAMETER is returned as follows:
>>
>>     @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>
>> However, the implementation will actually return the status code under
>> other conditions such as an invalid ImageIndex or NULL ImageSize pointer.
>>
>> I agree these should be cleaned up such that implementation and spec
>> match and the descriptions are accurate, but that could warrant its own
>> series.
>>
>> For this series, is the ask to leave the descriptions as-is? If so, I
>> can file a BZ to resolve the code/spec discrepancies.
>>
>> Thanks,
>> Michael
>>
>> On 8/5/2020 4:30 PM, Michael D Kinney wrote:
>>> Michael,
>>>
>>> That is a good point.  I missed that behavior in some of the APIs.
>>>
>>> What I also missed was that these APIs are defined in the UEFI Spec and the
>>> description of the return codes is from there and should match the UEFI Spec.
>>>
>>> The implementation should be conformant with the UEFI Spec.  If you notice
>>> behavior that is not conformant, then we need to update the code or potentially
>>> work on spec updates.
>>>
>>> For this patch series, let’s make sure the Firmware Management Protocol service
>>> headers match the UEFI Spec.
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: Michael Kubacki <michael.kubacki@outlook.com>
>>>> Sent: Wednesday, August 5, 2020 1:43 PM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
>>>> Cc: Gao, Liming <liming.gao@intel.com>
>>>> Subject: Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
>>>>
>>>> Hi Mike,
>>>>
>>>> Some of those functions currently return EFI_SUCCESS if ImageIndex is
>>>> invalid. Example:
>>>> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L851
>>>>
>>>> Given the request to update the EFI_INVALID_PARAMETER text for those
>>>> other functions, I'm assuming you'd like me to make those return
>>>> EFI_INVALID_PARAMETER like what GetTheImage() currently does -
>>>> https://github.com/tianocore/edk2/blob/master/FmpDevicePkg/FmpDxe/FmpDxe.c#L573?
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>> On 8/5/2020 9:51 AM, Kinney, Michael D wrote:
>>>>> Hi Michael,
>>>>>
>>>>> A few minor comments included below.  With those updates,
>>>>>
>>>>> Reviewed-bv: Michael D Kinney <michael.d.kinney@intel.com>
>>>>>
>>>>> Mike
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>>>>>> Sent: Thursday, July 30, 2020 8:15 PM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>>>>>> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
>>>>>>
>>>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>>
>>>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2869
>>>>>>
>>>>>> Makes some minor improvements to function parameter validation
>>>>>> in FmpDxe, in particular to externally exposed functions such
>>>>>> as those that back EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
>>>>>>
>>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>> ---
>>>>>>     FmpDevicePkg/FmpDxe/FmpDxe.c | 56 +++++++++++++++++---
>>>>>>     FmpDevicePkg/FmpDxe/FmpDxe.h | 10 ++--
>>>>>>     2 files changed, 54 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
>>>>>> index a3e342591936..958d9b394b71 100644
>>>>>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
>>>>>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
>>>>>> @@ -278,6 +278,11 @@ PopulateDescriptor (
>>>>>>       EFI_STATUS  Status;
>>>>>>       UINT32      DependenciesSize;
>>>>>>
>>>>>> +  if (Private == NULL) {
>>>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): PopulateDescriptor() - Private is NULL.\n", mImageIdName));
>>>>>> +    return;
>>>>>> +  }
>>>>>> +
>>>>>>       if (Private->DescriptorPopulated) {
>>>>>>         return;
>>>>>>       }
>>>>>> @@ -429,7 +434,7 @@ PopulateDescriptor (
>>>>>>       @retval EFI_SUCCESS                The device was successfully updated with the new image.
>>>>>>       @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>>>>>>                                          needed to hold the image(s) information is returned in ImageInfoSize.
>>>>>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>>>>>>       @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
>>>>>>
>>>>>>     **/
>>>>>> @@ -451,6 +456,12 @@ GetTheImageInfo (
>>>>>>
>>>>>>       Status = EFI_SUCCESS;
>>>>>>
>>>>>> +  if (This == NULL) {
>>>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - This is NULL.\n", mImageIdName));
>>>>>> +    Status = EFI_INVALID_PARAMETER;
>>>>>> +    goto cleanup;
>>>>>> +  }
>>>>>> +
>>>>>>       //
>>>>>>       // Retrieve the private context structure
>>>>>>       //
>>>>>> @@ -536,7 +547,7 @@ GetTheImageInfo (
>>>>>>       @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>>>>>>                                      image. The current buffer size needed to hold the image is returned
>>>>>>                                      in ImageSize.
>>>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>>>       @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>>>>>>       @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>>>       @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>>> @@ -561,6 +572,12 @@ GetTheImage (
>>>>>>
>>>>>>       Status = EFI_SUCCESS;
>>>>>>
>>>>>> +  if (This == NULL) {
>>>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImage() - This is NULL.\n", mImageIdName));
>>>>>> +    Status = EFI_INVALID_PARAMETER;
>>>>>> +    goto cleanup;
>>>>>> +  }
>>>>>> +
>>>>>>       //
>>>>>>       // Retrieve the private context structure
>>>>>>       //
>>>>>> @@ -615,7 +632,8 @@ GetTheImage (
>>>>>>       @param[in]   Image                 Pointer to the image.
>>>>>>       @param[in]   ImageSize             Size of the image.
>>>>>>       @param[in]   AdditionalHeaderSize  Size of any headers that cannot be calculated by this function.
>>>>>> -  @param[out]  PayloadSize
>>>>>> +  @param[out]  PayloadSize           An optional pointer to a UINTN that holds the size of the payload
>>>>>> +                                     (image size minus headers)
>>>>>>
>>>>>>       @retval  !NULL  Valid pointer to the header.
>>>>>>       @retval  NULL   Structure is bad and pointer cannot be found.
>>>>>> @@ -626,7 +644,7 @@ GetFmpHeader (
>>>>>>       IN  CONST EFI_FIRMWARE_IMAGE_AUTHENTICATION  *Image,
>>>>>>       IN  CONST UINTN                              ImageSize,
>>>>>>       IN  CONST UINTN                              AdditionalHeaderSize,
>>>>>> -  OUT UINTN                                    *PayloadSize
>>>>>> +  OUT UINTN                                    *PayloadSize OPTIONAL
>>>>>>       )
>>>>>>     {
>>>>>>       //
>>>>>> @@ -640,7 +658,10 @@ GetFmpHeader (
>>>>>>         return NULL;
>>>>>>       }
>>>>>>
>>>>>> -  *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
>>>>>> +  if (PayloadSize != NULL) {
>>>>>> +    *PayloadSize = ImageSize - (sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength + AdditionalHeaderSize);
>>>>>> +  }
>>>>>> +
>>>>>>       return (VOID *)((UINT8 *)Image + sizeof (Image->MonotonicCount) + Image->AuthInfo.Hdr.dwLength  + AdditionalHeaderSize);
>>>>>>     }
>>>>>>
>>>>>> @@ -663,6 +684,10 @@ GetAllHeaderSize (
>>>>>>     {
>>>>>>       UINT32  CalculatedSize;
>>>>>>
>>>>>> +  if (Image == NULL) {
>>>>>
>>>>> This is an internal helper function.  If Image is ever NULL, it must be a bug in the
>>>>> FmpDxe driver.  Should we do more than just return 0?  Perhaps a DEBUG_ERROR message too?
>>>>>
>>>>>> +    return 0;
>>>>>> +  }
>>>>>> +
>>>>>>       CalculatedSize = sizeof (Image->MonotonicCount) +
>>>>>>                        AdditionalHeaderSize +
>>>>>>                        Image->AuthInfo.Hdr.dwLength;
>>>>>> @@ -698,7 +723,7 @@ GetAllHeaderSize (
>>>>>>
>>>>>>       @retval EFI_SUCCESS            The image was successfully checked.
>>>>>>       @retval EFI_ABORTED            The operation is aborted.
>>>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>>>
>>>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>>>> @retval line should be:
>>>>>
>>>>>      @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>>
>>>>>>       @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>>>       @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>>>
>>>>>> @@ -743,6 +768,12 @@ CheckTheImage (
>>>>>>         return EFI_UNSUPPORTED;
>>>>>>       }
>>>>>>
>>>>>> +  if (This == NULL) {
>>>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
>>>>>> +    Status = EFI_INVALID_PARAMETER;
>>>>>> +    goto cleanup;
>>>>>> +  }
>>>>>> +
>>>>>>       //
>>>>>>       // Retrieve the private context structure
>>>>>>       //
>>>>>> @@ -978,7 +1009,7 @@ CheckTheImage (
>>>>>>
>>>>>>       @retval EFI_SUCCESS            The device was successfully updated with the new image.
>>>>>>       @retval EFI_ABORTED            The operation is aborted.
>>>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>>>
>>>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>>>> @retval line should be:
>>>>>
>>>>>      @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>>
>>>>>>       @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>>>       @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>>>
>>>>>> @@ -1026,6 +1057,12 @@ SetTheImage (
>>>>>>         return EFI_UNSUPPORTED;
>>>>>>       }
>>>>>>
>>>>>> +  if (This == NULL) {
>>>>>> +    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
>>>>>> +    Status = EFI_INVALID_PARAMETER;
>>>>>> +    goto cleanup;
>>>>>> +  }
>>>>>> +
>>>>>>       //
>>>>>>       // Retrieve the private context structure
>>>>>>       //
>>>>>> @@ -1382,6 +1419,11 @@ FmpDxeLockEventNotify (
>>>>>>       EFI_STATUS                        Status;
>>>>>>       FIRMWARE_MANAGEMENT_PRIVATE_DATA  *Private;
>>>>>>
>>>>>> +  if (Context == NULL) {
>>>>>> +    ASSERT (Context != NULL);
>>>>>> +    return;
>>>>>> +  }
>>>>>> +
>>>>>>       Private = (FIRMWARE_MANAGEMENT_PRIVATE_DATA *)Context;
>>>>>>
>>>>>>       if (!Private->FmpDeviceLocked) {
>>>>>> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
>>>>>> index 30754dea495e..4dfec316a558 100644
>>>>>> --- a/FmpDevicePkg/FmpDxe/FmpDxe.h
>>>>>> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
>>>>>> @@ -3,7 +3,7 @@
>>>>>>       image stored in a firmware device with platform and firmware device specific
>>>>>>       information provided through PCDs and libraries.
>>>>>>
>>>>>> -  Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
>>>>>> +  Copyright (c) Microsoft Corporation.<BR>
>>>>>>       Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
>>>>>>
>>>>>>       SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>>> @@ -132,7 +132,7 @@ DetectTestKey (
>>>>>>       @retval EFI_SUCCESS                The device was successfully updated with the new image.
>>>>>>       @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too small. The current buffer size
>>>>>>                                          needed to hold the image(s) information is returned in ImageInfoSize.
>>>>>> -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER      A required pointer is NULL.
>>>>>>       @retval EFI_DEVICE_ERROR           Valid information could not be returned. Possible corrupted image.
>>>>>>
>>>>>>     **/
>>>>>> @@ -166,7 +166,7 @@ GetTheImageInfo (
>>>>>>       @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is too small to hold the
>>>>>>                                      image. The current buffer size needed to hold the image is returned
>>>>>>                                      in ImageSize.
>>>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>>>       @retval EFI_NOT_FOUND          The current image is not copied to the buffer.
>>>>>>       @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>>>       @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>>> @@ -198,7 +198,7 @@ GetTheImage (
>>>>>>
>>>>>>       @retval EFI_SUCCESS            The image was successfully checked.
>>>>>>       @retval EFI_ABORTED            The operation is aborted.
>>>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>>>
>>>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>>>> @retval line should be:
>>>>>
>>>>>      @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>>
>>>>>>       @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>>>       @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>>>
>>>>>> @@ -254,7 +254,7 @@ CheckTheImage (
>>>>>>
>>>>>>       @retval EFI_SUCCESS            The device was successfully updated with the new image.
>>>>>>       @retval EFI_ABORTED            The operation is aborted.
>>>>>> -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>>>>>> +  @retval EFI_INVALID_PARAMETER  A required pointer is NULL.
>>>>>
>>>>> This function also uses ImageIndex.  Similar to updates above, I think this
>>>>> @retval line should be:
>>>>>
>>>>>      @retval EFI_INVALID_PARAMETER  A required pointer is NULL or ImageIndex is invalid.
>>>>>
>>>>>>       @retval EFI_UNSUPPORTED        The operation is not supported.
>>>>>>       @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
>>>>>>
>>>>>> --
>>>>>> 2.27.0.windows.1
>>>>>
>>>
>>> 
>>>