From: "Michael Kubacki" <michael.kubacki@outlook.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation
Date: Wed, 5 Aug 2020 13:42:48 -0700 [thread overview]
Message-ID: <MWHPR07MB344088D5813702AED6402AF8E94B0@MWHPR07MB3440.namprd07.prod.outlook.com> (raw)
In-Reply-To: <MN2PR11MB4461C03786C3092EF71BFE6CD24B0@MN2PR11MB4461.namprd11.prod.outlook.com>
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
>
next prev parent reply other threads:[~2020-08-05 20:42 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200731031448.1103-1-michael.kubacki@outlook.com>
2020-07-31 3:14 ` [PATCH v1 1/7] FmpDevicePkg/FmpDependencyLib: Correct ValidateDependency() documentation Michael Kubacki
2020-08-05 16:19 ` Michael D Kinney
2020-07-31 3:14 ` [PATCH v1 2/7] FmpDevicePkg/FmpDependencyLib: Fix "exression" typo Michael Kubacki
2020-08-05 16:08 ` Michael D Kinney
2020-07-31 3:14 ` [PATCH v1 3/7] FmpDevicePkg/FmpDependencyLib: Handle version string overflow Michael Kubacki
2020-08-05 16:13 ` Michael D Kinney
2020-07-31 3:14 ` [PATCH v1 4/7] FmpDevicePkg/FmpDependencyCheckLib: Return unsatisfied on handle failure Michael Kubacki
2020-08-05 16:16 ` Michael D Kinney
2020-07-31 3:14 ` [PATCH v1 5/7] FmpDevicePkg/FmpDxe: Better warn of potential ImageTypeId misconfig Michael Kubacki
2020-08-05 16:17 ` Michael D Kinney
2020-07-31 3:14 ` [PATCH v1 6/7] FmpDevicePkg/FmpDxe: Indicate ESRT GUID on invalid ImageIdName Michael Kubacki
2020-08-05 16:17 ` [edk2-devel] " Michael D Kinney
2020-07-31 3:14 ` [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation Michael Kubacki
2020-08-05 16:51 ` Michael D Kinney
2020-08-05 20:42 ` Michael Kubacki [this message]
2020-08-05 23:30 ` Michael D Kinney
2020-08-06 0:30 ` [edk2-devel] " Michael Kubacki
2020-08-06 16:06 ` Michael D Kinney
2020-08-06 18:22 ` Michael Kubacki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR07MB344088D5813702AED6402AF8E94B0@MWHPR07MB3440.namprd07.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox