On 11/24/20 2:10 PM, Ard Biesheuvel wrote:
On Tue, 24 Nov 2020 at 14:05, Heinrich Schuchardt <xypron.glpk@xxxxxx> 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@xxxxxxxxxxxxx> 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@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.
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;
+
+ 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_ */
Best regards
Heinrich
