On Mon, 19 Oct 2020 at 13:00, Heinrich Schuchardt <xypron.glpk@xxxxxx> wrote: > > On 19.10.20 12:03, Ard Biesheuvel wrote: > > On Mon, 19 Oct 2020 at 12:00, Heinrich Schuchardt <xypron.glpk@xxxxxx> wrote: > >> > >> On 19.10.20 11:31, Ard Biesheuvel wrote: > >>> On Wed, 14 Oct 2020 at 20:41, Heinrich Schuchardt <xypron.glpk@xxxxxx> wrote: > >>>> > >>>> On 14.10.20 19:58, Ard Biesheuvel wrote: > >>>>> On Wed, 14 Oct 2020 at 19:45, Heinrich Schuchardt <xypron.glpk@xxxxxx> 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.
