Re: SCT 2.3.1 v1.3

["Gao, Liming" <liming.gao@intel.com>]

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:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Wednesday, January 18, 2017 4:57 PM
>To: Daniel Samuelraj <daniel.samuelraj@broadcom.com>
>Cc: Jianning Wang <jianning.wang@broadcom.com>; edk2-devel@lists.01.org;
>Sathya Prakash <sathya.prakash@broadcom.com>; Chidambara GR
><chidambara.gr@broadcom.com>
>Subject: Re: [edk2] SCT 2.3.1 v1.3
>
>(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
>>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel