Re: fwts: RuntimeServicesSupported variable

[Ard Biesheuvel <ardb@kernel.org>]

On Tue, 24 Nov 2020 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/24/20 2:10 PM, Ard Biesheuvel wrote:
> > On Tue, 24 Nov 2020 at 14:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/20/20 9:22 AM, ivanhu wrote:
> >>>
> >>> 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.
> >>
> >> Hello Ard,
> >>
> >> what are you plans to get the issue solved?
> >>
> >
> > No plans. Patches welcome.
> >
>
> Hello Ard,
>
> would a change like the following make sense to you?
>
> Are there any unit tests for the efi_test module which would have to be
> enhanced? I could not find anything in tools/testing/selftests/firmware.
>
> diff --git a/drivers/firmware/efi/test/efi_test.c
> b/drivers/firmware/efi/test/efi_test.c
> index ddf9eae396fe..c9fa62378851 100644
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -663,6 +663,19 @@ static long efi_runtime_query_capsulecaps(unsigned
> long arg)
>         return rv;
>   }
>
> +static long efi_runtime_get_supported_mask(unsigned long arg)
> +{
> +       unsigned int __user *supported_mask;
> +       int rv = 0;
> +
> +       runtime_services_supported = (unsigned int *)arg;
> +

Did you mean

supported_mask = (unsigned int *)arg;

here?

> +       if (put_user(efi.runtime_supported_mask, supported_mask);
> +               rv = -EFAULT;
> +
> +       return rv;
> +}
> +
>   static long efi_test_ioctl(struct file *file, unsigned int cmd,
>                                                         unsigned long arg)
>   {
> @@ -701,6 +714,9 @@ static long efi_test_ioctl(struct file *file,
> unsigned int cmd,
>                 return efi_runtime_reset_system(arg);
>         }
>
> +       case EFI_RUNTIME_GET_SUPPORTED_MASK:
> +               return efi_runtime_get_supported_mask(arg);
> +
>         return -ENOTTY;
>   }
>
> diff --git a/drivers/firmware/efi/test/efi_test.h
> b/drivers/firmware/efi/test/efi_test.h
> index f2446aa1c2e3..117349e57993 100644
> --- a/drivers/firmware/efi/test/efi_test.h
> +++ b/drivers/firmware/efi/test/efi_test.h
> @@ -118,4 +118,7 @@ struct efi_resetsystem {
>   #define EFI_RUNTIME_RESET_SYSTEM \
>         _IOW('p', 0x0B, struct efi_resetsystem)
>
> +#define EFI_RUNTIME_GET_SUPPORTED_MASK \
> +       _IOR('p', 0x0C, unsigned int)
> +
>   #endif /* _DRIVERS_FIRMWARE_EFI_TEST_H_ */
>

Thanks Heinrich, this looks reasonable to me (modulo the tweak above).
However, it is really up to Ivan, who maintains this stuff, along with
the user space components that go along with it.