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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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