From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 17C9C81E10 for ; Wed, 18 Jan 2017 00:56:33 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3771981222; Wed, 18 Jan 2017 08:56:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-38.phx2.redhat.com [10.3.116.38]) by smtp.corp.redhat.com (Postfix) with ESMTP id BCD9C84D10; Wed, 18 Jan 2017 08:56:32 +0000 (UTC) To: Daniel Samuelraj References: Cc: edk2-devel@lists.01.org, Jianning Wang , Sathya Prakash , Chidambara GR From: Laszlo Ersek Message-ID: Date: Wed, 18 Jan 2017 09:56:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.74 on 10.5.11.28 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 18 Jan 2017 08:56:34 +0000 (UTC) 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: Wed, 18 Jan 2017 08:56:33 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit (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 == NULL || Results == NULL) >>> + { >>> + return EFI_INVALID_PARAMETER; >>> + } >>> + >>> DEBUG ((EFI_D_VERBOSE, "%a: Request=\"%s\"\n", __FUNCTION__, Request)); >>> >>> Status = 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 == NULL || Progress == NULL) >>> + { >>> + return EFI_INVALID_PARAMETER; >>> + } >>> + >>> DEBUG ((EFI_D_VERBOSE, "%a: Configuration=\"%s\"\n", __FUNCTION__, >>> Configuration)); >>> >>> >> >> The (Configuration == 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 == 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==NULL) 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 will. > > Thanks > Laszlo > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel >