mbox series

[v1,0/4] Enable SetvariableRT

Message ID 20240406140203.248211-1-ilias.apalodimas@linaro.org
Headers show
Series Enable SetvariableRT | expand

Message

Ilias Apalodimas April 6, 2024, 2:01 p.m. UTC
Hi all,

This is an updated version of [0].

When EFI variables are stored on file we don't allow SetVariableRT,
since the OS doesn't know how to access or write that file.  At the same
time keeping the U-Boot drivers alive in runtime sections and performing
writes from the firmware is dangerous -- if at all possible.

For GetVariableRT  we copy runtime variables in RAM and expose them to
the OS. Add a Kconfig option and provide SetVariableRT using the same
memory back end. The OS will be responsible for syncing the RAM contents
to the file, otherwise any changes made during runtime won't persist
reboots.

It's worth noting that the variable store format is defined in EBBR [1]
and authenticated variables are explicitly prohibited, since they have
to be stored on a medium that's tamper and rollback protected.

The original RFC was just enabling the memory back end. This is a more
complete version and we should be able, with small adjustments of
userspace tools, fix SetVariableRT. If enabled the firmware will add two
new RO EFI variables after ExitBootServices.

RTStorageVolatile -- contains the filename, relative to the ESP
VarToFile -- an EFI variable that holds all the BS,RT, NV variables and can
be copied to the file defined by RTStorageVolatile.

If any errors occur during the variable init, the firmware will delete them
and adjust the RT_PROP table accordingly, disabling SetvarRT.

- pre-patch
$~ mount | grep efiva
efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)

$~ efibootmgr -n 0001
Could not set BootNext: Read-only file system

- post-patch
$~ mount | grep efiva
efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
, disabling SetvarRT.
$~ efibootmgr -n 0001
BootNext: 0001
BootCurrent: 0000
BootOrder: 0000,0001
Boot0000* debian        HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
Boot0001* virtio 0      VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}

$~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
Name: "BootNext"
Attributes:
        Non-Volatile
        Boot Service Access
        Runtime Service Access
Value:
00000000  01 00

[0] https://lore.kernel.org/u-boot/20240329071929.2461441-1-ilias.apalodimas@linaro.org/
[1] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage

Changes since the RFC:
- Return EFI_INVALID_PARAM if attributes are not volatile
- Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
- Add 2 EFI variables and allow userspace to write the file
- Add selftests

Ilias Apalodimas (4):
  efi_loader: conditionally enable SetvariableRT
  efi_loader: Add OS notifications for SetVariableRT in RAM
  efi_loader: add an EFI variable with the variable file contents
  efi_selftest: add tests for setvariableRT

 include/efi_loader.h                          |   4 +
 include/efi_variable.h                        |  15 +-
 lib/efi_loader/Kconfig                        |  16 ++
 lib/efi_loader/efi_boottime.c                 |   2 +
 lib/efi_loader/efi_runtime.c                  |   1 +
 lib/efi_loader/efi_var_common.c               |  43 ++--
 lib/efi_loader/efi_var_file.c                 |   1 -
 lib/efi_loader/efi_var_mem.c                  |  90 +++-----
 lib/efi_loader/efi_variable.c                 | 210 +++++++++++++++++-
 lib/efi_loader/efi_variable_tee.c             |   1 -
 .../efi_selftest_variables_runtime.c          | 116 +++++++++-
 11 files changed, 401 insertions(+), 98 deletions(-)

--
2.37.2

Comments

Ilias Apalodimas April 6, 2024, 2:01 p.m. UTC | #1
Hi all,

This is an updated version of [0].

When EFI variables are stored on file we don't allow SetVariableRT,
since the OS doesn't know how to access or write that file.  At the same
time keeping the U-Boot drivers alive in runtime sections and performing
writes from the firmware is dangerous -- if at all possible.

For GetVariableRT  we copy runtime variables in RAM and expose them to
the OS. Add a Kconfig option and provide SetVariableRT using the same
memory back end. The OS will be responsible for syncing the RAM contents
to the file, otherwise any changes made during runtime won't persist
reboots.

It's worth noting that the variable store format is defined in EBBR [1]
and authenticated variables are explicitly prohibited, since they have
to be stored on a medium that's tamper and rollback protected.

The original RFC was just enabling the memory back end. This is a more
complete version and we should be able, with small adjustments of
userspace tools, fix SetVariableRT. If enabled the firmware will add two
new RO EFI variables after ExitBootServices.

RTStorageVolatile -- contains the filename, relative to the ESP
VarToFile -- an EFI variable that holds all the BS,RT, NV variables and can
be copied to the file defined by RTStorageVolatile.

If any errors occur during the variable init, the firmware will delete them
and adjust the RT_PROP table accordingly, disabling SetvarRT.

- pre-patch
$~ mount | grep efiva
efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)

$~ efibootmgr -n 0001
Could not set BootNext: Read-only file system

- post-patch
$~ mount | grep efiva
efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
, disabling SetvarRT.
$~ efibootmgr -n 0001
BootNext: 0001
BootCurrent: 0000
BootOrder: 0000,0001
Boot0000* debian        HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
Boot0001* virtio 0      VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}

$~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
Name: "BootNext"
Attributes:
        Non-Volatile
        Boot Service Access
        Runtime Service Access
Value:
00000000  01 00

[0] https://lore.kernel.org/u-boot/20240329071929.2461441-1-ilias.apalodimas@linaro.org/
[1] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage

Changes since the RFC:
- Return EFI_INVALID_PARAM if attributes are not volatile
- Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
- Add 2 EFI variables and allow userspace to write the file
- Add selftests

Ilias Apalodimas (4):
  efi_loader: conditionally enable SetvariableRT
  efi_loader: Add OS notifications for SetVariableRT in RAM
  efi_loader: add an EFI variable with the variable file contents
  efi_selftest: add tests for setvariableRT

 include/efi_loader.h                          |   4 +
 include/efi_variable.h                        |  15 +-
 lib/efi_loader/Kconfig                        |  16 ++
 lib/efi_loader/efi_boottime.c                 |   2 +
 lib/efi_loader/efi_runtime.c                  |   1 +
 lib/efi_loader/efi_var_common.c               |  43 ++--
 lib/efi_loader/efi_var_file.c                 |   1 -
 lib/efi_loader/efi_var_mem.c                  |  90 +++-----
 lib/efi_loader/efi_variable.c                 | 210 +++++++++++++++++-
 lib/efi_loader/efi_variable_tee.c             |   1 -
 .../efi_selftest_variables_runtime.c          | 116 +++++++++-
 11 files changed, 401 insertions(+), 98 deletions(-)

--
2.37.2
Mark Kettenis April 7, 2024, 9:53 p.m. UTC | #2
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> Hi all,
> 
> This is an updated version of [0].
> 
> When EFI variables are stored on file we don't allow SetVariableRT,
> since the OS doesn't know how to access or write that file.  At the same
> time keeping the U-Boot drivers alive in runtime sections and performing
> writes from the firmware is dangerous -- if at all possible.
> 
> For GetVariableRT  we copy runtime variables in RAM and expose them to
> the OS. Add a Kconfig option and provide SetVariableRT using the same
> memory back end. The OS will be responsible for syncing the RAM contents
> to the file, otherwise any changes made during runtime won't persist
> reboots.
> 
> It's worth noting that the variable store format is defined in EBBR [1]
> and authenticated variables are explicitly prohibited, since they have
> to be stored on a medium that's tamper and rollback protected.
> 
> The original RFC was just enabling the memory back end. This is a more
> complete version and we should be able, with small adjustments of
> userspace tools, fix SetVariableRT. If enabled the firmware will add two
> new RO EFI variables after ExitBootServices.
> 
> RTStorageVolatile -- contains the filename, relative to the ESP
> VarToFile -- an EFI variable that holds all the BS,RT, NV variables and can
> be copied to the file defined by RTStorageVolatile.
> 
> If any errors occur during the variable init, the firmware will delete them
> and adjust the RT_PROP table accordingly, disabling SetvarRT.
> 
> - pre-patch
> $~ mount | grep efiva
> efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)
> 
> $~ efibootmgr -n 0001
> Could not set BootNext: Read-only file system
> 
> - post-patch
> $~ mount | grep efiva
> efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
> , disabling SetvarRT.
> $~ efibootmgr -n 0001
> BootNext: 0001
> BootCurrent: 0000
> BootOrder: 0000,0001
> Boot0000* debian        HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
> Boot0001* virtio 0      VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}
> 
> $~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
> GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
> Name: "BootNext"
> Attributes:
>         Non-Volatile
>         Boot Service Access
>         Runtime Service Access
> Value:
> 00000000  01 00
> 
> [0] https://lore.kernel.org/u-boot/20240329071929.2461441-1-ilias.apalodimas@linaro.org/
> [1] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> 
> Changes since the RFC:
> - Return EFI_INVALID_PARAM if attributes are not volatile
> - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> - Add 2 EFI variables and allow userspace to write the file
> - Add selftests
> 
> Ilias Apalodimas (4):
>   efi_loader: conditionally enable SetvariableRT
>   efi_loader: Add OS notifications for SetVariableRT in RAM
>   efi_loader: add an EFI variable with the variable file contents
>   efi_selftest: add tests for setvariableRT
> 
>  include/efi_loader.h                          |   4 +
>  include/efi_variable.h                        |  15 +-
>  lib/efi_loader/Kconfig                        |  16 ++
>  lib/efi_loader/efi_boottime.c                 |   2 +
>  lib/efi_loader/efi_runtime.c                  |   1 +
>  lib/efi_loader/efi_var_common.c               |  43 ++--
>  lib/efi_loader/efi_var_file.c                 |   1 -
>  lib/efi_loader/efi_var_mem.c                  |  90 +++-----
>  lib/efi_loader/efi_variable.c                 | 210 +++++++++++++++++-
>  lib/efi_loader/efi_variable_tee.c             |   1 -
>  .../efi_selftest_variables_runtime.c          | 116 +++++++++-
>  11 files changed, 401 insertions(+), 98 deletions(-)

I can't get patch 0003 from this series to apply on master or next.
Ilias Apalodimas April 8, 2024, 5:50 a.m. UTC | #3
Hi Mark


On Mon, 8 Apr 2024 at 00:53, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > Hi all,
> >
> > This is an updated version of [0].
> >
> > When EFI variables are stored on file we don't allow SetVariableRT,
> > since the OS doesn't know how to access or write that file.  At the same
> > time keeping the U-Boot drivers alive in runtime sections and performing
> > writes from the firmware is dangerous -- if at all possible.
> >
> > For GetVariableRT  we copy runtime variables in RAM and expose them to
> > the OS. Add a Kconfig option and provide SetVariableRT using the same
> > memory back end. The OS will be responsible for syncing the RAM contents
> > to the file, otherwise any changes made during runtime won't persist
> > reboots.
> >
> > It's worth noting that the variable store format is defined in EBBR [1]
> > and authenticated variables are explicitly prohibited, since they have
> > to be stored on a medium that's tamper and rollback protected.
> >
> > The original RFC was just enabling the memory back end. This is a more
> > complete version and we should be able, with small adjustments of
> > userspace tools, fix SetVariableRT. If enabled the firmware will add two
> > new RO EFI variables after ExitBootServices.
> >
> > RTStorageVolatile -- contains the filename, relative to the ESP
> > VarToFile -- an EFI variable that holds all the BS,RT, NV variables and can
> > be copied to the file defined by RTStorageVolatile.
> >
> > If any errors occur during the variable init, the firmware will delete them
> > and adjust the RT_PROP table accordingly, disabling SetvarRT.
> >
> > - pre-patch
> > $~ mount | grep efiva
> > efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)
> >
> > $~ efibootmgr -n 0001
> > Could not set BootNext: Read-only file system
> >
> > - post-patch
> > $~ mount | grep efiva
> > efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
> > , disabling SetvarRT.
> > $~ efibootmgr -n 0001
> > BootNext: 0001
> > BootCurrent: 0000
> > BootOrder: 0000,0001
> > Boot0000* debian        HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
> > Boot0001* virtio 0      VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}
> >
> > $~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
> > GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
> > Name: "BootNext"
> > Attributes:
> >         Non-Volatile
> >         Boot Service Access
> >         Runtime Service Access
> > Value:
> > 00000000  01 00
> >
> > [0] https://lore.kernel.org/u-boot/20240329071929.2461441-1-ilias.apalodimas@linaro.org/
> > [1] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >
> > Changes since the RFC:
> > - Return EFI_INVALID_PARAM if attributes are not volatile
> > - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> > - Add 2 EFI variables and allow userspace to write the file
> > - Add selftests
> >
> > Ilias Apalodimas (4):
> >   efi_loader: conditionally enable SetvariableRT
> >   efi_loader: Add OS notifications for SetVariableRT in RAM
> >   efi_loader: add an EFI variable with the variable file contents
> >   efi_selftest: add tests for setvariableRT
> >
> >  include/efi_loader.h                          |   4 +
> >  include/efi_variable.h                        |  15 +-
> >  lib/efi_loader/Kconfig                        |  16 ++
> >  lib/efi_loader/efi_boottime.c                 |   2 +
> >  lib/efi_loader/efi_runtime.c                  |   1 +
> >  lib/efi_loader/efi_var_common.c               |  43 ++--
> >  lib/efi_loader/efi_var_file.c                 |   1 -
> >  lib/efi_loader/efi_var_mem.c                  |  90 +++-----
> >  lib/efi_loader/efi_variable.c                 | 210 +++++++++++++++++-
> >  lib/efi_loader/efi_variable_tee.c             |   1 -
> >  .../efi_selftest_variables_runtime.c          | 116 +++++++++-
> >  11 files changed, 401 insertions(+), 98 deletions(-)
>
> I can't get patch 0003 from this series to apply on master or next.


Yes, my bad I forgot to mention you also need (before applying this series)
https://lore.kernel.org/u-boot/20240403153335.96971-1-heinrich.schuchardt@canonical.com/
https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masahisa@socionext.com/
https://lore.kernel.org/u-boot/20240405065058.591452-1-ilias.apalodimas@linaro.org/

Alternatively, you can clone directly
https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/setvar_rt1

Thanks
/Ilias





>
Heinrich Schuchardt April 8, 2024, 5:54 a.m. UTC | #4
On 4/7/24 23:53, Mark Kettenis wrote:
>> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>
>> Hi all,
>>
>> This is an updated version of [0].
>>
>> When EFI variables are stored on file we don't allow SetVariableRT,
>> since the OS doesn't know how to access or write that file.  At the same
>> time keeping the U-Boot drivers alive in runtime sections and performing
>> writes from the firmware is dangerous -- if at all possible.
>>
>> For GetVariableRT  we copy runtime variables in RAM and expose them to
>> the OS. Add a Kconfig option and provide SetVariableRT using the same
>> memory back end. The OS will be responsible for syncing the RAM contents
>> to the file, otherwise any changes made during runtime won't persist
>> reboots.
>>
>> It's worth noting that the variable store format is defined in EBBR [1]
>> and authenticated variables are explicitly prohibited, since they have
>> to be stored on a medium that's tamper and rollback protected.
>>
>> The original RFC was just enabling the memory back end. This is a more
>> complete version and we should be able, with small adjustments of
>> userspace tools, fix SetVariableRT. If enabled the firmware will add two
>> new RO EFI variables after ExitBootServices.
>>
>> RTStorageVolatile -- contains the filename, relative to the ESP
>> VarToFile -- an EFI variable that holds all the BS,RT, NV variables and can
>> be copied to the file defined by RTStorageVolatile.
>>
>> If any errors occur during the variable init, the firmware will delete them
>> and adjust the RT_PROP table accordingly, disabling SetvarRT.
>>
>> - pre-patch
>> $~ mount | grep efiva
>> efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)
>>
>> $~ efibootmgr -n 0001
>> Could not set BootNext: Read-only file system
>>
>> - post-patch
>> $~ mount | grep efiva
>> efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
>> , disabling SetvarRT.
>> $~ efibootmgr -n 0001
>> BootNext: 0001
>> BootCurrent: 0000
>> BootOrder: 0000,0001
>> Boot0000* debian        HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
>> Boot0001* virtio 0      VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}
>>
>> $~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
>> GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
>> Name: "BootNext"
>> Attributes:
>>          Non-Volatile
>>          Boot Service Access
>>          Runtime Service Access
>> Value:
>> 00000000  01 00
>>
>> [0] https://lore.kernel.org/u-boot/20240329071929.2461441-1-ilias.apalodimas@linaro.org/
>> [1] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
>>
>> Changes since the RFC:
>> - Return EFI_INVALID_PARAM if attributes are not volatile
>> - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
>> - Add 2 EFI variables and allow userspace to write the file
>> - Add selftests
>>
>> Ilias Apalodimas (4):
>>    efi_loader: conditionally enable SetvariableRT
>>    efi_loader: Add OS notifications for SetVariableRT in RAM
>>    efi_loader: add an EFI variable with the variable file contents
>>    efi_selftest: add tests for setvariableRT
>>
>>   include/efi_loader.h                          |   4 +
>>   include/efi_variable.h                        |  15 +-
>>   lib/efi_loader/Kconfig                        |  16 ++
>>   lib/efi_loader/efi_boottime.c                 |   2 +
>>   lib/efi_loader/efi_runtime.c                  |   1 +
>>   lib/efi_loader/efi_var_common.c               |  43 ++--
>>   lib/efi_loader/efi_var_file.c                 |   1 -
>>   lib/efi_loader/efi_var_mem.c                  |  90 +++-----
>>   lib/efi_loader/efi_variable.c                 | 210 +++++++++++++++++-
>>   lib/efi_loader/efi_variable_tee.c             |   1 -
>>   .../efi_selftest_variables_runtime.c          | 116 +++++++++-
>>   11 files changed, 401 insertions(+), 98 deletions(-)
>
> I can't get patch 0003 from this series to apply on master or next.
>

Hello Mark,

there are some prerequisite pending patches:

See
https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commits/setvar_rt1?ref_type=heads

Best regards

Heinrich
Mark Kettenis April 8, 2024, 2:16 p.m. UTC | #5
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Date: Sat,  6 Apr 2024 17:01:51 +0300
> 
> Hi all,

Hi Ilias,

> This is an updated version of [0].
> 
> When EFI variables are stored on file we don't allow SetVariableRT,
> since the OS doesn't know how to access or write that file.  At the same
> time keeping the U-Boot drivers alive in runtime sections and performing
> writes from the firmware is dangerous -- if at all possible.
> 
> For GetVariableRT  we copy runtime variables in RAM and expose them to
> the OS. Add a Kconfig option and provide SetVariableRT using the same
> memory back end. The OS will be responsible for syncing the RAM contents
> to the file, otherwise any changes made during runtime won't persist
> reboots.
> 
> It's worth noting that the variable store format is defined in EBBR [1]
> and authenticated variables are explicitly prohibited, since they have
> to be stored on a medium that's tamper and rollback protected.
> 
> The original RFC was just enabling the memory back end. This is a more
> complete version and we should be able, with small adjustments of
> userspace tools, fix SetVariableRT. If enabled the firmware will add two
> new RO EFI variables after ExitBootServices.
> 
> RTStorageVolatile -- contains the filename, relative to the ESP
> VarToFile -- an EFI variable that holds all the BS,RT, NV variables and can
> be copied to the file defined by RTStorageVolatile.

How does the OS know what the right ESP is?  The system might have
multiple disks that each have an ESP and more than one of those ESPs
might contain an ubootefi.var file.  Maybe this should be a full
EFI device path that includes the partition?

Also, is RTStorageVolutile the right name for this?  This is about
storing Non-Volatile variables after all...

> 
> If any errors occur during the variable init, the firmware will delete them
> and adjust the RT_PROP table accordingly, disabling SetvarRT.
> 
> - pre-patch
> $~ mount | grep efiva
> efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)
> 
> $~ efibootmgr -n 0001
> Could not set BootNext: Read-only file system
> 
> - post-patch
> $~ mount | grep efiva
> efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
> , disabling SetvarRT.
> $~ efibootmgr -n 0001
> BootNext: 0001
> BootCurrent: 0000
> BootOrder: 0000,0001
> Boot0000* debian        HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
> Boot0001* virtio 0      VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}
> 
> $~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
> GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
> Name: "BootNext"
> Attributes:
>         Non-Volatile
>         Boot Service Access
>         Runtime Service Access
> Value:
> 00000000  01 00
> 
> [0] https://lore.kernel.org/u-boot/20240329071929.2461441-1-ilias.apalodimas@linaro.org/
> [1] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> 
> Changes since the RFC:
> - Return EFI_INVALID_PARAM if attributes are not volatile
> - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> - Add 2 EFI variables and allow userspace to write the file
> - Add selftests
> 
> Ilias Apalodimas (4):
>   efi_loader: conditionally enable SetvariableRT
>   efi_loader: Add OS notifications for SetVariableRT in RAM
>   efi_loader: add an EFI variable with the variable file contents
>   efi_selftest: add tests for setvariableRT
> 
>  include/efi_loader.h                          |   4 +
>  include/efi_variable.h                        |  15 +-
>  lib/efi_loader/Kconfig                        |  16 ++
>  lib/efi_loader/efi_boottime.c                 |   2 +
>  lib/efi_loader/efi_runtime.c                  |   1 +
>  lib/efi_loader/efi_var_common.c               |  43 ++--
>  lib/efi_loader/efi_var_file.c                 |   1 -
>  lib/efi_loader/efi_var_mem.c                  |  90 +++-----
>  lib/efi_loader/efi_variable.c                 | 210 +++++++++++++++++-
>  lib/efi_loader/efi_variable_tee.c             |   1 -
>  .../efi_selftest_variables_runtime.c          | 116 +++++++++-
>  11 files changed, 401 insertions(+), 98 deletions(-)
> 
> --
> 2.37.2
> 
>
Ilias Apalodimas April 10, 2024, 2:32 p.m. UTC | #6
Hi Mark ,

I am on a conference, not checking emails too much

On Mon, 8 Apr 2024 at 16:16, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Date: Sat,  6 Apr 2024 17:01:51 +0300
> >
> > Hi all,
>
> Hi Ilias,
>
> > This is an updated version of [0].
> >
> > When EFI variables are stored on file we don't allow SetVariableRT,
> > since the OS doesn't know how to access or write that file.  At the same
> > time keeping the U-Boot drivers alive in runtime sections and performing
> > writes from the firmware is dangerous -- if at all possible.
> >
> > For GetVariableRT  we copy runtime variables in RAM and expose them to
> > the OS. Add a Kconfig option and provide SetVariableRT using the same
> > memory back end. The OS will be responsible for syncing the RAM contents
> > to the file, otherwise any changes made during runtime won't persist
> > reboots.
> >
> > It's worth noting that the variable store format is defined in EBBR [1]
> > and authenticated variables are explicitly prohibited, since they have
> > to be stored on a medium that's tamper and rollback protected.
> >
> > The original RFC was just enabling the memory back end. This is a more
> > complete version and we should be able, with small adjustments of
> > userspace tools, fix SetVariableRT. If enabled the firmware will add two
> > new RO EFI variables after ExitBootServices.
> >
> > RTStorageVolatile -- contains the filename, relative to the ESP
> > VarToFile -- an EFI variable that holds all the BS,RT, NV variables and can
> > be copied to the file defined by RTStorageVolatile.
>
> How does the OS know what the right ESP is?  The system might have
> multiple disks that each have an ESP and more than one of those ESPs
> might contain an ubootefi.var file.  Maybe this should be a full
> EFI device path that includes the partition?

This delegates the problem no? How does the firmware know what ESP the
OS will need, so it can inject the proper DP?
Usually in Linux,  each OS version will know and mount the ESP it
needs using a UUID defined during the installation, so the name
relative to ESP seemed better.

In any case, this was a concern of mine as well on the RFC and I did
say we can alternatively use a full device path, but it will make the
userspace app harder. But as I said we will need a mechanism that the
firmware understands which OS pairs with each ESP.

>
> Also, is RTStorageVolutile the right name for this?  This is about
> storing Non-Volatile variables after all...

It's supposed to mean "Variable storage is volatile, here's the
filename you can use to persist them", but suggestions are always
welcome!

Thanks
/Ilias
>
> >
> > If any errors occur during the variable init, the firmware will delete them
> > and adjust the RT_PROP table accordingly, disabling SetvarRT.
> >
> > - pre-patch
> > $~ mount | grep efiva
> > efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)
> >
> > $~ efibootmgr -n 0001
> > Could not set BootNext: Read-only file system
> >
> > - post-patch
> > $~ mount | grep efiva
> > efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
> > , disabling SetvarRT.
> > $~ efibootmgr -n 0001
> > BootNext: 0001
> > BootCurrent: 0000
> > BootOrder: 0000,0001
> > Boot0000* debian        HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
> > Boot0001* virtio 0      VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}
> >
> > $~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
> > GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
> > Name: "BootNext"
> > Attributes:
> >         Non-Volatile
> >         Boot Service Access
> >         Runtime Service Access
> > Value:
> > 00000000  01 00
> >
> > [0] https://lore.kernel.org/u-boot/20240329071929.2461441-1-ilias.apalodimas@linaro.org/
> > [1] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >
> > Changes since the RFC:
> > - Return EFI_INVALID_PARAM if attributes are not volatile
> > - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> > - Add 2 EFI variables and allow userspace to write the file
> > - Add selftests
> >
> > Ilias Apalodimas (4):
> >   efi_loader: conditionally enable SetvariableRT
> >   efi_loader: Add OS notifications for SetVariableRT in RAM
> >   efi_loader: add an EFI variable with the variable file contents
> >   efi_selftest: add tests for setvariableRT
> >
> >  include/efi_loader.h                          |   4 +
> >  include/efi_variable.h                        |  15 +-
> >  lib/efi_loader/Kconfig                        |  16 ++
> >  lib/efi_loader/efi_boottime.c                 |   2 +
> >  lib/efi_loader/efi_runtime.c                  |   1 +
> >  lib/efi_loader/efi_var_common.c               |  43 ++--
> >  lib/efi_loader/efi_var_file.c                 |   1 -
> >  lib/efi_loader/efi_var_mem.c                  |  90 +++-----
> >  lib/efi_loader/efi_variable.c                 | 210 +++++++++++++++++-
> >  lib/efi_loader/efi_variable_tee.c             |   1 -
> >  .../efi_selftest_variables_runtime.c          | 116 +++++++++-
> >  11 files changed, 401 insertions(+), 98 deletions(-)
> >
> > --
> > 2.37.2
> >
> >