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

["Michael Kubacki" <michael.kubacki@outlook.com>]

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