On 08.06.23 21:38, Ilias Apalodimas wrote:
On Thu, 8 Jun 2023 at 16:52, Ard Biesheuvel ardb@kernel.org wrote:
On Thu, 8 Jun 2023 at 08:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Jan
On Wed, 7 Jun 2023 at 22:46, Jan Kiszka jan.kiszka@siemens.com wrote:
On 07.06.23 20:17, Ilias Apalodimas wrote:
On Wed, 7 Jun 2023 at 20:14, Jan Kiszka jan.kiszka@siemens.com wrote:
On 07.06.23 18:59, Ilias Apalodimas wrote: > On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas > ilias.apalodimas@linaro.org wrote: >> >> Hi Jan, >> >> [...] >>>>>> No I don't, this will work reliably without the need to remount the efivarfs. >>>>>> As you point out you will still have this dependency if you end up >>>>>> building them as modules and you manage to mount the efivarfs before >>>>>> those get inserted. Does anyone see a reasonable workaround? >>>>>> Deceiving the kernel and making the bootloader set the RT property bit >>>>>> to force the filesystem being mounted as rw is a nasty hack that we >>>>>> should avoid. Maybe adding a kernel command line parameter that says >>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love >>>>>> this either, but it's not unreasonable. >>>>> >>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094, >>>>> basically this issue mapped on reboot/shutdown, I would really love to >>>>> see the unhandy tee-supplicant daemon to be overcome. >>>> >>>> I have seen this error before and it has been on my todo list. So I >>>> have tried to fix it here [1]. Feel free to test it and let me know if >>>> you see any further issues. >>>> >>>> [1] https://lkml.org/lkml/2023/6/7/927 >>>> >>> >>> Ah, nice, will test ASAP! >>> >>> Meanwhile more food: I managed to build a firmware that was missing >>> STMM. But the driver loaded, and I got this: >> >> Thanks for the testing. I'll try to reproduce it locally and get back to you > > Can you provide a bit more info on how that was triggered btw? I would > be helpful to know > > - OP-TEE version
Today's master, 145953d55.
> - was it compiled as a module or built-in?
Sorry, not sure anymore, switching back and forth right now. I think it was built-in.
> - was the supplicant running?
Yes.
Ok thanks, that helps. I guess this also means U-Boot was compiled to store the variables in a file in the ESP instead of the RPMB right? Otherwise, I can't see how the device booted in the first place.
U-Boot was not configured to perform secure booting in this case. It had RPMB support enabled, just didn't have to use it.
In your initial mail you said you managed to build a firmware without StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI variable storage in an RPMB, the EFI subsystem will fail to start.
In any case, I don't think the ooops you are seeing is not connected to this patchset. Looking at the kernel EFI stub we only set the SetVariableRT if the RTPROP table is set accordingly by the firmware. U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it can't support it. What you are doing is remount the efivarfs as rw and then trying to set a variable, but the callback for it is NULL. I think you'll be able to replicate the same behavior on the kernel without even inserting the new module.
I have dropped this series from efi/next for now, given that it obviously has problems in its current state.
The risk of merging this now and fixing it later is that it may cause regressions for early adopters that rely on the behavior we are introducing here. Better to get this in shape first.
The ooops Jan was seeing is reproducible in the current code with $ sudo mount -o remount,rw /sys/firmware/efi/efivars $ sudo efi-updatevar -f PK.auth PK
So the only real problem we are discussing here is users having to remount the efivarfs once the module is inserted and the supplicant is running right? We could do something along the lines of the patch below. That would solve both of the problems.
However, the patch changes the way efivarfs is mounted -- it's now always mounted as RW. What I am worried about is userspace tools that rely on this. I know fwupd uses it and looks for a RO mounted efivarfs on it's initial checking, but since userspace apps are already dealing with firmware that exposes SetVariableRT but always returns EFI_UNSUPPORTED this should be safe ?(famous last words)
Jan can you please apply this and see if it solves your problem?
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 4cc8d0e7d0fd..37accd9e885c 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -206,6 +206,13 @@ static bool generic_ops_supported(void) return true; }
+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
u32 attributes, unsigned long data_size,void *data)+{
return EFI_UNSUPPORTED;+}
static int generic_ops_register(void) { if (!generic_ops_supported()) @@ -219,6 +226,9 @@ static int generic_ops_register(void) if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) { generic_ops.set_variable = efi.set_variable; generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
} else {generic_ops.set_variable = set_variable_int;generic_ops.set_variable_nonblocking = set_variable_int; } return efivars_register(&generic_efivars, &generic_ops);} diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index e9dc7116daf1..c75eff3f409d 100644 --- a/drivers/firmware/efi/vars.c +++ b/drivers/firmware/efi/vars.c @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);
bool efivar_supports_writes(void) {
return __efivars && __efivars->ops->set_variable;
return __efivars && __efivars->ops->set_variable != set_variable_int;} EXPORT_SYMBOL_GPL(efivar_supports_writes);
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index e028fafa04f3..e40b5c4c5106 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_d_op = &efivarfs_d_ops; sb->s_time_gran = 1;
if (!efivar_supports_writes())sb->s_flags |= SB_RDONLY;inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true); if (!inode) return -ENOMEM;diff --git a/include/linux/efi.h b/include/linux/efi.h index 58d1c271d3b0..ec0ac6ef50a3 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars, const struct efivar_operations *ops); int efivars_unregister(struct efivars *efivars);
+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
u32 attributes, unsigned long data_size,void *data);void efivars_generic_ops_register(void); void efivars_generic_ops_unregister(void);
apalos@hades:~/work/linux-next>; apalos@hades:~/work/linux-next>git diff diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 4cc8d0e7d0fd..37accd9e885c 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -206,6 +206,13 @@ static bool generic_ops_supported(void) return true; }
+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
u32 attributes, unsigned long data_size,void *data)+{
return EFI_UNSUPPORTED;+}
static int generic_ops_register(void) { if (!generic_ops_supported()) @@ -219,6 +226,9 @@ static int generic_ops_register(void) if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) { generic_ops.set_variable = efi.set_variable; generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
} else {generic_ops.set_variable = set_variable_int;generic_ops.set_variable_nonblocking = set_variable_int; } return efivars_register(&generic_efivars, &generic_ops);} diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index e9dc7116daf1..c75eff3f409d 100644 --- a/drivers/firmware/efi/vars.c +++ b/drivers/firmware/efi/vars.c @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);
bool efivar_supports_writes(void) {
return __efivars && __efivars->ops->set_variable;
return __efivars && __efivars->ops->set_variable != set_variable_int;} EXPORT_SYMBOL_GPL(efivar_supports_writes);
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index e028fafa04f3..e40b5c4c5106 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_d_op = &efivarfs_d_ops; sb->s_time_gran = 1;
if (!efivar_supports_writes())sb->s_flags |= SB_RDONLY;inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true); if (!inode) return -ENOMEM;diff --git a/include/linux/efi.h b/include/linux/efi.h index 58d1c271d3b0..ec0ac6ef50a3 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars, const struct efivar_operations *ops); int efivars_unregister(struct efivars *efivars);
+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
u32 attributes, unsigned long data_size,void *data);void efivars_generic_ops_register(void); void efivars_generic_ops_unregister(void);
Thanks /Ilias
As just written in my other reply: The root cause is the dependency on tee-supplicant daemon. That needs to be resolved, and then also r/w mounting will just work.
Jan