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 (#63735): https://edk2.groups.io/g/devel/message/63735 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] -=-=-=-=-=-=-=-=-=-=-=-
