Re: SCT 2.3.1 v1.3

Re: SCT 2.3.1 v1.3

[Daniel Samuelraj]

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’t 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!



Thanks,

Daniel

Re: SCT 2.3.1 v1.3

[Laszlo Ersek]

(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
>

Re: SCT 2.3.1 v1.3

[Gao, Liming]

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

Re: SCT 2.3.1 v1.3

[Jin, Eric]

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 on 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 <lersek@redhat.com>; 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

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel