[edk2-devel] [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
Michael D Kinney
michael.d.kinney at intel.com
Wed Aug 5 23:30:07 UTC 2020
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 at outlook.com>
> Sent: Wednesday, August 5, 2020 1:43 PM
> To: Kinney, Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io
> Cc: Gao, Liming <liming.gao at 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 at intel.com>
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: michael.kubacki at outlook.com <michael.kubacki at outlook.com>
> >> Sent: Thursday, July 30, 2020 8:15 PM
> >> To: devel at edk2.groups.io
> >> Cc: Gao, Liming <liming.gao at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>
> >> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
> >>
> >> From: Michael Kubacki <michael.kubacki at 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 at intel.com>
> >> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> >> Signed-off-by: Michael Kubacki <michael.kubacki at 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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#63749): https://edk2.groups.io/g/devel/message/63749
Mute This Topic: https://groups.io/mt/75900913/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list