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

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

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