| Message ID | 20230624180446.2048867-1-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | efivarfs: Expose RandomSeed variable but with limited permissions | expand |
On Sat, Jun 24, 2023 at 08:04:46PM +0200, Ard Biesheuvel wrote: > The efivarfs pseudo-filesystems exposes all EFI variables as > world-readable, and carries some logic to prevent accidental deletion > from bricking a system, by setting the immutable flag on all variables > whose purpose is unknown. > > When the RandomSeed support was added, we decided not to expose this > variable via efivarfs at all, given that the kernel itself was intended > to both produce and consume it directly, without involvement from user > space. This removed the need to deal with the world-readable default > permissions (which would be undesirable for the random seed that will be > used on the next boot), as this would require special handling of the > RandomSeed variable, given that we cannot restrict those permissions for > all EFI variables without running the risk of breaking user space. > > Now that the producer side of this mechanism has been reverted [0], it > is no longer possible to set the RandomSeed variable at all. Whether > and how we will replace the in-kernel producer with something more > robust is still under discussion, but in the mean time, let's relax the > efivarfs restriction on any direct access of the variable, and instead, > ensure that it is created as user read-write only, both when the EFI > varstore is enumerated (at mount time) and when the file is created > explicitly by user space. > > Also ensure that the file is not created with the immutable flag set so > that user space can manipulate the file as usual. Hm, I'm still not so sure we want to open the pandora's box of having to deal with userspaces writing this. Maybe we can keep it in the kernel but delay it until a little later in boot so it doesn't cause an outright bootup hang? Or just bite the bullet and do it at shutdown? I think I'd prefer just doing it in a delayed workqueue in late/post boot though. (On the topic of this patch, technically this only needs to be write-only, not read-write, right?) Jason
On Sun, 25 Jun 2023 at 07:44, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hm, I'm still not so sure we want to open the pandora's box of having to > deal with userspaces writing this. Maybe we can keep it in the kernel > but delay it until a little later in boot so it doesn't cause an > outright bootup hang? I don't know a good point to do it at, which would be after the system is clearly up and running. The obvious "just delay it by a X amount" might not even work, because you have things like "the system is still waiting for the user to type the DM password" or something like that. That said, it might be enough to at least see if Sami gets more information. Sami - can you try to revert the revert: git revert 69cbeb61ff9093a9155cb19a36d633033f71093a but then additionally in drivers/firmware/efi/efi.c, just change the static DECLARE_WORK(work, refresh_nv_rng_seed); schedule_work(&work); in refresh_nv_rng_seed_notification() to be something like static DECLARE_DELAYED_WORK(work, refresh_nv_rng_seed); schedule_delayed_work(&work, 120*HZ); to make the work fire two minutes after boot? The question then being: (a) does that fix the boot for you (maybe it's the schedule_work itself that confused things, however unlikely that sounds) (b) if it does boot, do you notice something happening two minutes after booting? But see all the craziness (for example) in efi_query_variable_store() and the whole 'efi_no_storage_paranoia' thing etc. Messing with EFI variables has always been painful. Or just look at efi_crash_gracefully_on_page_fault() and the whole efi runtime services garbage with efi_rts_work etc. Yes, those are presumably mostly old EFI setups, but that is kind of the basic problem here. Many of these EFI calls are much more tested and reliable in the context of EFI boot services, and *much* less reliable in the context of "runtime services". EFI runtime services have been a major PITA over the years. And it's possible that it's just entirely broken on Sami's laptop. I'm not entirely sure which laptop it is, with laptop manufacturers often re-using model names over several years, but "HP 6730b" seems to be basically from July 2008 (going by the service manual I found). So we're talking 15 years ago, and yes, EFI was likely *much* less reliable back then. Linus
On Sun, 25 Jun 2023 at 21:22, Linus Torvalds <torvalds@linux-foundation.org> wrote: > ... > > Yes, those are presumably mostly old EFI setups, but that is kind of > the basic problem here. Many of these EFI calls are much more tested > and reliable in the context of EFI boot services, and *much* less > reliable in the context of "runtime services". > Many of these systems shipped with Vista or older booting in BIOS emulation mode (CSM), so the OS visible API surface was barely tested, not even by booting Windows (which appears to be the gold standard for EFI compliance testing according to the OEMs building Windows PCs) > EFI runtime services have been a major PITA over the years. And it's > possible that it's just entirely broken on Sami's laptop. > Agreed. The virtual remapping stuff is particularly nasty, and a constant source of problems, also on arm64, even though it is completely pointless on 64-bit architectures. So I'm generally not a fan of wiring up more of the EFI runtime stuff, but I did not anticipate issues like this one with the varstore API that we already use in various other places. > I'm not entirely sure which laptop it is, with laptop manufacturers > often re-using model names over several years, but "HP 6730b" seems to > be basically from July 2008 (going by the service manual I found). So > we're talking 15 years ago, and yes, EFI was likely *much* less > reliable back then. > Yeah. What we might consider is keying off of the UEFI revision field at a certain cutoff value (e.g., only support this for ~v2.4 or higher), but this would be somewhat arbitrary, of course. I'd rather have an explicit opt-in, but that is policy, making user space a more appropriate venue for this.
On Sun, Jun 25, 2023 at 9:58 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > I'm not entirely sure which laptop it is, with laptop manufacturers > > often re-using model names over several years, but "HP 6730b" seems to > > be basically from July 2008 (going by the service manual I found). So > > we're talking 15 years ago, and yes, EFI was likely *much* less > > reliable back then. > > > > Yeah. What we might consider is keying off of the UEFI revision field > at a certain cutoff value (e.g., only support this for ~v2.4 or > higher) Oh I actually really like that idea. I didn't realize it was possible. Let's just choose a quite modern UEFI revision, and then we won't need to deal with the horrors of the past. And eventually what is quite modern now will be old and widespread. Jason
On Sunday, June 25th, 2023 at 22.21, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Sami - can you try to revert the revert: > > git revert 69cbeb61ff9093a9155cb19a36d633033f71093a > > but then additionally in drivers/firmware/efi/efi.c, just change the > > static DECLARE_WORK(work, refresh_nv_rng_seed); > schedule_work(&work); > > in refresh_nv_rng_seed_notification() to be something like > > static DECLARE_DELAYED_WORK(work, refresh_nv_rng_seed); > schedule_delayed_work(&work, 120*HZ); > > to make the work fire two minutes after boot? > > The question then being: > > (a) does that fix the boot for you (maybe it's the schedule_work > itself that confused things, however unlikely that sounds) > > (b) if it does boot, do you notice something happening two minutes > after booting? I did that and: (a) It does boot fine with this change. (b) I don't notice anything happening at two minutes after booting. I can see there is a random-seed file located in EFI partition, but it does not change two minutes after boot (but it is different each boot). > I'm not entirely sure which laptop it is, with laptop manufacturers > often re-using model names over several years, but "HP 6730b" seems to > be basically from July 2008 (going by the service manual I found). So > we're talking 15 years ago, and yes, EFI was likely much less > reliable back then. I got the laptop secondhand 7 years ago, but the 2008 has been my guess as well based on online resources. I have the _latest_ bios on it, 68PDD Ver. F.20 from 2011.
On Mon, Jun 26, 2023 at 03:15:08PM +0000, Sami Korkalainen wrote: > On Sunday, June 25th, 2023 at 22.21, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > > Sami - can you try to revert the revert: > > > > git revert 69cbeb61ff9093a9155cb19a36d633033f71093a > > > > but then additionally in drivers/firmware/efi/efi.c, just change the > > > > static DECLARE_WORK(work, refresh_nv_rng_seed); > > schedule_work(&work); > > > > in refresh_nv_rng_seed_notification() to be something like > > > > static DECLARE_DELAYED_WORK(work, refresh_nv_rng_seed); > > schedule_delayed_work(&work, 120*HZ); > > > > to make the work fire two minutes after boot? > > > > The question then being: > > > > (a) does that fix the boot for you (maybe it's the schedule_work > > itself that confused things, however unlikely that sounds) > > > > (b) if it does boot, do you notice something happening two minutes > > after booting? > > I did that and: > (a) It does boot fine with this change. Interesting! And after around 4 minutes, you don't see any stall warnings in your dmesg? > (b) I don't notice anything happening at two minutes after booting. I > can see there is a random-seed file located in EFI partition, but it > does not change two minutes after boot (but it is different each > boot). That's an unrelated file. This has to do with EFI variables.
On Monday, June 26th, 2023 at 18.23, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > Interesting! And after around 4 minutes, you don't see any stall > warnings in your dmesg? > No stall warnings or other interesting messages. Between 30s and 600s, only some [UFW BLOCK] messages and perf messages like this: [ 163.495537] perf: interrupt took too long (2517 > 2500), lowering kernel.perf_event_max_sample_rate to 79200 –Sami
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c index b973a2c03dde825e..00cf368fb0ea7315 100644 --- a/fs/efivarfs/inode.c +++ b/fs/efivarfs/inode.c @@ -92,8 +92,8 @@ static int efivarfs_create(struct mnt_idmap *idmap, struct inode *dir, if (err) goto out; if (guid_equal(&var->var.VendorGuid, &LINUX_EFI_RANDOM_SEED_TABLE_GUID)) { - err = -EPERM; - goto out; + mode &= S_IFMT | S_IRUSR | S_IWUSR; + is_removable = true; } if (efivar_variable_is_removable(var->var.VendorGuid, diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index 482d612b716bb1f0..f98597ec2105ffbb 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -115,9 +115,12 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor, int len; int err = -ENOMEM; bool is_removable = false; + umode_t mode = S_IRUGO | S_IWUSR; - if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID)) - return 0; + if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID)) { + mode = S_IRUSR | S_IWUSR; + is_removable = true; + } entry = kzalloc(sizeof(*entry), GFP_KERNEL); if (!entry) @@ -147,7 +150,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor, /* replace invalid slashes like kobject_set_name_vargs does for /sys/firmware/efi/vars. */ strreplace(name, '/', '!'); - inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0, + inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | mode, 0, is_removable); if (!inode) goto fail_name;
The efivarfs pseudo-filesystems exposes all EFI variables as world-readable, and carries some logic to prevent accidental deletion from bricking a system, by setting the immutable flag on all variables whose purpose is unknown. When the RandomSeed support was added, we decided not to expose this variable via efivarfs at all, given that the kernel itself was intended to both produce and consume it directly, without involvement from user space. This removed the need to deal with the world-readable default permissions (which would be undesirable for the random seed that will be used on the next boot), as this would require special handling of the RandomSeed variable, given that we cannot restrict those permissions for all EFI variables without running the risk of breaking user space. Now that the producer side of this mechanism has been reverted [0], it is no longer possible to set the RandomSeed variable at all. Whether and how we will replace the in-kernel producer with something more robust is still under discussion, but in the mean time, let's relax the efivarfs restriction on any direct access of the variable, and instead, ensure that it is created as user read-write only, both when the EFI varstore is enumerated (at mount time) and when the file is created explicitly by user space. Also ensure that the file is not created with the immutable flag set so that user space can manipulate the file as usual. [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=69cbeb61ff9093 Cc: Jason A. Donenfeld <Jason@zx2c4.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- fs/efivarfs/inode.c | 4 ++-- fs/efivarfs/super.c | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-)