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.kuba...@outlook.com> > Sent: Wednesday, August 5, 2020 1:43 PM > To: Kinney, Michael D <michael.d.kin...@intel.com>; devel@edk2.groups.io > Cc: Gao, Liming <liming....@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.kin...@intel.com> > > > > Mike > > > >> -----Original Message----- > >> From: michael.kuba...@outlook.com <michael.kuba...@outlook.com> > >> Sent: Thursday, July 30, 2020 8:15 PM > >> To: devel@edk2.groups.io > >> Cc: Gao, Liming <liming....@intel.com>; Kinney, Michael D > >> <michael.d.kin...@intel.com> > >> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter > >> validation > >> > >> From: Michael Kubacki <michael.kuba...@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....@intel.com> > >> Cc: Michael D Kinney <michael.d.kin...@intel.com> > >> Signed-off-by: Michael Kubacki <michael.kuba...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
