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

[PATCH v3 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>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
Reviewed-by: Wei6 Xu <wei6.xu@intel.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 v3 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>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
Reviewed-by: Wei6 Xu <wei6.xu@intel.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 v3 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>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
Reviewed-by: Wei6 Xu <wei6.xu@intel.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 v3 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>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
Reviewed-by: Wei6 Xu <wei6.xu@intel.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 v3 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>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
Reviewed-by: Wei6 Xu <wei6.xu@intel.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 v3 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>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
Reviewed-by: Wei6 Xu <wei6.xu@intel.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 v3 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>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
Reviewed-by: Wei6 Xu <wei6.xu@intel.com>
---
 FmpDevicePkg/FmpDxe/FmpDxe.c | 51 ++++++++++++++++++--
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index a3e342591936..854feec0a162 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;
   }
@@ -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
   //
@@ -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,11 @@ GetAllHeaderSize (
 {
   UINT32  CalculatedSize;
 
+  if (Image == NULL) {
+    DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetAllHeaderSize() - Image is NULL.\n", mImageIdName));
+    return 0;
+  }
+
   CalculatedSize = sizeof (Image->MonotonicCount) +
                    AdditionalHeaderSize +
                    Image->AuthInfo.Hdr.dwLength;
@@ -743,6 +769,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
   //
@@ -851,7 +883,7 @@ CheckTheImage (
   if (ImageIndex != 1) {
     DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - Image Index Invalid.\n", mImageIdName));
     *ImageUpdatable = IMAGE_UPDATABLE_INVALID_TYPE;
-    Status = EFI_SUCCESS;
+    Status = EFI_INVALID_PARAMETER;
     goto cleanup;
   }
 
@@ -1026,6 +1058,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 +1420,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) {
-- 
2.27.0.windows.1