Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

efivar: Copy VarToFile to RTStorageVolatile file at ESP #267

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jetm
Copy link

@jetm jetm commented Jun 15, 2024

An extension of Ilias Apalodimas' work (@apalos), merged in U-Boot, to support it in userspace.

EFI is becoming more common on embedded boards with the embracing of SystemReady-IR.

U-Boot which is most commonly used, is usually storing the EFI variables in a file in the ESP. That makes it impossible to support SetVariable at Runtime reliably, since the OS doesn't know how to access, read or write that file.

OS'es usually need SetVariable at runtime for three reasons:

  • Set the BootOrder
  • Enable UEFI Secure Boot
  • OSIndication to signal capsule updates on-disk

Since the variables are stored in a file U-Boot enables SetVariable at runtime in the EFI config table and stores any updates in RAM. At the same file it creates two volatile variables:

  • RTStorageVolatile is the location of the file relative to the ESP
  • VarTofile contains a binary dump of the EFI variables that need to be preserved on the file (BS, RT, NV)

U-Boot fills in the VarToFile dynamically on reads and that includes any updates the OS did in the meantime.

So, let's update efivar to do the same thing. Once a variable is written to the efivarfs, make sure efivars is mounted as rw and scan for the file RTStorageVolatile. If we find that, copy the VarToFile contents in a file and preserve the variables across reboots

@apalos suggested that similar work should be done in efibootmgr. We will need to share code from efivar, so it can be called from efibootmgr. Please advise on what would be a better approach.

Suggested-by: Ilias Apalodimas [email protected]
Signed-off-by: Javier Tia [email protected]

@apalos
Copy link

apalos commented Jun 17, 2024

@jetm apart from any code comments people might have, I'd like a better description on the commit message.

Something along the lines of:
"EFI is becoming more common on embedded boards with the embracing of SystemReady-IR.
U-Boot which is most commonly used, is usually storing the EFI variables in a file in the ESP. That makes it impossible to support SetVariable at Runtime reliably, since the OS doesn't know how to access, read or write that file.

OS'es usually need SetVariable at runtime for three reasons

  • Set the BootOrder
  • Enable UEFI Secure Boot
  • OSIndication to signal capsule updates on-disk.

Since the variables are stored in a file U-Boot enables SetVariable at runtime in the EFI config table and stores any updates in RAM. At the same file it creates 2 volatile variables

  • RTStorageVolatile is the location of the file relative to the ESP
  • VarTofile contains a binary dump of the EFI variables that need to be preserved on the file (BS, RT, NV)

U-Boot fills in the VarToFile dynamically on reads and that includes any updates the OS did in the meantime.

So, let's update efivar to do the same thing. Once a variable is written to the efivarfs, make sure efivars is mounted as rw and scan for the file "RTStorageVolatile". If we find that, copy the "VarToFile" contents in a file and preserve the variables across reboots"

also it's 'apalos', please fix the mention, some pool soul is getting notifications for EFI :)

@jetm
Copy link
Author

jetm commented Jun 17, 2024

@apalos Updated the commit message and PR description. Thank you!

src/efivar.c Outdated Show resolved Hide resolved
src/efivar.c Outdated Show resolved Hide resolved
@marcan
Copy link

marcan commented Jul 9, 2024

We are interested in this change for Asahi Linux, since we are one such U-Boot platform with EFI vars only storable in the ESP. However, there is one additional quirk this code does not handle. Our systems can have multiple ESPs, since our platform is not UEFI-native: the platform has its own lower level concept of multiple OS installs that our UEFI implementation nests under, so a UEFI "container" with its own ESP is created for every OS install.

If I'm reading the code right here, this would clobber the EFI variables of every mounted ESP. That would work in 95% of cases (since normally only the current OS ESP is mounted) and cause all kinds of hard to debug trouble in the 5% of cases where someone with multiple OS installs ends up with other ESPs mounted.

Possible solutions:

  1. Actually implement our bespoke protocol for identifying the right ESP, which is to read /proc/device-tree/chosen/asahi,efi-system-partition which will return the NUL-terminated GPT PARTUUID (in ASCII), and then only look for that one specific ESP.
  2. Only allow ESPs mounted somewhere under /boot (if someone mounts another ESP it's almost certainly going to be under /mnt or /run/media or something)
  3. Only target the first discovered ESP in /proc/mounts order (the real ESP is almost certainly going to be mounted first)
  4. Standardize a mechanism for U-Boot to indicate the ESP PARTUUID (maybe RTStorageVolatile should also include the PARTUUID? This seems like the cleanest solution and very UEFI-like, though changing the format of that variable now would mean a breaking change vs. the state of things on the u-boot side as things stand today).

Of these 2 and 3 are obviously heuristics but avoid platform-specific code, while 1 is the "correct" solution but obviously very specific to our platform, and 4 makes the most sense to me TBH but would require a rethink of RTStorageVolatile.

Worth noting that this is not just about weird multi-ESP platforms like ours. If I'm reading the code correctly, this will also clobber the vars in any ESPs in external storage and things like that. That's going to cause some really hard to debug badness when people do stuff like plug in bootable USB sticks or SD cards from embedded systems into other systems using the same mechanism. I suspect this is a bad enough oversight to warrant changing the definition of RTStorageVolatile before it's too late and we're stuck with ambiguous ESP detection.

@apalos
Copy link

apalos commented Jul 22, 2024

Hey Hector thanks for taking the time!

We are interested in this change for Asahi Linux, since we are one such U-Boot platform with EFI vars only storable in the ESP. However, there is one additional quirk this code does not handle. Our systems can have multiple ESPs, since our platform is not UEFI-native: the platform has its own lower level concept of multiple OS installs that our UEFI implementation nests under, so a UEFI "container" with its own ESP is created for every OS install.

If I'm reading the code right here, this would clobber the EFI variables of every mounted ESP. That would work in 95% of cases (since normally only the current OS ESP is mounted) and cause all kinds of hard to debug trouble in the 5% of cases where someone with multiple OS installs ends up with other ESPs mounted.

Yes it would, we also discussed this on the U-Boot ML while we reviewed the patches. I am happy to send an update once we figure this out.

One additional problem to keep in mind (which works with the current code) is OS installers. Some installers, e.g Ubuntu, contain an ESP in the installer image. They also create a new one during the installation. Storing the file in the pre-existing ESP won't work and the changes will be lost. We want the userspace tools to update the variables in the newly created partition.

Possible solutions:

1. Actually implement our bespoke protocol for identifying the right ESP, which is to read `/proc/device-tree/chosen/asahi,efi-system-partition` which will return the NUL-terminated GPT PARTUUID (in ASCII), and then only look for that one specific ESP.

2. Only allow ESPs mounted somewhere under /boot (if someone mounts another ESP it's almost certainly going to be under `/mnt` or `/run/media` or something)

3. Only target the _first_ discovered ESP in `/proc/mounts` order (the real ESP is almost certainly going to be mounted first)

4. Standardize a mechanism for U-Boot to indicate the ESP PARTUUID (maybe `RTStorageVolatile` should also include the PARTUUID? This seems like the cleanest solution and very UEFI-like, though changing the format of that variable now would mean a breaking change vs. the state of things on the u-boot side as things stand today).

The feature in U-Boot just got merged a few weeks ago, and I did not enable by default for a reason. We don't expect to break anything, so we are free to change this.

When I sent the patches, we had a short discussion about this and thought about storing the device path in RTStorageVolatile, but as I mentioned in that email, I think this delegates the problem but doesn't solve it....

The real problem is how U-Boot will decide/guess which ESP the OS is going to mount. If we can answer that we can encode whatever information we want in the variable to hint the OS. Even that though doesn't solve the OS installer problem I mentioned in the beginning...

Of these 2 and 3 are obviously heuristics but avoid platform-specific code, while 1 is the "correct" solution but obviously very specific to our platform, and 4 makes the most sense to me TBH but would require a rethink of RTStorageVolatile.

Worth noting that this is not just about weird multi-ESP platforms like ours. If I'm reading the code correctly, this will also clobber the vars in any ESPs in external storage and things like that. That's going to cause some really hard to debug badness when people do stuff like plug in bootable USB sticks or SD cards from embedded systems into other systems using the same mechanism. I suspect this is a bad enough oversight to warrant changing the definition of RTStorageVolatile before it's too late and we're stuck with ambiguous ESP detection.

It's not an oversight, we were very well aware of the problem -- even since my first RFC , but since we didn't have any valid use cases, we decided to keep the code that solves the 'installer problem'.

U-Boot itself is a bit problematic when it comes to the file and ESP variables. The SetVariable implementation will use the first file it finds on any ESP partition -- we don't have a way to select which file we want to write.

Out of the 4 solutions you proposed:

  1. This will still suffer from the 'installer problem'
  2. This might feel a bit hacky -- simply because the EFI allows multiple ESPs, but I also think it solves the majority of our problems
  3. I prefer 2 over this
  4. I can't find a reasonable way for u-boot to decide which ESP the OS is going to mount, If we can agree on this with distros, this is indeed the cleanest solution

@vathpela
Copy link
Contributor

vathpela commented Aug 2, 2024

I strongly prefer solution 2 here. I think that will also mean that we don't have to use blkid_probe_all(), which isn't the nicest thing to run on some machines.

@apalos
Copy link

apalos commented Aug 5, 2024

I strongly prefer solution 2 here. I think that will also mean that we don't have to use blkid_probe_all(), which isn't the nicest thing to run on some machines.

Thanks @vathpela this helps, we all seem to prefer 2. The last bit I don't like on this PR, is that the code only works for 'efivar'. We ideally need it as part of the library so it works with efibootmgr out of the box.

@vathpela
Copy link
Contributor

vathpela commented Aug 5, 2024

We ideally need it as part of the library so it works with efibootmgr out of the box.

If you plumb it through as a full third method like vars vs efivarfs that should just happen.

@jetm
Copy link
Author

jetm commented Oct 6, 2024

Addressed @marcan @apalos comments.

  • It searches for ESP file in three possible ESP partitions. I know in the PR discussion was only mentioned /boot, but /boot/efi and /efi are possible cases too, like systemd does. The code is simpler now because it's not using blkid anymore.
  • Expose the required APIs in libefivar to be used by efibootmgr tool, which is in another repo. This will avoid duplicating code and it will not be required to change efibootmgr

src/lib.c Outdated Show resolved Hide resolved
src/lib.c Outdated Show resolved Hide resolved
@apalos
Copy link

apalos commented Oct 10, 2024

@jetm some observations from testing

  1. If you rename the ESP file and try to store a variable the app correctly figures out there's no file to store, but the variable in memory is updated. The file existence check must run before updating the in-memory copy
  2. efivar: RSTStorageVolatile found with ubootefi.var
    efivar: VarToFile content saved at /boot/efi/ubootefi.var
    Don't display this on success, it doesn't mean much to users. Instead display an error only if RSTStorageVolatile points to a file that doesn't exist
  3. Even if the app prints
efivar: RSTStorageVolatile found with ubootefi.var
efivar: VarToFile content saved at /boot/efi/ubootefi.var

The variables don't persist a reboot

@vathpela does the function moving around make sense to expose the functionality to the library? Or is there a better way to do it?

src/lib.c Outdated Show resolved Hide resolved
src/lib.c Outdated Show resolved Hide resolved
src/lib.c Outdated Show resolved Hide resolved
src/efivar.c Outdated Show resolved Hide resolved
src/lib.c Show resolved Hide resolved
src/lib.c Outdated Show resolved Hide resolved
src/lib.c Outdated Show resolved Hide resolved
src/lib.c Outdated Show resolved Hide resolved
src/lib.c Outdated Show resolved Hide resolved
src/efivarfs.c Outdated Show resolved Hide resolved
@jetm jetm requested a review from apalos October 25, 2024 13:48
Copy link

@apalos apalos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vathpela FWIW

Acked-by: Ilias Apalodimas [email protected]
Tested-by: Ilias Apalodimas [email protected] # for efivar only, not efibootmgr

@apalos
Copy link

apalos commented Oct 30, 2024

@vathpela FWIW

Acked-by: Ilias Apalodimas [email protected] Tested-by: Ilias Apalodimas [email protected] # for efivar only, not efibootmgr

@jetm there's still one thing to fix.

The prints and the checking should only happen if RTStorageVolatile is there to begin with.

Running it on an EFI system that doesn't need a file to store variables works, but prints

efivar: Error -1 getting filename from RTStorageVolatile

This should not be visible if the variable is not there

src/lib.c Outdated Show resolved Hide resolved
EFI is becoming more common on embedded boards with the embracing of
SystemReady-IR.

U-Boot which is most commonly used, is usually storing the EFI variables
in a file in the ESP. That makes it impossible to support SetVariable at
Runtime reliably, since the OS doesn't know how to access, read or write
that file.

OS'es usually need SetVariable at runtime for three reasons:

- Set the BootOrder
- Enable UEFI Secure Boot
- OSIndication to signal capsule updates on-disk

Since the variables are stored in a file U-Boot enables SetVariable at
runtime in the EFI config table and stores any updates in RAM. At the
same file it creates two volatile variables:

- RTStorageVolatile is the location of the file relative to the ESP
- VarTofile contains a binary dump of the EFI variables that need to be
  preserved on the file (BS, RT, NV)

U-Boot fills in the VarToFile dynamically on reads and that includes any
updates the OS did in the meantime.

So, let's update efivar to do the same thing. Once a variable is written
to the efivarfs, make sure efivars is mounted as rw and scan for the
file RTStorageVolatile. If we find that, copy the VarToFile contents in
a file and preserve the variables across reboots.

Suggested-by: Ilias Apalodimas <[email protected]>
Acked-by: Ilias Apalodimas [email protected]
Tested-by: Ilias Apalodimas [email protected]
Signed-off-by: Javier Tia <[email protected]>
@jetm jetm changed the title RFC: efivar: Copy VarToFile to RTStorageVolatile file at ESP efivar: Copy VarToFile to RTStorageVolatile file at ESP Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants