From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (NAM11-DM6-obe.outbound.protection.outlook.com [40.92.19.56]) by mx.groups.io with SMTP id smtpd.web10.115.1596660169385093251 for ; Wed, 05 Aug 2020 13:42:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@outlook.com header.s=selector1 header.b=Rhtg+1PB; spf=pass (domain: outlook.com, ip: 40.92.19.56, mailfrom: michael.kubacki@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dnZbaq/0FOvLdQJsyotYoSJp7yg+d/C33MmKtqizG2CAJ6iErkQHmxh+87T0mWKVzQ326u/gfjkNagHENwn3cv+ZxCKDYih4njQQF/vka9Z3fjvY0qDLpkfEyhdDu2EK0cLAFPrW0E6GhlYuyo06NAwb+7bttwAFaWvQNRbu46FeMnF9C0paKQ/CbNbLGYQ1bQkOPtJPXk7TSMZCd7fwVwtBOCM8symjrr5WnYstq/loqM7SjE9QNNV7IKKZVlLFyjOY/XMG9zzhCgrM5lHxhm+tXDjBiYZUIQWpvbnAbNp7rTFwOv3D3oSdNoLkYsZnIlxpUY2lrfKTCuHHYNJkIQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=KK/reL1CjV/+HwEhu+CCU74oTSogSATLjvdna5gLZBw=; b=FRblgJ11+m7a2EJT86JPceRDn+B+U3VNKZTY7ZM+Yo+gTnHUOMM0Lek/W2YvVrAnMwJZhTO1e7VyBhjyQ7ee/XRqf3i/AlOw9sxZ8pxLPt5biKka92jxZ+ltmPKpQyBo3+mlUc8fBBCPh8R+AaVPW3moPhvFemZ1WOuyIno0x1fwYU3KKFL8PdaMm8pC/lzvQZof1ln+3UaSwXnaI6u8qnnd2O2FjkMlzvcZ9kf3ZnQA2s2QPBtPBKxnauGGN0yvKSSb1CcZjBGv5a09fAvmzAk6+sS+RzNAk0lVc1006DjInK73jGtlrdjCpZftYVHqXexd787YmP+2OK0Ir4o4CA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=KK/reL1CjV/+HwEhu+CCU74oTSogSATLjvdna5gLZBw=; b=Rhtg+1PBjnA2hVViaRz/uaBT6ER1G99biEUrL9TgfFIlPaSb07vlBHjMUdZLQ4VeB5c1ChSJ5pZMYVmb0RHh3e0mwvTJso4ktPBWKQo+7M5Su8EBx/dh8ZhvRREki4PCXBdA6LXVons313forl5vt1C2wFc6qVKeXLArBTsOl7DUqyd03Pgop/UTXvkP/BeVTrEFK1uMmcOE6vkA+CWQzCliJtzWHcwDigP9f97CrqLuKGu8To6Vc8YfV5x2wPkLmUPan6tMle7pktMbwMSshlf5NL4DCsqoPri7G5HSuS34NSTA0mcLJkV8UzpfbGVHD5s/xMztTWBjJ3W3pF7y9w== Received: from DM6NAM11FT068.eop-nam11.prod.protection.outlook.com (2a01:111:e400:fc4d::4b) by DM6NAM11HT218.eop-nam11.prod.protection.outlook.com (2a01:111:e400:fc4d::331) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3239.20; Wed, 5 Aug 2020 20:42:48 +0000 Received: from MWHPR07MB3440.namprd07.prod.outlook.com (2a01:111:e400:fc4d::44) by DM6NAM11FT068.mail.protection.outlook.com (2a01:111:e400:fc4d::323) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3261.16 via Frontend Transport; Wed, 5 Aug 2020 20:42:48 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:356F41DE3DD392585325A6F30025C6EF89469884476C6AA6F3B71D22110C58E0;UpperCasedChecksum:B7765743401809DD81009C746507EA82A882B82E504AA7E6D34EC9739D418E6D;SizeAsReceived:9177;Count:49 Received: from MWHPR07MB3440.namprd07.prod.outlook.com ([fe80::9856:570e:1735:974e]) by MWHPR07MB3440.namprd07.prod.outlook.com ([fe80::9856:570e:1735:974e%7]) with mapi id 15.20.3239.022; Wed, 5 Aug 2020 20:42:48 +0000 Subject: Re: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: "Gao, Liming" References: <20200731031448.1103-1-michael.kubacki@outlook.com> From: "Michael Kubacki" Message-ID: Date: Wed, 5 Aug 2020 13:42:48 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 In-Reply-To: X-ClientProxiedBy: CO2PR07CA0060.namprd07.prod.outlook.com (2603:10b6:100::28) To MWHPR07MB3440.namprd07.prod.outlook.com (2603:10b6:301:69::28) Return-Path: michael.kubacki@outlook.com X-Microsoft-Original-Message-ID: MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [IPv6:2001:4898:d8:39:759d:f844:3092:4e93] (2001:4898:80e8:0:f5c5:f844:3092:4e93) by CO2PR07CA0060.namprd07.prod.outlook.com (2603:10b6:100::28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3261.15 via Frontend Transport; Wed, 5 Aug 2020 20:42:47 +0000 X-Microsoft-Original-Message-ID: X-TMN: [6vD5FaJCjD3TWJP5yiWcEZKAddjwA8kD+MQjnwm9YZIIg9v8pasvaSu3DrrL2nON] X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 49 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: c0ea3dd0-363f-4cf1-3bff-08d839801cb1 X-MS-TrafficTypeDiagnostic: DM6NAM11HT218: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: n6G6agzdxZhPIlQA0BoIFril81hQDQy9JyDSswaexn9L0vcit+ijiPmkJpnyd5cL7bHbi8w4OhQvvGTk5ljN0HjafwM3QCiu1Hgarw9HhEkvuXpL8uIg9IaggZNKpRk4fZjIGAA06LSTyqC5yJgtcawnAl83o+R8y1lfC1SiAD1tLrIu2wTs7m4N9sdFe/OwXonGx6D1Df81Q4HUpccVC8h+D9KSog7AIyhIElm1qvOZ2fYFfnEGomArwkabJdTh X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:0;SRV:;IPV:NLI;SFV:NSPM;H:MWHPR07MB3440.namprd07.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:;DIR:OUT;SFP:1901; X-MS-Exchange-AntiSpam-MessageData: MuH7Xbi1SlFsq2MPGzIKVERGA/R8ONEJAcqiCmnVszCnyR7TEPF/62jPMiocaFrNCMP7bEoF65TH0tcNla2fOmW0XVf8yFBIHlWyylRrMOIjnVvGVQf97IHkngnD7I4HnR4EJDoQtQHs7KYyPoFtx0cKSgdt0NlQYE7hDPSF+FCKfKPUtNK/dJaRILgudiPdZ71ggoldD9e1Zx9L06RqBg== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: c0ea3dd0-363f-4cf1-3bff-08d839801cb1 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Aug 2020 20:42:48.3153 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT068.eop-nam11.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: Internet X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6NAM11HT218 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > > Mike > >> -----Original Message----- >> From: michael.kubacki@outlook.com >> Sent: Thursday, July 30, 2020 8:15 PM >> To: devel@edk2.groups.io >> Cc: Gao, Liming ; Kinney, Michael D >> Subject: [PATCH v1 7/7] FmpDevicePkg/FmpDxe: Improve function parameter validation >> >> From: Michael Kubacki >> >> 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 >> Cc: Michael D Kinney >> Signed-off-by: Michael Kubacki >> --- >> 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.
>> + Copyright (c) Microsoft Corporation.
>> Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.
>> >> 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 >