Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
* Re: fwts: RuntimeServicesSupported variable
       [not found] <4898db16-7f9b-2efc-a5ae-356461d790b8@gmx.de>
@ 2020-10-14 17:45 ` Heinrich Schuchardt
  2020-10-14 17:58   ` Ard Biesheuvel
  0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-10-14 17:45 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi

On 14.10.20 19:31, Heinrich Schuchardt wrote:
> Dear all,
>
> the fwts fails on U-Boot due to testing for a non-existent
> RuntimeServicesSupported variable.
>
> If you look at the UEFI specification 2.8 (Errata B) [1] you will
> discover in the change log:
>
> 2.8 A2049
> RuntimeServicesSupported EFI variable should be a config table
> February 2020
>
> Please, read the configuration table to determine if a runtime service
> is available on UEFI 2.8 systems.
>
> On lower UEFI firmware version neither the variable nor the table exists.
>
> Best regards
>
> Heinrich
>
> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>

Hello Ard,

what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
the efi_test driver?

Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
function is not marked as supported in the table? Or will the
configuration table itself be make available?

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-14 17:45 ` fwts: RuntimeServicesSupported variable Heinrich Schuchardt
@ 2020-10-14 17:58   ` Ard Biesheuvel
  2020-10-14 18:41     ` Heinrich Schuchardt
  0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-10-14 17:58 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi

On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 14.10.20 19:31, Heinrich Schuchardt wrote:
> > Dear all,
> >
> > the fwts fails on U-Boot due to testing for a non-existent
> > RuntimeServicesSupported variable.
> >
> > If you look at the UEFI specification 2.8 (Errata B) [1] you will
> > discover in the change log:
> >
> > 2.8 A2049
> > RuntimeServicesSupported EFI variable should be a config table
> > February 2020
> >
> > Please, read the configuration table to determine if a runtime service
> > is available on UEFI 2.8 systems.
> >
> > On lower UEFI firmware version neither the variable nor the table exists.
> >
> > Best regards
> >
> > Heinrich
> >
> > [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
> > https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
> >
>
> Hello Ard,
>
> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
> the efi_test driver?
>
> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
> function is not marked as supported in the table? Or will the
> configuration table itself be make available?
>

The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
runtime, but requires that they are marked as such in the
EFI_RT_PROPERTIES_TABLE.

So assuming that the purpose of efi_test is compliance with the spec,
it should only allow EFI_UNSUPPORTED as a return value for each of the
tested runtime services if it is omitted from
efi.runtime_supported_mask.

Since the efi_test ioctl returns both an error code and the actual EFI
status code, we should only fail the call on a EFI_UNSUPPORTED status
code if the RTPROP mask does not allow that.

E.g.,

--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
                goto out;
        }

-       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
+       if (status == EFI_SUCCESS ||
+           (status == EFI_UNSUPPORTED &&
+            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
+               rv = 0;
+       else
+               rv = -EINVAL;

 out:
        kfree(data);


Do you think that could work?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-14 17:58   ` Ard Biesheuvel
@ 2020-10-14 18:41     ` Heinrich Schuchardt
  2020-10-19  9:31       ` Ard Biesheuvel
  0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-10-14 18:41 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On 14.10.20 19:58, Ard Biesheuvel wrote:
> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
>>> Dear all,
>>>
>>> the fwts fails on U-Boot due to testing for a non-existent
>>> RuntimeServicesSupported variable.
>>>
>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
>>> discover in the change log:
>>>
>>> 2.8 A2049
>>> RuntimeServicesSupported EFI variable should be a config table
>>> February 2020
>>>
>>> Please, read the configuration table to determine if a runtime service
>>> is available on UEFI 2.8 systems.
>>>
>>> On lower UEFI firmware version neither the variable nor the table exists.
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>>>
>>
>> Hello Ard,
>>
>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
>> the efi_test driver?
>>
>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
>> function is not marked as supported in the table? Or will the
>> configuration table itself be make available?
>>
>
> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
> runtime, but requires that they are marked as such in the
> EFI_RT_PROPERTIES_TABLE.
>
> So assuming that the purpose of efi_test is compliance with the spec,
> it should only allow EFI_UNSUPPORTED as a return value for each of the
> tested runtime services if it is omitted from
> efi.runtime_supported_mask.
>
> Since the efi_test ioctl returns both an error code and the actual EFI
> status code, we should only fail the call on a EFI_UNSUPPORTED status
> code if the RTPROP mask does not allow that.
>
> E.g.,
>
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>                 goto out;
>         }
>
> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
> +       if (status == EFI_SUCCESS ||
> +           (status == EFI_UNSUPPORTED &&
> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
> +               rv = 0;
> +       else
> +               rv = -EINVAL;
>
>  out:
>         kfree(data);
>
>
> Do you think that could work?
>

The current fwts implementation assumes that EFI_UNSUPPORTED leads to
ioctl() returning -1. This value should not be changed. It would be
preferable to use another error code than -EINVAL, e.g. -EDOM if there
is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
a future verision of fwts can evaluate errno to discover the problem.

Do I read you correctly: the EFI runtime wrapper does not fend of calls
to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
Directly returning an error code might help to avoid crashes on
non-compliant firmware.

Best regards

Heinrich



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-14 18:41     ` Heinrich Schuchardt
@ 2020-10-19  9:31       ` Ard Biesheuvel
  2020-10-19 10:00         ` Heinrich Schuchardt
  0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-10-19  9:31 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 14.10.20 19:58, Ard Biesheuvel wrote:
> > On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 14.10.20 19:31, Heinrich Schuchardt wrote:
> >>> Dear all,
> >>>
> >>> the fwts fails on U-Boot due to testing for a non-existent
> >>> RuntimeServicesSupported variable.
> >>>
> >>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
> >>> discover in the change log:
> >>>
> >>> 2.8 A2049
> >>> RuntimeServicesSupported EFI variable should be a config table
> >>> February 2020
> >>>
> >>> Please, read the configuration table to determine if a runtime service
> >>> is available on UEFI 2.8 systems.
> >>>
> >>> On lower UEFI firmware version neither the variable nor the table exists.
> >>>
> >>> Best regards
> >>>
> >>> Heinrich
> >>>
> >>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
> >>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
> >>>
> >>
> >> Hello Ard,
> >>
> >> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
> >> the efi_test driver?
> >>
> >> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
> >> function is not marked as supported in the table? Or will the
> >> configuration table itself be make available?
> >>
> >
> > The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
> > runtime, but requires that they are marked as such in the
> > EFI_RT_PROPERTIES_TABLE.
> >
> > So assuming that the purpose of efi_test is compliance with the spec,
> > it should only allow EFI_UNSUPPORTED as a return value for each of the
> > tested runtime services if it is omitted from
> > efi.runtime_supported_mask.
> >
> > Since the efi_test ioctl returns both an error code and the actual EFI
> > status code, we should only fail the call on a EFI_UNSUPPORTED status
> > code if the RTPROP mask does not allow that.
> >
> > E.g.,
> >
> > --- a/drivers/firmware/efi/test/efi_test.c
> > +++ b/drivers/firmware/efi/test/efi_test.c
> > @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
> >                 goto out;
> >         }
> >
> > -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
> > +       if (status == EFI_SUCCESS ||
> > +           (status == EFI_UNSUPPORTED &&
> > +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
> > +               rv = 0;
> > +       else
> > +               rv = -EINVAL;
> >
> >  out:
> >         kfree(data);
> >
> >
> > Do you think that could work?
> >
>
> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
> ioctl() returning -1. This value should not be changed. It would be
> preferable to use another error code than -EINVAL, e.g. -EDOM if there
> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
> a future verision of fwts can evaluate errno to discover the problem.
>
> Do I read you correctly: the EFI runtime wrapper does not fend of calls
> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
> Directly returning an error code might help to avoid crashes on
> non-compliant firmware.
>

It is not the kernel's job to work around non-compliant firmware. The
EFI spec is crystal clear that every runtime service needs to be
implemented, but is permitted to return EFI_UNSUPPORTED after
ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
you calling certain runtime services is disallowed, it tells you that
there is no point in even trying. That is why users such as efi-pstore
now take this information into account in their probe path (and
efivarfs will only mount read/write if SetVariable() is not marked as
unsupported).

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-19  9:31       ` Ard Biesheuvel
@ 2020-10-19 10:00         ` Heinrich Schuchardt
  2020-10-19 10:03           ` Ard Biesheuvel
  0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-10-19 10:00 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On 19.10.20 11:31, Ard Biesheuvel wrote:
> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 14.10.20 19:58, Ard Biesheuvel wrote:
>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
>>>>> Dear all,
>>>>>
>>>>> the fwts fails on U-Boot due to testing for a non-existent
>>>>> RuntimeServicesSupported variable.
>>>>>
>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
>>>>> discover in the change log:
>>>>>
>>>>> 2.8 A2049
>>>>> RuntimeServicesSupported EFI variable should be a config table
>>>>> February 2020
>>>>>
>>>>> Please, read the configuration table to determine if a runtime service
>>>>> is available on UEFI 2.8 systems.
>>>>>
>>>>> On lower UEFI firmware version neither the variable nor the table exists.
>>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>>>>>
>>>>
>>>> Hello Ard,
>>>>
>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
>>>> the efi_test driver?
>>>>
>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
>>>> function is not marked as supported in the table? Or will the
>>>> configuration table itself be make available?
>>>>
>>>
>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
>>> runtime, but requires that they are marked as such in the
>>> EFI_RT_PROPERTIES_TABLE.
>>>
>>> So assuming that the purpose of efi_test is compliance with the spec,
>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
>>> tested runtime services if it is omitted from
>>> efi.runtime_supported_mask.
>>>
>>> Since the efi_test ioctl returns both an error code and the actual EFI
>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
>>> code if the RTPROP mask does not allow that.
>>>
>>> E.g.,
>>>
>>> --- a/drivers/firmware/efi/test/efi_test.c
>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>>>                 goto out;
>>>         }
>>>
>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
>>> +       if (status == EFI_SUCCESS ||
>>> +           (status == EFI_UNSUPPORTED &&
>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
>>> +               rv = 0;
>>> +       else
>>> +               rv = -EINVAL;
>>>
>>>  out:
>>>         kfree(data);
>>>
>>>
>>> Do you think that could work?
>>>
>>
>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
>> ioctl() returning -1. This value should not be changed. It would be
>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
>> a future verision of fwts can evaluate errno to discover the problem.
>>
>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
>> Directly returning an error code might help to avoid crashes on
>> non-compliant firmware.
>>
>
> It is not the kernel's job to work around non-compliant firmware. The
> EFI spec is crystal clear that every runtime service needs to be
> implemented, but is permitted to return EFI_UNSUPPORTED after
> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
> you calling certain runtime services is disallowed, it tells you that
> there is no point in even trying. That is why users such as efi-pstore
> now take this information into account in their probe path (and
> efivarfs will only mount read/write if SetVariable() is not marked as
> unsupported).
>

How about the return code?

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-19 10:00         ` Heinrich Schuchardt
@ 2020-10-19 10:03           ` Ard Biesheuvel
  2020-10-19 11:00             ` Heinrich Schuchardt
  2020-10-19 11:22             ` Heinrich Schuchardt
  0 siblings, 2 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-10-19 10:03 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 19.10.20 11:31, Ard Biesheuvel wrote:
> > On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 14.10.20 19:58, Ard Biesheuvel wrote:
> >>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
> >>>>> Dear all,
> >>>>>
> >>>>> the fwts fails on U-Boot due to testing for a non-existent
> >>>>> RuntimeServicesSupported variable.
> >>>>>
> >>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
> >>>>> discover in the change log:
> >>>>>
> >>>>> 2.8 A2049
> >>>>> RuntimeServicesSupported EFI variable should be a config table
> >>>>> February 2020
> >>>>>
> >>>>> Please, read the configuration table to determine if a runtime service
> >>>>> is available on UEFI 2.8 systems.
> >>>>>
> >>>>> On lower UEFI firmware version neither the variable nor the table exists.
> >>>>>
> >>>>> Best regards
> >>>>>
> >>>>> Heinrich
> >>>>>
> >>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
> >>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
> >>>>>
> >>>>
> >>>> Hello Ard,
> >>>>
> >>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
> >>>> the efi_test driver?
> >>>>
> >>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
> >>>> function is not marked as supported in the table? Or will the
> >>>> configuration table itself be make available?
> >>>>
> >>>
> >>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
> >>> runtime, but requires that they are marked as such in the
> >>> EFI_RT_PROPERTIES_TABLE.
> >>>
> >>> So assuming that the purpose of efi_test is compliance with the spec,
> >>> it should only allow EFI_UNSUPPORTED as a return value for each of the
> >>> tested runtime services if it is omitted from
> >>> efi.runtime_supported_mask.
> >>>
> >>> Since the efi_test ioctl returns both an error code and the actual EFI
> >>> status code, we should only fail the call on a EFI_UNSUPPORTED status
> >>> code if the RTPROP mask does not allow that.
> >>>
> >>> E.g.,
> >>>
> >>> --- a/drivers/firmware/efi/test/efi_test.c
> >>> +++ b/drivers/firmware/efi/test/efi_test.c
> >>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
> >>>                 goto out;
> >>>         }
> >>>
> >>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
> >>> +       if (status == EFI_SUCCESS ||
> >>> +           (status == EFI_UNSUPPORTED &&
> >>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
> >>> +               rv = 0;
> >>> +       else
> >>> +               rv = -EINVAL;
> >>>
> >>>  out:
> >>>         kfree(data);
> >>>
> >>>
> >>> Do you think that could work?
> >>>
> >>
> >> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
> >> ioctl() returning -1. This value should not be changed. It would be
> >> preferable to use another error code than -EINVAL, e.g. -EDOM if there
> >> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
> >> a future verision of fwts can evaluate errno to discover the problem.
> >>
> >> Do I read you correctly: the EFI runtime wrapper does not fend of calls
> >> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
> >> Directly returning an error code might help to avoid crashes on
> >> non-compliant firmware.
> >>
> >
> > It is not the kernel's job to work around non-compliant firmware. The
> > EFI spec is crystal clear that every runtime service needs to be
> > implemented, but is permitted to return EFI_UNSUPPORTED after
> > ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
> > you calling certain runtime services is disallowed, it tells you that
> > there is no point in even trying. That is why users such as efi-pstore
> > now take this information into account in their probe path (and
> > efivarfs will only mount read/write if SetVariable() is not marked as
> > unsupported).
> >
>
> How about the return code?
>

As I attempted to explain, I think EFI_UNSUPPORTED should not be
reported as an error if RT_PROP_TABLE permits it. The caller has
access to the raw efi_status_t that was returned, so it can
distinguish between the two cases.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-19 10:03           ` Ard Biesheuvel
@ 2020-10-19 11:00             ` Heinrich Schuchardt
  2020-10-19 11:01               ` Ard Biesheuvel
  2020-10-19 11:22             ` Heinrich Schuchardt
  1 sibling, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-10-19 11:00 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On 19.10.20 12:03, Ard Biesheuvel wrote:
> On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 19.10.20 11:31, Ard Biesheuvel wrote:
>>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 14.10.20 19:58, Ard Biesheuvel wrote:
>>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
>>>>>>> Dear all,
>>>>>>>
>>>>>>> the fwts fails on U-Boot due to testing for a non-existent
>>>>>>> RuntimeServicesSupported variable.
>>>>>>>
>>>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
>>>>>>> discover in the change log:
>>>>>>>
>>>>>>> 2.8 A2049
>>>>>>> RuntimeServicesSupported EFI variable should be a config table
>>>>>>> February 2020
>>>>>>>
>>>>>>> Please, read the configuration table to determine if a runtime service
>>>>>>> is available on UEFI 2.8 systems.
>>>>>>>
>>>>>>> On lower UEFI firmware version neither the variable nor the table exists.
>>>>>>>
>>>>>>> Best regards
>>>>>>>
>>>>>>> Heinrich
>>>>>>>
>>>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
>>>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>>>>>>>
>>>>>>
>>>>>> Hello Ard,
>>>>>>
>>>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
>>>>>> the efi_test driver?
>>>>>>
>>>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
>>>>>> function is not marked as supported in the table? Or will the
>>>>>> configuration table itself be make available?
>>>>>>
>>>>>
>>>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
>>>>> runtime, but requires that they are marked as such in the
>>>>> EFI_RT_PROPERTIES_TABLE.
>>>>>
>>>>> So assuming that the purpose of efi_test is compliance with the spec,
>>>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
>>>>> tested runtime services if it is omitted from
>>>>> efi.runtime_supported_mask.
>>>>>
>>>>> Since the efi_test ioctl returns both an error code and the actual EFI
>>>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
>>>>> code if the RTPROP mask does not allow that.
>>>>>
>>>>> E.g.,
>>>>>
>>>>> --- a/drivers/firmware/efi/test/efi_test.c
>>>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>>>>>                 goto out;
>>>>>         }
>>>>>
>>>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
>>>>> +       if (status == EFI_SUCCESS ||
>>>>> +           (status == EFI_UNSUPPORTED &&
>>>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
>>>>> +               rv = 0;
>>>>> +       else
>>>>> +               rv = -EINVAL;
>>>>>
>>>>>  out:
>>>>>         kfree(data);
>>>>>
>>>>>
>>>>> Do you think that could work?
>>>>>
>>>>
>>>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
>>>> ioctl() returning -1. This value should not be changed. It would be
>>>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
>>>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
>>>> a future verision of fwts can evaluate errno to discover the problem.
>>>>
>>>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
>>>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
>>>> Directly returning an error code might help to avoid crashes on
>>>> non-compliant firmware.
>>>>
>>>
>>> It is not the kernel's job to work around non-compliant firmware. The
>>> EFI spec is crystal clear that every runtime service needs to be
>>> implemented, but is permitted to return EFI_UNSUPPORTED after
>>> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
>>> you calling certain runtime services is disallowed, it tells you that
>>> there is no point in even trying. That is why users such as efi-pstore
>>> now take this information into account in their probe path (and
>>> efivarfs will only mount read/write if SetVariable() is not marked as
>>> unsupported).
>>>
>>
>> How about the return code?
>>
>
> As I attempted to explain, I think EFI_UNSUPPORTED should not be
> reported as an error if RT_PROP_TABLE permits it. The caller has
> access to the raw efi_status_t that was returned, so it can
> distinguish between the two cases.
>

The fwts tires to figure out if a firmware implementation is compliant.

The return value according to you suggestion would be as follows
depending on the UEFI status and the entry in EFI_RT_PROPERTIES_TABLE.

          | EFI_SUCCESS  | EFI_UNSUPPORTED | EFI_INVALID_PARAMETER
----------|--------------|-----------------|----------------------
Available |              |                 |
according |     0        |   -EINVAL       |       -EINVAL
EFT_RT_PRO|              |                 |
-------------------------------------------------------------------
Not       |              |                 |
available |              |                 |
according |     0        |       0         |       -EINVAL
EFT_RT_PRO|              |                 |
-------------------------------------------------------------------

fwts would not be able to detect that according to the
EFI_RT_PROPERTIES_TABLE the service is marked as not available
but returns a value other than EFI_UNSUPPORTED.

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-19 11:00             ` Heinrich Schuchardt
@ 2020-10-19 11:01               ` Ard Biesheuvel
  2020-10-19 11:25                 ` Heinrich Schuchardt
  0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-10-19 11:01 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On Mon, 19 Oct 2020 at 13:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 19.10.20 12:03, Ard Biesheuvel wrote:
> > On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 19.10.20 11:31, Ard Biesheuvel wrote:
> >>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 14.10.20 19:58, Ard Biesheuvel wrote:
> >>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>
> >>>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
> >>>>>>> Dear all,
> >>>>>>>
> >>>>>>> the fwts fails on U-Boot due to testing for a non-existent
> >>>>>>> RuntimeServicesSupported variable.
> >>>>>>>
> >>>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
> >>>>>>> discover in the change log:
> >>>>>>>
> >>>>>>> 2.8 A2049
> >>>>>>> RuntimeServicesSupported EFI variable should be a config table
> >>>>>>> February 2020
> >>>>>>>
> >>>>>>> Please, read the configuration table to determine if a runtime service
> >>>>>>> is available on UEFI 2.8 systems.
> >>>>>>>
> >>>>>>> On lower UEFI firmware version neither the variable nor the table exists.
> >>>>>>>
> >>>>>>> Best regards
> >>>>>>>
> >>>>>>> Heinrich
> >>>>>>>
> >>>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
> >>>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
> >>>>>>>
> >>>>>>
> >>>>>> Hello Ard,
> >>>>>>
> >>>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
> >>>>>> the efi_test driver?
> >>>>>>
> >>>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
> >>>>>> function is not marked as supported in the table? Or will the
> >>>>>> configuration table itself be make available?
> >>>>>>
> >>>>>
> >>>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
> >>>>> runtime, but requires that they are marked as such in the
> >>>>> EFI_RT_PROPERTIES_TABLE.
> >>>>>
> >>>>> So assuming that the purpose of efi_test is compliance with the spec,
> >>>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
> >>>>> tested runtime services if it is omitted from
> >>>>> efi.runtime_supported_mask.
> >>>>>
> >>>>> Since the efi_test ioctl returns both an error code and the actual EFI
> >>>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
> >>>>> code if the RTPROP mask does not allow that.
> >>>>>
> >>>>> E.g.,
> >>>>>
> >>>>> --- a/drivers/firmware/efi/test/efi_test.c
> >>>>> +++ b/drivers/firmware/efi/test/efi_test.c
> >>>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
> >>>>>                 goto out;
> >>>>>         }
> >>>>>
> >>>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
> >>>>> +       if (status == EFI_SUCCESS ||
> >>>>> +           (status == EFI_UNSUPPORTED &&
> >>>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
> >>>>> +               rv = 0;
> >>>>> +       else
> >>>>> +               rv = -EINVAL;
> >>>>>
> >>>>>  out:
> >>>>>         kfree(data);
> >>>>>
> >>>>>
> >>>>> Do you think that could work?
> >>>>>
> >>>>
> >>>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
> >>>> ioctl() returning -1. This value should not be changed. It would be
> >>>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
> >>>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
> >>>> a future verision of fwts can evaluate errno to discover the problem.
> >>>>
> >>>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
> >>>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
> >>>> Directly returning an error code might help to avoid crashes on
> >>>> non-compliant firmware.
> >>>>
> >>>
> >>> It is not the kernel's job to work around non-compliant firmware. The
> >>> EFI spec is crystal clear that every runtime service needs to be
> >>> implemented, but is permitted to return EFI_UNSUPPORTED after
> >>> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
> >>> you calling certain runtime services is disallowed, it tells you that
> >>> there is no point in even trying. That is why users such as efi-pstore
> >>> now take this information into account in their probe path (and
> >>> efivarfs will only mount read/write if SetVariable() is not marked as
> >>> unsupported).
> >>>
> >>
> >> How about the return code?
> >>
> >
> > As I attempted to explain, I think EFI_UNSUPPORTED should not be
> > reported as an error if RT_PROP_TABLE permits it. The caller has
> > access to the raw efi_status_t that was returned, so it can
> > distinguish between the two cases.
> >
>
> The fwts tires to figure out if a firmware implementation is compliant.
>
> The return value according to you suggestion would be as follows
> depending on the UEFI status and the entry in EFI_RT_PROPERTIES_TABLE.
>
>           | EFI_SUCCESS  | EFI_UNSUPPORTED | EFI_INVALID_PARAMETER
> ----------|--------------|-----------------|----------------------
> Available |              |                 |
> according |     0        |   -EINVAL       |       -EINVAL
> EFT_RT_PRO|              |                 |
> -------------------------------------------------------------------
> Not       |              |                 |
> available |              |                 |
> according |     0        |       0         |       -EINVAL
> EFT_RT_PRO|              |                 |
> -------------------------------------------------------------------
>
> fwts would not be able to detect that according to the
> EFI_RT_PROPERTIES_TABLE the service is marked as not available
> but returns a value other than EFI_UNSUPPORTED.
>

But that would be permitted by the spec anyway. A runtime service is
not required to always return EFI_UNSUPPORTED if it is marked as
unavaialble in EFI_RT_PROP.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-19 10:03           ` Ard Biesheuvel
  2020-10-19 11:00             ` Heinrich Schuchardt
@ 2020-10-19 11:22             ` Heinrich Schuchardt
  1 sibling, 0 replies; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-10-19 11:22 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On 19.10.20 12:03, Ard Biesheuvel wrote:
> On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 19.10.20 11:31, Ard Biesheuvel wrote:
>>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 14.10.20 19:58, Ard Biesheuvel wrote:
>>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
>>>>>>> Dear all,
>>>>>>>
>>>>>>> the fwts fails on U-Boot due to testing for a non-existent
>>>>>>> RuntimeServicesSupported variable.
>>>>>>>
>>>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
>>>>>>> discover in the change log:
>>>>>>>
>>>>>>> 2.8 A2049
>>>>>>> RuntimeServicesSupported EFI variable should be a config table
>>>>>>> February 2020
>>>>>>>
>>>>>>> Please, read the configuration table to determine if a runtime service
>>>>>>> is available on UEFI 2.8 systems.
>>>>>>>
>>>>>>> On lower UEFI firmware version neither the variable nor the table exists.
>>>>>>>
>>>>>>> Best regards
>>>>>>>
>>>>>>> Heinrich
>>>>>>>
>>>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
>>>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>>>>>>>
>>>>>>
>>>>>> Hello Ard,
>>>>>>
>>>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
>>>>>> the efi_test driver?
>>>>>>
>>>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
>>>>>> function is not marked as supported in the table? Or will the
>>>>>> configuration table itself be make available?
>>>>>>
>>>>>
>>>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
>>>>> runtime, but requires that they are marked as such in the
>>>>> EFI_RT_PROPERTIES_TABLE.
>>>>>
>>>>> So assuming that the purpose of efi_test is compliance with the spec,
>>>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
>>>>> tested runtime services if it is omitted from
>>>>> efi.runtime_supported_mask.
>>>>>
>>>>> Since the efi_test ioctl returns both an error code and the actual EFI
>>>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
>>>>> code if the RTPROP mask does not allow that.
>>>>>
>>>>> E.g.,
>>>>>
>>>>> --- a/drivers/firmware/efi/test/efi_test.c
>>>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>>>>>                 goto out;
>>>>>         }
>>>>>
>>>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
>>>>> +       if (status == EFI_SUCCESS ||
>>>>> +           (status == EFI_UNSUPPORTED &&
>>>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
>>>>> +               rv = 0;
>>>>> +       else
>>>>> +               rv = -EINVAL;
>>>>>
>>>>>  out:
>>>>>         kfree(data);
>>>>>
>>>>>
>>>>> Do you think that could work?
>>>>>
>>>>
>>>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
>>>> ioctl() returning -1. This value should not be changed. It would be
>>>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
>>>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
>>>> a future verision of fwts can evaluate errno to discover the problem.
>>>>
>>>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
>>>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
>>>> Directly returning an error code might help to avoid crashes on
>>>> non-compliant firmware.
>>>>
>>>
>>> It is not the kernel's job to work around non-compliant firmware. The
>>> EFI spec is crystal clear that every runtime service needs to be
>>> implemented, but is permitted to return EFI_UNSUPPORTED after
>>> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
>>> you calling certain runtime services is disallowed, it tells you that
>>> there is no point in even trying. That is why users such as efi-pstore
>>> now take this information into account in their probe path (and
>>> efivarfs will only mount read/write if SetVariable() is not marked as
>>> unsupported).
>>>
>>
>> How about the return code?
>>
>
> As I attempted to explain, I think EFI_UNSUPPORTED should not be
> reported as an error if RT_PROP_TABLE permits it. The caller has
> access to the raw efi_status_t that was returned, so it can
> distinguish between the two cases.
>

In the chapter "EFI_RT _PROPERTIES_TABLE" you can find this description:

*RuntimeServicesSupported* mask of which calls are or are not supported,
where a bit set to 1 indicates that the call is supported, and 0
indicates that it is not.

This leaves no room for implementing a service that is marked as not
supported.

In the descriptions of the return codes of the individual runtime services:

"*EFI_UNSUPPORTED* This call is not supported by this platform at the
time the call is made. The platform should describe this runtime service
as unsupported at runtime via an EFI_RT_PROPERTIES_TABLE configuration
table."

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-19 11:01               ` Ard Biesheuvel
@ 2020-10-19 11:25                 ` Heinrich Schuchardt
  2020-10-20  6:20                   ` ivanhu
  0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-10-19 11:25 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Alex Hung, Colin Ian King, Ivan Hu, linux-efi, fwts-devel

On 19.10.20 13:01, Ard Biesheuvel wrote:
> On Mon, 19 Oct 2020 at 13:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 19.10.20 12:03, Ard Biesheuvel wrote:
>>> On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 19.10.20 11:31, Ard Biesheuvel wrote:
>>>>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 14.10.20 19:58, Ard Biesheuvel wrote:
>>>>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>
>>>>>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
>>>>>>>>> Dear all,
>>>>>>>>>
>>>>>>>>> the fwts fails on U-Boot due to testing for a non-existent
>>>>>>>>> RuntimeServicesSupported variable.
>>>>>>>>>
>>>>>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
>>>>>>>>> discover in the change log:
>>>>>>>>>
>>>>>>>>> 2.8 A2049
>>>>>>>>> RuntimeServicesSupported EFI variable should be a config table
>>>>>>>>> February 2020
>>>>>>>>>
>>>>>>>>> Please, read the configuration table to determine if a runtime service
>>>>>>>>> is available on UEFI 2.8 systems.
>>>>>>>>>
>>>>>>>>> On lower UEFI firmware version neither the variable nor the table exists.
>>>>>>>>>
>>>>>>>>> Best regards
>>>>>>>>>
>>>>>>>>> Heinrich
>>>>>>>>>
>>>>>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
>>>>>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hello Ard,
>>>>>>>>
>>>>>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
>>>>>>>> the efi_test driver?
>>>>>>>>
>>>>>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
>>>>>>>> function is not marked as supported in the table? Or will the
>>>>>>>> configuration table itself be make available?
>>>>>>>>
>>>>>>>
>>>>>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
>>>>>>> runtime, but requires that they are marked as such in the
>>>>>>> EFI_RT_PROPERTIES_TABLE.
>>>>>>>
>>>>>>> So assuming that the purpose of efi_test is compliance with the spec,
>>>>>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
>>>>>>> tested runtime services if it is omitted from
>>>>>>> efi.runtime_supported_mask.
>>>>>>>
>>>>>>> Since the efi_test ioctl returns both an error code and the actual EFI
>>>>>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
>>>>>>> code if the RTPROP mask does not allow that.
>>>>>>>
>>>>>>> E.g.,
>>>>>>>
>>>>>>> --- a/drivers/firmware/efi/test/efi_test.c
>>>>>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>>>>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>>>>>>>                 goto out;
>>>>>>>         }
>>>>>>>
>>>>>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
>>>>>>> +       if (status == EFI_SUCCESS ||
>>>>>>> +           (status == EFI_UNSUPPORTED &&
>>>>>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
>>>>>>> +               rv = 0;
>>>>>>> +       else
>>>>>>> +               rv = -EINVAL;
>>>>>>>
>>>>>>>  out:
>>>>>>>         kfree(data);
>>>>>>>
>>>>>>>
>>>>>>> Do you think that could work?
>>>>>>>
>>>>>>
>>>>>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
>>>>>> ioctl() returning -1. This value should not be changed. It would be
>>>>>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
>>>>>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
>>>>>> a future verision of fwts can evaluate errno to discover the problem.
>>>>>>
>>>>>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
>>>>>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
>>>>>> Directly returning an error code might help to avoid crashes on
>>>>>> non-compliant firmware.
>>>>>>
>>>>>
>>>>> It is not the kernel's job to work around non-compliant firmware. The
>>>>> EFI spec is crystal clear that every runtime service needs to be
>>>>> implemented, but is permitted to return EFI_UNSUPPORTED after
>>>>> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
>>>>> you calling certain runtime services is disallowed, it tells you that
>>>>> there is no point in even trying. That is why users such as efi-pstore
>>>>> now take this information into account in their probe path (and
>>>>> efivarfs will only mount read/write if SetVariable() is not marked as
>>>>> unsupported).
>>>>>
>>>>
>>>> How about the return code?
>>>>
>>>
>>> As I attempted to explain, I think EFI_UNSUPPORTED should not be
>>> reported as an error if RT_PROP_TABLE permits it. The caller has
>>> access to the raw efi_status_t that was returned, so it can
>>> distinguish between the two cases.
>>>
>>
>> The fwts tires to figure out if a firmware implementation is compliant.
>>
>> The return value according to you suggestion would be as follows
>> depending on the UEFI status and the entry in EFI_RT_PROPERTIES_TABLE.
>>
>>           | EFI_SUCCESS  | EFI_UNSUPPORTED | EFI_INVALID_PARAMETER
>> ----------|--------------|-----------------|----------------------
>> Available |              |                 |
>> according |     0        |   -EINVAL       |       -EINVAL
>> EFT_RT_PRO|              |                 |
>> -------------------------------------------------------------------
>> Not       |              |                 |
>> available |              |                 |
>> according |     0        |       0         |       -EINVAL
>> EFT_RT_PRO|              |                 |
>> -------------------------------------------------------------------
>>
>> fwts would not be able to detect that according to the
>> EFI_RT_PROPERTIES_TABLE the service is marked as not available
>> but returns a value other than EFI_UNSUPPORTED.
>>
>
> But that would be permitted by the spec anyway. A runtime service is
> not required to always return EFI_UNSUPPORTED if it is marked as
> unavaialble in EFI_RT_PROP.
>

In the chapter "EFI_RT _PROPERTIES_TABLE" you can find this description:

"*RuntimeServicesSupported* mask of which calls are or are not
supported, where a bit set to 1 indicates that the call is supported,
and 0 indicates that it is not."

This leaves no room for implementing a service that is marked as not
supported.

In the descriptions of the return codes of the individual runtime services:

"*EFI_UNSUPPORTED* This call is not supported by this platform at the
time the call is made. The platform should describe this runtime service
as unsupported at runtime via an EFI_RT_PROPERTIES_TABLE configuration
table."

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-19 11:25                 ` Heinrich Schuchardt
@ 2020-10-20  6:20                   ` ivanhu
  2020-10-20  6:46                     ` Ard Biesheuvel
  0 siblings, 1 reply; 13+ messages in thread
From: ivanhu @ 2020-10-20  6:20 UTC (permalink / raw)
  To: Heinrich Schuchardt, Ard Biesheuvel
  Cc: Alex Hung, Colin Ian King, linux-efi, fwts-devel


On 10/19/20 7:25 PM, Heinrich Schuchardt wrote:
> On 19.10.20 13:01, Ard Biesheuvel wrote:
>> On Mon, 19 Oct 2020 at 13:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 19.10.20 12:03, Ard Biesheuvel wrote:
>>>> On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> On 19.10.20 11:31, Ard Biesheuvel wrote:
>>>>>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> On 14.10.20 19:58, Ard Biesheuvel wrote:
>>>>>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
>>>>>>>>>> Dear all,
>>>>>>>>>>
>>>>>>>>>> the fwts fails on U-Boot due to testing for a non-existent
>>>>>>>>>> RuntimeServicesSupported variable.
>>>>>>>>>>
>>>>>>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
>>>>>>>>>> discover in the change log:
>>>>>>>>>>
>>>>>>>>>> 2.8 A2049
>>>>>>>>>> RuntimeServicesSupported EFI variable should be a config table
>>>>>>>>>> February 2020
>>>>>>>>>>
>>>>>>>>>> Please, read the configuration table to determine if a runtime service
>>>>>>>>>> is available on UEFI 2.8 systems.
>>>>>>>>>>
>>>>>>>>>> On lower UEFI firmware version neither the variable nor the table exists.
>>>>>>>>>>
>>>>>>>>>> Best regards
>>>>>>>>>>
>>>>>>>>>> Heinrich
>>>>>>>>>>
>>>>>>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
>>>>>>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>>>>>>>>>>
>>>>>>>>> Hello Ard,
>>>>>>>>>
>>>>>>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
>>>>>>>>> the efi_test driver?
>>>>>>>>>
>>>>>>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
>>>>>>>>> function is not marked as supported in the table? Or will the
>>>>>>>>> configuration table itself be make available?
>>>>>>>>>
>>>>>>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
>>>>>>>> runtime, but requires that they are marked as such in the
>>>>>>>> EFI_RT_PROPERTIES_TABLE.
>>>>>>>>
>>>>>>>> So assuming that the purpose of efi_test is compliance with the spec,
>>>>>>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
>>>>>>>> tested runtime services if it is omitted from
>>>>>>>> efi.runtime_supported_mask.
>>>>>>>>
>>>>>>>> Since the efi_test ioctl returns both an error code and the actual EFI
>>>>>>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
>>>>>>>> code if the RTPROP mask does not allow that.
>>>>>>>>
>>>>>>>> E.g.,
>>>>>>>>
>>>>>>>> --- a/drivers/firmware/efi/test/efi_test.c
>>>>>>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>>>>>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>>>>>>>>                 goto out;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
>>>>>>>> +       if (status == EFI_SUCCESS ||
>>>>>>>> +           (status == EFI_UNSUPPORTED &&
>>>>>>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
>>>>>>>> +               rv = 0;
>>>>>>>> +       else
>>>>>>>> +               rv = -EINVAL;
>>>>>>>>
>>>>>>>>  out:
>>>>>>>>         kfree(data);
>>>>>>>>
>>>>>>>>
>>>>>>>> Do you think that could work?
>>>>>>>>
>>>>>>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
>>>>>>> ioctl() returning -1. This value should not be changed. It would be
>>>>>>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
>>>>>>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
>>>>>>> a future verision of fwts can evaluate errno to discover the problem.
>>>>>>>
>>>>>>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
>>>>>>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
>>>>>>> Directly returning an error code might help to avoid crashes on
>>>>>>> non-compliant firmware.
>>>>>>>
>>>>>> It is not the kernel's job to work around non-compliant firmware. The
>>>>>> EFI spec is crystal clear that every runtime service needs to be
>>>>>> implemented, but is permitted to return EFI_UNSUPPORTED after
>>>>>> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
>>>>>> you calling certain runtime services is disallowed, it tells you that
>>>>>> there is no point in even trying. That is why users such as efi-pstore
>>>>>> now take this information into account in their probe path (and
>>>>>> efivarfs will only mount read/write if SetVariable() is not marked as
>>>>>> unsupported).
>>>>>>
>>>>> How about the return code?
>>>>>
>>>> As I attempted to explain, I think EFI_UNSUPPORTED should not be
>>>> reported as an error if RT_PROP_TABLE permits it. The caller has
>>>> access to the raw efi_status_t that was returned, so it can
>>>> distinguish between the two cases.
>>>>
>>> The fwts tires to figure out if a firmware implementation is compliant.
>>>
>>> The return value according to you suggestion would be as follows
>>> depending on the UEFI status and the entry in EFI_RT_PROPERTIES_TABLE.
>>>
>>>           | EFI_SUCCESS  | EFI_UNSUPPORTED | EFI_INVALID_PARAMETER
>>> ----------|--------------|-----------------|----------------------
>>> Available |              |                 |
>>> according |     0        |   -EINVAL       |       -EINVAL
>>> EFT_RT_PRO|              |                 |
>>> -------------------------------------------------------------------
>>> Not       |              |                 |
>>> available |              |                 |
>>> according |     0        |       0         |       -EINVAL
>>> EFT_RT_PRO|              |                 |
>>> -------------------------------------------------------------------
>>>
>>> fwts would not be able to detect that according to the
>>> EFI_RT_PROPERTIES_TABLE the service is marked as not available
>>> but returns a value other than EFI_UNSUPPORTED.
>>>
>> But that would be permitted by the spec anyway. A runtime service is
>> not required to always return EFI_UNSUPPORTED if it is marked as
>> unavaialble in EFI_RT_PROP.
>>
> In the chapter "EFI_RT _PROPERTIES_TABLE" you can find this description:
>
> "*RuntimeServicesSupported* mask of which calls are or are not
> supported, where a bit set to 1 indicates that the call is supported,
> and 0 indicates that it is not."
>
> This leaves no room for implementing a service that is marked as not
> supported.
>
> In the descriptions of the return codes of the individual runtime services:
>
> "*EFI_UNSUPPORTED* This call is not supported by this platform at the
> time the call is made. The platform should describe this runtime service
> as unsupported at runtime via an EFI_RT_PROPERTIES_TABLE configuration
> table."

From the spec, it clearly describes

If a platform cannot support calls defined in EFI_RUNTIME_SERVICES after
ExitBootServices() is called, that platform is permitted to provide
implementations of those runtime services that return EFI_UNSUPPORTED
when invoked at runtime. On such systems, an EFI_RT_PROPERTIES_TABLE
configuration table should be published describing which runtime
services are supported at runtime.

I think it's better not to modify efi_test base on the
EFI_RT_PROPERTIES_TABLE or RuntimeServicesSupported, let efi_test be
simply ioctl and FWTS tests can do the modifications.


Cheers,

Ivan

>
> Best regards
>
> Heinrich
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-20  6:20                   ` ivanhu
@ 2020-10-20  6:46                     ` Ard Biesheuvel
  2020-10-20  7:22                       ` ivanhu
  0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-10-20  6:46 UTC (permalink / raw)
  To: ivanhu
  Cc: Heinrich Schuchardt, Alex Hung, Colin Ian King, linux-efi, fwts-devel

On Tue, 20 Oct 2020 at 08:20, ivanhu <ivan.hu@canonical.com> wrote:
>
>
> On 10/19/20 7:25 PM, Heinrich Schuchardt wrote:
> > On 19.10.20 13:01, Ard Biesheuvel wrote:
> >> On Mon, 19 Oct 2020 at 13:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>> On 19.10.20 12:03, Ard Biesheuvel wrote:
> >>>> On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>> On 19.10.20 11:31, Ard Biesheuvel wrote:
> >>>>>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>> On 14.10.20 19:58, Ard Biesheuvel wrote:
> >>>>>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
> >>>>>>>>>> Dear all,
> >>>>>>>>>>
> >>>>>>>>>> the fwts fails on U-Boot due to testing for a non-existent
> >>>>>>>>>> RuntimeServicesSupported variable.
> >>>>>>>>>>
> >>>>>>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
> >>>>>>>>>> discover in the change log:
> >>>>>>>>>>
> >>>>>>>>>> 2.8 A2049
> >>>>>>>>>> RuntimeServicesSupported EFI variable should be a config table
> >>>>>>>>>> February 2020
> >>>>>>>>>>
> >>>>>>>>>> Please, read the configuration table to determine if a runtime service
> >>>>>>>>>> is available on UEFI 2.8 systems.
> >>>>>>>>>>
> >>>>>>>>>> On lower UEFI firmware version neither the variable nor the table exists.
> >>>>>>>>>>
> >>>>>>>>>> Best regards
> >>>>>>>>>>
> >>>>>>>>>> Heinrich
> >>>>>>>>>>
> >>>>>>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
> >>>>>>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
> >>>>>>>>>>
> >>>>>>>>> Hello Ard,
> >>>>>>>>>
> >>>>>>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
> >>>>>>>>> the efi_test driver?
> >>>>>>>>>
> >>>>>>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
> >>>>>>>>> function is not marked as supported in the table? Or will the
> >>>>>>>>> configuration table itself be make available?
> >>>>>>>>>
> >>>>>>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
> >>>>>>>> runtime, but requires that they are marked as such in the
> >>>>>>>> EFI_RT_PROPERTIES_TABLE.
> >>>>>>>>
> >>>>>>>> So assuming that the purpose of efi_test is compliance with the spec,
> >>>>>>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
> >>>>>>>> tested runtime services if it is omitted from
> >>>>>>>> efi.runtime_supported_mask.
> >>>>>>>>
> >>>>>>>> Since the efi_test ioctl returns both an error code and the actual EFI
> >>>>>>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
> >>>>>>>> code if the RTPROP mask does not allow that.
> >>>>>>>>
> >>>>>>>> E.g.,
> >>>>>>>>
> >>>>>>>> --- a/drivers/firmware/efi/test/efi_test.c
> >>>>>>>> +++ b/drivers/firmware/efi/test/efi_test.c
> >>>>>>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
> >>>>>>>>                 goto out;
> >>>>>>>>         }
> >>>>>>>>
> >>>>>>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
> >>>>>>>> +       if (status == EFI_SUCCESS ||
> >>>>>>>> +           (status == EFI_UNSUPPORTED &&
> >>>>>>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
> >>>>>>>> +               rv = 0;
> >>>>>>>> +       else
> >>>>>>>> +               rv = -EINVAL;
> >>>>>>>>
> >>>>>>>>  out:
> >>>>>>>>         kfree(data);
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Do you think that could work?
> >>>>>>>>
> >>>>>>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
> >>>>>>> ioctl() returning -1. This value should not be changed. It would be
> >>>>>>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
> >>>>>>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
> >>>>>>> a future verision of fwts can evaluate errno to discover the problem.
> >>>>>>>
> >>>>>>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
> >>>>>>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
> >>>>>>> Directly returning an error code might help to avoid crashes on
> >>>>>>> non-compliant firmware.
> >>>>>>>
> >>>>>> It is not the kernel's job to work around non-compliant firmware. The
> >>>>>> EFI spec is crystal clear that every runtime service needs to be
> >>>>>> implemented, but is permitted to return EFI_UNSUPPORTED after
> >>>>>> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
> >>>>>> you calling certain runtime services is disallowed, it tells you that
> >>>>>> there is no point in even trying. That is why users such as efi-pstore
> >>>>>> now take this information into account in their probe path (and
> >>>>>> efivarfs will only mount read/write if SetVariable() is not marked as
> >>>>>> unsupported).
> >>>>>>
> >>>>> How about the return code?
> >>>>>
> >>>> As I attempted to explain, I think EFI_UNSUPPORTED should not be
> >>>> reported as an error if RT_PROP_TABLE permits it. The caller has
> >>>> access to the raw efi_status_t that was returned, so it can
> >>>> distinguish between the two cases.
> >>>>
> >>> The fwts tires to figure out if a firmware implementation is compliant.
> >>>
> >>> The return value according to you suggestion would be as follows
> >>> depending on the UEFI status and the entry in EFI_RT_PROPERTIES_TABLE.
> >>>
> >>>           | EFI_SUCCESS  | EFI_UNSUPPORTED | EFI_INVALID_PARAMETER
> >>> ----------|--------------|-----------------|----------------------
> >>> Available |              |                 |
> >>> according |     0        |   -EINVAL       |       -EINVAL
> >>> EFT_RT_PRO|              |                 |
> >>> -------------------------------------------------------------------
> >>> Not       |              |                 |
> >>> available |              |                 |
> >>> according |     0        |       0         |       -EINVAL
> >>> EFT_RT_PRO|              |                 |
> >>> -------------------------------------------------------------------
> >>>
> >>> fwts would not be able to detect that according to the
> >>> EFI_RT_PROPERTIES_TABLE the service is marked as not available
> >>> but returns a value other than EFI_UNSUPPORTED.
> >>>
> >> But that would be permitted by the spec anyway. A runtime service is
> >> not required to always return EFI_UNSUPPORTED if it is marked as
> >> unavaialble in EFI_RT_PROP.
> >>
> > In the chapter "EFI_RT _PROPERTIES_TABLE" you can find this description:
> >
> > "*RuntimeServicesSupported* mask of which calls are or are not
> > supported, where a bit set to 1 indicates that the call is supported,
> > and 0 indicates that it is not."
> >
> > This leaves no room for implementing a service that is marked as not
> > supported.
> >
> > In the descriptions of the return codes of the individual runtime services:
> >
> > "*EFI_UNSUPPORTED* This call is not supported by this platform at the
> > time the call is made. The platform should describe this runtime service
> > as unsupported at runtime via an EFI_RT_PROPERTIES_TABLE configuration
> > table."
>
> From the spec, it clearly describes
>
> If a platform cannot support calls defined in EFI_RUNTIME_SERVICES after
> ExitBootServices() is called, that platform is permitted to provide
> implementations of those runtime services that return EFI_UNSUPPORTED
> when invoked at runtime. On such systems, an EFI_RT_PROPERTIES_TABLE
> configuration table should be published describing which runtime
> services are supported at runtime.
>
> I think it's better not to modify efi_test base on the
> EFI_RT_PROPERTIES_TABLE or RuntimeServicesSupported, let efi_test be
> simply ioctl and FWTS tests can do the modifications.
>

Doesn't that mean FTWS would need to be able to access the
EFI_RT_PROPERTIES_TABLE?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: fwts: RuntimeServicesSupported variable
  2020-10-20  6:46                     ` Ard Biesheuvel
@ 2020-10-20  7:22                       ` ivanhu
  0 siblings, 0 replies; 13+ messages in thread
From: ivanhu @ 2020-10-20  7:22 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Heinrich Schuchardt, Alex Hung, Colin Ian King, linux-efi, fwts-devel


On 10/20/20 2:46 PM, Ard Biesheuvel wrote:
> On Tue, 20 Oct 2020 at 08:20, ivanhu <ivan.hu@canonical.com> wrote:
>>
>> On 10/19/20 7:25 PM, Heinrich Schuchardt wrote:
>>> On 19.10.20 13:01, Ard Biesheuvel wrote:
>>>> On Mon, 19 Oct 2020 at 13:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> On 19.10.20 12:03, Ard Biesheuvel wrote:
>>>>>> On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> On 19.10.20 11:31, Ard Biesheuvel wrote:
>>>>>>>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>> On 14.10.20 19:58, Ard Biesheuvel wrote:
>>>>>>>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>>>> On 14.10.20 19:31, Heinrich Schuchardt wrote:
>>>>>>>>>>>> Dear all,
>>>>>>>>>>>>
>>>>>>>>>>>> the fwts fails on U-Boot due to testing for a non-existent
>>>>>>>>>>>> RuntimeServicesSupported variable.
>>>>>>>>>>>>
>>>>>>>>>>>> If you look at the UEFI specification 2.8 (Errata B) [1] you will
>>>>>>>>>>>> discover in the change log:
>>>>>>>>>>>>
>>>>>>>>>>>> 2.8 A2049
>>>>>>>>>>>> RuntimeServicesSupported EFI variable should be a config table
>>>>>>>>>>>> February 2020
>>>>>>>>>>>>
>>>>>>>>>>>> Please, read the configuration table to determine if a runtime service
>>>>>>>>>>>> is available on UEFI 2.8 systems.
>>>>>>>>>>>>
>>>>>>>>>>>> On lower UEFI firmware version neither the variable nor the table exists.
>>>>>>>>>>>>
>>>>>>>>>>>> Best regards
>>>>>>>>>>>>
>>>>>>>>>>>> Heinrich
>>>>>>>>>>>>
>>>>>>>>>>>> [1] UEFI Specification Version 2.8 (Errata B) (released June 2020),
>>>>>>>>>>>> https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf
>>>>>>>>>>>>
>>>>>>>>>>> Hello Ard,
>>>>>>>>>>>
>>>>>>>>>>> what is your idea how the EFI_RT_PROPERTIES_TABLE shall be exposed to
>>>>>>>>>>> the efi_test driver?
>>>>>>>>>>>
>>>>>>>>>>> Will the EFI runtime wrapper simply return EFI_UNSUPPORTED if the
>>>>>>>>>>> function is not marked as supported in the table? Or will the
>>>>>>>>>>> configuration table itself be make available?
>>>>>>>>>>>
>>>>>>>>>> The UEFI spec permits that runtime services return EFI_UNSUPPORTED at
>>>>>>>>>> runtime, but requires that they are marked as such in the
>>>>>>>>>> EFI_RT_PROPERTIES_TABLE.
>>>>>>>>>>
>>>>>>>>>> So assuming that the purpose of efi_test is compliance with the spec,
>>>>>>>>>> it should only allow EFI_UNSUPPORTED as a return value for each of the
>>>>>>>>>> tested runtime services if it is omitted from
>>>>>>>>>> efi.runtime_supported_mask.
>>>>>>>>>>
>>>>>>>>>> Since the efi_test ioctl returns both an error code and the actual EFI
>>>>>>>>>> status code, we should only fail the call on a EFI_UNSUPPORTED status
>>>>>>>>>> code if the RTPROP mask does not allow that.
>>>>>>>>>>
>>>>>>>>>> E.g.,
>>>>>>>>>>
>>>>>>>>>> --- a/drivers/firmware/efi/test/efi_test.c
>>>>>>>>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>>>>>>>>> @@ -265,7 +265,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>>>>>>>>>>                 goto out;
>>>>>>>>>>         }
>>>>>>>>>>
>>>>>>>>>> -       rv = status == EFI_SUCCESS ? 0 : -EINVAL;
>>>>>>>>>> +       if (status == EFI_SUCCESS ||
>>>>>>>>>> +           (status == EFI_UNSUPPORTED &&
>>>>>>>>>> +            !efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)))
>>>>>>>>>> +               rv = 0;
>>>>>>>>>> +       else
>>>>>>>>>> +               rv = -EINVAL;
>>>>>>>>>>
>>>>>>>>>>  out:
>>>>>>>>>>         kfree(data);
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Do you think that could work?
>>>>>>>>>>
>>>>>>>>> The current fwts implementation assumes that EFI_UNSUPPORTED leads to
>>>>>>>>> ioctl() returning -1. This value should not be changed. It would be
>>>>>>>>> preferable to use another error code than -EINVAL, e.g. -EDOM if there
>>>>>>>>> is a mismatch with the EFI_RT_PROPERTIES_TABLE configuration table. Then
>>>>>>>>> a future verision of fwts can evaluate errno to discover the problem.
>>>>>>>>>
>>>>>>>>> Do I read you correctly: the EFI runtime wrapper does not fend of calls
>>>>>>>>> to runtime services marked as disallowed in EFI_RT_PROPERTIES_TABLE?
>>>>>>>>> Directly returning an error code might help to avoid crashes on
>>>>>>>>> non-compliant firmware.
>>>>>>>>>
>>>>>>>> It is not the kernel's job to work around non-compliant firmware. The
>>>>>>>> EFI spec is crystal clear that every runtime service needs to be
>>>>>>>> implemented, but is permitted to return EFI_UNSUPPORTED after
>>>>>>>> ExitBootServices(). This means EFI_RT_PROPERTIES_TABLE does not tell
>>>>>>>> you calling certain runtime services is disallowed, it tells you that
>>>>>>>> there is no point in even trying. That is why users such as efi-pstore
>>>>>>>> now take this information into account in their probe path (and
>>>>>>>> efivarfs will only mount read/write if SetVariable() is not marked as
>>>>>>>> unsupported).
>>>>>>>>
>>>>>>> How about the return code?
>>>>>>>
>>>>>> As I attempted to explain, I think EFI_UNSUPPORTED should not be
>>>>>> reported as an error if RT_PROP_TABLE permits it. The caller has
>>>>>> access to the raw efi_status_t that was returned, so it can
>>>>>> distinguish between the two cases.
>>>>>>
>>>>> The fwts tires to figure out if a firmware implementation is compliant.
>>>>>
>>>>> The return value according to you suggestion would be as follows
>>>>> depending on the UEFI status and the entry in EFI_RT_PROPERTIES_TABLE.
>>>>>
>>>>>           | EFI_SUCCESS  | EFI_UNSUPPORTED | EFI_INVALID_PARAMETER
>>>>> ----------|--------------|-----------------|----------------------
>>>>> Available |              |                 |
>>>>> according |     0        |   -EINVAL       |       -EINVAL
>>>>> EFT_RT_PRO|              |                 |
>>>>> -------------------------------------------------------------------
>>>>> Not       |              |                 |
>>>>> available |              |                 |
>>>>> according |     0        |       0         |       -EINVAL
>>>>> EFT_RT_PRO|              |                 |
>>>>> -------------------------------------------------------------------
>>>>>
>>>>> fwts would not be able to detect that according to the
>>>>> EFI_RT_PROPERTIES_TABLE the service is marked as not available
>>>>> but returns a value other than EFI_UNSUPPORTED.
>>>>>
>>>> But that would be permitted by the spec anyway. A runtime service is
>>>> not required to always return EFI_UNSUPPORTED if it is marked as
>>>> unavaialble in EFI_RT_PROP.
>>>>
>>> In the chapter "EFI_RT _PROPERTIES_TABLE" you can find this description:
>>>
>>> "*RuntimeServicesSupported* mask of which calls are or are not
>>> supported, where a bit set to 1 indicates that the call is supported,
>>> and 0 indicates that it is not."
>>>
>>> This leaves no room for implementing a service that is marked as not
>>> supported.
>>>
>>> In the descriptions of the return codes of the individual runtime services:
>>>
>>> "*EFI_UNSUPPORTED* This call is not supported by this platform at the
>>> time the call is made. The platform should describe this runtime service
>>> as unsupported at runtime via an EFI_RT_PROPERTIES_TABLE configuration
>>> table."
>> From the spec, it clearly describes
>>
>> If a platform cannot support calls defined in EFI_RUNTIME_SERVICES after
>> ExitBootServices() is called, that platform is permitted to provide
>> implementations of those runtime services that return EFI_UNSUPPORTED
>> when invoked at runtime. On such systems, an EFI_RT_PROPERTIES_TABLE
>> configuration table should be published describing which runtime
>> services are supported at runtime.
>>
>> I think it's better not to modify efi_test base on the
>> EFI_RT_PROPERTIES_TABLE or RuntimeServicesSupported, let efi_test be
>> simply ioctl and FWTS tests can do the modifications.
>>
> Doesn't that mean FTWS would need to be able to access the
> EFI_RT_PROPERTIES_TABLE?
>
Right, FWTS need to be able to get the RuntimeServicesSupported value.

I'm not sure if kernel will implement it or not, if not, maybe efi_test
can help to get and export the RuntimeServicesSupported from configure
table to FWTS.


Cheers,

Ivan


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <4898db16-7f9b-2efc-a5ae-356461d790b8@gmx.de>
2020-10-14 17:45 ` fwts: RuntimeServicesSupported variable Heinrich Schuchardt
2020-10-14 17:58   ` Ard Biesheuvel
2020-10-14 18:41     ` Heinrich Schuchardt
2020-10-19  9:31       ` Ard Biesheuvel
2020-10-19 10:00         ` Heinrich Schuchardt
2020-10-19 10:03           ` Ard Biesheuvel
2020-10-19 11:00             ` Heinrich Schuchardt
2020-10-19 11:01               ` Ard Biesheuvel
2020-10-19 11:25                 ` Heinrich Schuchardt
2020-10-20  6:20                   ` ivanhu
2020-10-20  6:46                     ` Ard Biesheuvel
2020-10-20  7:22                       ` ivanhu
2020-10-19 11:22             ` Heinrich Schuchardt

Linux-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org
	public-inbox-index linux-efi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-efi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git