Laszlo: I agree this is gap between SCT and UEFI spec. I think UEFI spec can be updated to describe these error conditions.
Thanks Liming >-----Original Message----- >From: edk2-devel [mailto:[email protected]] On Behalf Of >Laszlo Ersek >Sent: Wednesday, January 18, 2017 4:57 PM >To: Daniel Samuelraj <[email protected]> >Cc: Jianning Wang <[email protected]>; [email protected]; >Sathya Prakash <[email protected]>; Chidambara GR ><[email protected]> >Subject: Re: [edk2] SCT 2.3.1 v1.3 > >(Dropping the <[email protected]> and <[email protected]> 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 <[email protected]> >>>> --- >>>> 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 >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >_______________________________________________ >edk2-devel mailing list >[email protected] >https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
