From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 39BB181E9C for ; Fri, 20 Jan 2017 01:35:18 -0800 (PST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga105.fm.intel.com with ESMTP; 20 Jan 2017 01:35:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,258,1477983600"; d="scan'208";a="1115414563" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 20 Jan 2017 01:35:17 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 20 Jan 2017 01:35:17 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 20 Jan 2017 01:35:17 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.132]) with mapi id 14.03.0248.002; Fri, 20 Jan 2017 17:35:15 +0800 From: "Gao, Liming" To: Laszlo Ersek , Daniel Samuelraj CC: Jianning Wang , "edk2-devel@lists.01.org" , Sathya Prakash , Chidambara GR Thread-Topic: [edk2] SCT 2.3.1 v1.3 Thread-Index: AdJbFAPuQQ4X5ROyTkWret63bEcHDQV84UFwAAeK1wAAbbOnAA== Date: Fri, 20 Jan 2017 09:35:14 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6D1E85@shsmsx102.ccr.corp.intel.com> References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: SCT 2.3.1 v1.3 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Jan 2017 09:35:18 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo: I agree this is gap between SCT and UEFI spec. I think UEFI spec can be u= pdated to describe these error conditions.=20 Thanks Liming >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >Laszlo Ersek >Sent: Wednesday, January 18, 2017 4:57 PM >To: Daniel Samuelraj >Cc: Jianning Wang ; edk2-devel@lists.01.org; >Sathya Prakash ; Chidambara GR > >Subject: Re: [edk2] SCT 2.3.1 v1.3 > >(Dropping the and email addresses; >please never cross-post the public edk2-devel list with the confidential >spec development / working group lists on UEFI.org!) > >On 01/17/17 22:22, Daniel Samuelraj wrote: >> Hi, >> >> What SCT v1.3 states for HII Config Access Protocol seems not in align >> with UEFI spec? For example, for extract config, when Progress or >> Result or Request is NULL, SCT is expecting EFI Invalid Parameter; >> similarly for Route Config, when progress is NULL, SCT expects >> EFI_INVALID_PARAMETER. UEFI spec doesn\u2019t seem mention anything >> for these cases. >> >> >> >> Should driver adhere to what SCT expects? Or is this fixed in newer >> SCT or will this be addressed in future? Please advise! > >I confirm this is a problem between the SCT and the UEFI spec. > >I've never personally built or used the SCT, but in 2015, Heyi Guo from >Linaro submitted patches for OVMF's PlatformDxe, some of which, for >example > > [PATCH 2/3] OvmfPkg: PlatformDxe: Add sanity check for HiiConfigAccess > >suppressed this kind of SCT failure report, by modifying OvmfPkg code. > >I didn't accept the patch, because we couldn't find any normative >reference (released UEFI spec version, or pending Mantis ticket) that >justified the SCT's expectations. > >... I would like to provide you with a mailing list archive link, but >that discussion was apparently only captured in the old GMANE archive, >and GMANE went belly-up a few months ago. Albeit being resuscitated, the >edk2-devel messages seem to be lost from it for good (or at least cannot >be looked up based on Message-Id). > >For lack of a better means, I'll quote one message from that thread >below, with context. > >Thanks >Laszlo > >On 06/01/15 16:27, Laszlo Ersek wrote: >> On 06/01/15 16:14, Laszlo Ersek wrote: >>> On 06/01/15 14:08, Heyi Guo wrote: >>>> During UEFI SCT, it will throw an exception because "Progress" is >>>> passed in with NULL and RouteConfig will try to access the string at >>>> *(EFI_STRING *0), i.e. 0xFFFFFFFF14000400. >>>> >>>> Add sanity check for ExtractConfig and RouteConfig to avoid NULL >>>> pointer dereference. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Heyi Guo >>>> --- >>>> OvmfPkg/PlatformDxe/Platform.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/OvmfPkg/PlatformDxe/Platform.c >b/OvmfPkg/PlatformDxe/Platform.c >>>> index 4ec327e..35fabf8 100644 >>>> --- a/OvmfPkg/PlatformDxe/Platform.c >>>> +++ b/OvmfPkg/PlatformDxe/Platform.c >>>> @@ -234,6 +234,11 @@ ExtractConfig ( >>>> MAIN_FORM_STATE MainFormState; >>>> EFI_STATUS Status; >>>> >>>> + if (Progress =3D=3D NULL || Results =3D=3D NULL) >>>> + { >>>> + return EFI_INVALID_PARAMETER; >>>> + } >>>> + >>>> DEBUG ((EFI_D_VERBOSE, "%a: Request=3D\"%s\"\n", __FUNCTION__, >Request)); >>>> >>>> Status =3D PlatformConfigToFormState (&MainFormState); >>> >>> EFI_HII_CONFIG_ROUTING_PROTOCOL.ExtractConfig() does not require >any of >>> these checks. Both Progress and Results are OUT parameters (ie. >>> pointers) and nothing in the spec resolves the caller from passing in >>> NULL (or non-NULL garbage, for that matter, which you couldn't catch >>> anyway). >>> >>> I can ACK this hunk if you show me a confirmed Mantis ticket for the >>> UEFI spec. >> >> Sorry, I just noticed that above I referenced >> EFI_HII_CONFIG_ROUTING_PROTOCOL rather than >EFI_HII_CONFIG_ACCESS_PROTOCOL. >> >> However, after checking >EFI_HII_CONFIG_ACCESS_PROTOCOL.ExtractConfig() >> specifically, I still can't see any requirement for these checks. >> >>>> @@ -327,6 +332,11 @@ RouteConfig ( >>>> UINTN BlockSize; >>>> EFI_STATUS Status; >>>> >>>> + if (Configuration =3D=3D NULL || Progress =3D=3D NULL) >>>> + { >>>> + return EFI_INVALID_PARAMETER; >>>> + } >>>> + >>>> DEBUG ((EFI_D_VERBOSE, "%a: Configuration=3D\"%s\"\n", >__FUNCTION__, >>>> Configuration)); >>>> >>>> >>> >>> The (Configuration =3D=3D NULL) check is valid, according to UEFI-2.5. = But, >>> please update the leading comment on the function accordingly -- please >>> document the EFI_INVALID_PARAMETER retval. >>> >>> The (Progress =3D=3D NULL) check is not required by the spec, and the >>> responsibility of the caller is made clear for this output parameter: >>> >>> Progress >>> >>> A pointer to a string filled in with the offset of the most recent >>> \u2018&\u2019 before the first failing name / value pair (or the >beginning of >>> the string if the failure is in the first name / value pair) or the >>> terminating NULL if all was successful. >>> >>> "Pointer to a string" implies Progress cannot be NULL. (And, on output, >>> *Progress will point into Configuration.) >>> >>> I do understand that suppressing these SCT bugs in OVMF would be easy. >>> That doesn't make it right. Not NACKing this, but you'll have to get an >>> ACK from Jordan. (Personally I'm okay only with the >>> (Configuration=3D=3DNULL) check in RouteConfig().) >> >> Again, I looked at the wrong part of the spec, sorry about that. >> >> But, EFI_HII_CONFIG_ACCESS_PROTOCOL.RouteConfig() doesn't even >require a >> nullity check for Configuration. (It speaks about "Results", which >> doesn't exist as a parameter in that function -- this is a spec bug.) >> >> Please ask someone else to give his/her R-b, because I don't think I wil= l. >> >> Thanks >> Laszlo >> >> ------------------------------------------------------------------------= ------ >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >_______________________________________________ >edk2-devel mailing list >edk2-devel@lists.01.org >https://lists.01.org/mailman/listinfo/edk2-devel