On Fri, 23 Jun 2023 at 23:52, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Fri, 23 Jun 2023 at 13:31, Jason A. Donenfeld <Jason@xxxxxxxxx> wrote: > > > > We always have to write when using so that we don't credit the same > > seed twice, so it's gotta be used at a stage when SetVariable is > > somewhat working. > > This code isn't even the code that "uses" the alleged entropy from > that EFI variable in the first place. That's the code in > efi_random_get_seed() in the EFI boot sequence, and appends it to the > bootup randomness buffers. > > And that code already seems to clear the EFI variable (or seems to > append to it). > It reads the variable twice (once to obtain the size and once to grab the data), and replaces it with a zero-length string, which causes the variable to disappear. (This is typically NOR flash with spare blocks managed by a fault tolerant write layer in software, and so really wiping the seed or overwriting it is not generally possible) Using SetVariable() from boot services to delete a variable is highly unlikely to regress older systems in a similar way. > So this argument seems to be complete garbage - we absolutely do not > have to write it, and your patch already just wrote it in the wrong > place anyway. > > Don't make excuses. That code caused boot failures, it was all done in > the wrong place, and at entirely the wrong time. > With the revert applied, the kernel/EFI stub only consumes the variable and deletes it, but never creates it by itself, and so the code does nothing if the variable is never created in the first place. If we leave it up to user space to create it, we won´t need any policy or quirks handling in the kernel at all, which I´d prefer. The only thing we should do is special case the variable's scope GUID in efivarfs so the file is not created world-readable like we do for other variables. (This predates my involvement but I think this was an oversight). Using efivarfs will also ensure that the 'storage paranoia' logic is used on x86. (This is something I failed to take into account when I reviewed Jason's patch)
