From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 C7A1381ECE for ; Sat, 21 Jan 2017 17:21:23 -0800 (PST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP; 21 Jan 2017 17:21:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,266,1477983600"; d="scan'208";a="56722335" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga006.fm.intel.com with ESMTP; 21 Jan 2017 17:21:23 -0800 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 21 Jan 2017 17:21:23 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 21 Jan 2017 17:21:22 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.20]) by SHSMSX104.ccr.corp.intel.com ([10.239.4.70]) with mapi id 14.03.0248.002; Sun, 22 Jan 2017 09:21:20 +0800 From: "Jin, Eric" To: "Gao, Liming" , Laszlo Ersek , Daniel Samuelraj CC: Jianning Wang , "edk2-devel@lists.01.org" , Sathya Prakash , Chidambara GR , "Jin, Eric" Thread-Topic: [edk2] SCT 2.3.1 v1.3 Thread-Index: AQHScWjNvh9Mvg157U2aLd+tW2qT6KFAmBMAgAMfbcA= Date: Sun, 22 Jan 2017 01:21:20 +0000 Message-ID: References: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6D1E85@shsmsx102.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6D1E85@shsmsx102.ccr.corp.intel.com> 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: Sun, 22 Jan 2017 01:21:23 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I agree it is the gap between current UEFI SCT and UEFI Spec. Removing the checkpoint from the test is simple. But the Spec enhancement o= n these missed error description is the better choice. Best Regards Eric -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Gao,= Liming Sent: Friday, January 20, 2017 5:35 PM To: Laszlo Ersek ; Daniel Samuelraj Cc: Jianning Wang ; edk2-devel@lists.01.org; Sa= thya Prakash ; Chidambara GR Subject: Re: [edk2] SCT 2.3.1 v1.3 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=20 >Laszlo Ersek >Sent: Wednesday, January 18, 2017 4:57 PM >To: Daniel Samuelraj >Cc: Jianning Wang ;=20 >edk2-devel@lists.01.org; Sathya Prakash ;=20 >Chidambara GR >Subject: Re: [edk2] SCT 2.3.1 v1.3 > >(Dropping the and email addresses;=20 >please never cross-post the public edk2-devel list with the=20 >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=20 >> align with UEFI spec? For example, for extract config, when Progress=20 >> or Result or Request is NULL, SCT is expecting EFI Invalid Parameter;=20 >> similarly for Route Config, when progress is NULL, SCT expects=20 >> EFI_INVALID_PARAMETER. UEFI spec doesn\u2019t seem mention anything=20 >> for these cases. >> >> >> >> Should driver adhere to what SCT expects? Or is this fixed in newer=20 >> 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=20 >Linaro submitted patches for OVMF's PlatformDxe, some of which, for=20 >example > > [PATCH 2/3] OvmfPkg: PlatformDxe: Add sanity check for=20 > 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=20 >reference (released UEFI spec version, or pending Mantis ticket) that=20 >justified the SCT's expectations. > >... I would like to provide you with a mailing list archive link, but=20 >that discussion was apparently only captured in the old GMANE archive,=20 >and GMANE went belly-up a few months ago. Albeit being resuscitated,=20 >the edk2-devel messages seem to be lost from it for good (or at least=20 >cannot be looked up based on Message-Id). > >For lack of a better means, I'll quote one message from that thread=20 >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=20 >>>> passed in with NULL and RouteConfig will try to access the string=20 >>>> at *(EFI_STRING *0), i.e. 0xFFFFFFFF14000400. >>>> >>>> Add sanity check for ExtractConfig and RouteConfig to avoid NULL=20 >>>> 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=20 >>> in NULL (or non-NULL garbage, for that matter, which you couldn't=20 >>> catch anyway). >>> >>> I can ACK this hunk if you show me a confirmed Mantis ticket for the=20 >>> UEFI spec. >> >> Sorry, I just noticed that above I referenced=20 >> 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.= =20 >>> But, please update the leading comment on the function accordingly=20 >>> -- please document the EFI_INVALID_PARAMETER retval. >>> >>> The (Progress =3D=3D NULL) check is not required by the spec, and the=20 >>> 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=20 >>> 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=20 >>> 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=20 >> 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel