Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Technology
Competence Center Embedded Linux