Re: SCT 2.3.1 v1.3

[Laszlo Ersek <lersek@redhat.com>]

(Dropping the <utwg@uefi.org> and <uswg@uefi.org> 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 <heyi.guo@linaro.org>
>>> ---
>>>  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
>