| Message ID | 20240315002616.422802-1-timschumi@gmx.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/3] efi: pstore: Request at most 512 bytes for variable names | expand |
Hi Tim, On Fri, 15 Mar 2024 at 01:27, Tim Schumacher <timschumi@gmx.de> wrote: > > Work around a quirk in a few old (2011-ish) UEFI implementations, where > a call to `GetNextVariableName` with a buffer size larger than 512 bytes > will always return EFI_INVALID_PARAMETER. > > This was already done to efivarfs in f45812cc23fb ("efivarfs: Request at > most 512 bytes for variable names"), but the second copy of the variable > iteration implementation was overlooked. > > Signed-off-by: Tim Schumacher <timschumi@gmx.de> Thanks for the patch. I'll take it as a fix. As an aside, you really want to avoid EFI pstore in general, and specifically on such old systems with quirky UEFI implementations. > --- > I CC'd the pstore people and linux-hardening mailing list because > get_maintainer.pl suggested to do so. Apologies in case this was the > incorrect decision, this is a very non-pstore-specific patch after all. > If any of the linux-hardening/pstore people give you grief, just send them to me :-) (I am part of the linux-hardening group myself, and work closely with Kees) > I have taken the liberty of adding a TODO for the future, the actual > refactor can follow at some point down the line. > --- > drivers/firmware/efi/efi-pstore.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c > index e7b9ec6f8a86..f0ceb5702d21 100644 > --- a/drivers/firmware/efi/efi-pstore.c > +++ b/drivers/firmware/efi/efi-pstore.c > @@ -135,7 +135,15 @@ static ssize_t efi_pstore_read(struct pstore_record *record) > efi_status_t status; > > for (;;) { > - varname_size = 1024; > + /* > + * A small set of old UEFI implementations reject sizes > + * above a certain threshold, the lowest seen in the wild > + * is 512. > + * > + * TODO: Commonize with the iteration implementation in > + * fs/efivarfs to keep all the quirks in one place. > + */ > + varname_size = 512; > > /* > * If this is the first read() call in the pstore enumeration, > -- > 2.44.0 >
On 15.03.24 10:16, Ard Biesheuvel wrote: > Hi Tim, > > On Fri, 15 Mar 2024 at 01:27, Tim Schumacher <timschumi@gmx.de> wrote: >> >> Work around a quirk in a few old (2011-ish) UEFI implementations, where >> a call to `GetNextVariableName` with a buffer size larger than 512 bytes >> will always return EFI_INVALID_PARAMETER. >> >> This was already done to efivarfs in f45812cc23fb ("efivarfs: Request at >> most 512 bytes for variable names"), but the second copy of the variable >> iteration implementation was overlooked. >> >> Signed-off-by: Tim Schumacher <timschumi@gmx.de> > > Thanks for the patch. I'll take it as a fix. > > As an aside, you really want to avoid EFI pstore in general, and > specifically on such old systems with quirky UEFI implementations. > I found this by chance while looking for appearances of the magic value of 1024, and decided to split it out because this would have been the only change that modifies behavior. I didn't intend to actually use it after fixing it up, although I did make sure that it now does more than it did previously. If we can save someone a confused "Why is this done differently here?" (and have a reason to boil down the code to a single implementation in the future), then that is probably good enough on its own.
On 15/03/2024 06:16, Ard Biesheuvel wrote: > [...] > As an aside, you really want to avoid EFI pstore in general, and > specifically on such old systems with quirky UEFI implementations. > Hi Ard, this comment made me very curious; apart from old quirky UEFI implementations, what's the reason you see to avoid using efi-pstore in general ? Thanks in advance for your insights! Cheers, Guilherme
On Fri, 15 Mar 2024 at 21:46, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote: > > On 15/03/2024 06:16, Ard Biesheuvel wrote: > > [...] > > As an aside, you really want to avoid EFI pstore in general, and > > specifically on such old systems with quirky UEFI implementations. > > > > Hi Ard, this comment made me very curious; apart from old quirky UEFI > implementations, what's the reason you see to avoid using efi-pstore in > general ? > > Thanks in advance for your insights! I'm just not impressed by the general quality of implementations - relying on this when the system is going down is a reasonable last resort, perhaps, but if other options are available, I'd prioritize those. And this is for the oops/panic logs only - other uses of pstore seem better served with ordinary file based persistence.
On 29/03/2024 04:34, Ard Biesheuvel wrote: > [...] > > I'm just not impressed by the general quality of implementations - > relying on this when the system is going down is a reasonable last > resort, perhaps, but if other options are available, I'd prioritize > those. > > And this is for the oops/panic logs only - other uses of pstore seem > better served with ordinary file based persistence. OK, I understand now. Thanks, Guilherme
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index e7b9ec6f8a86..f0ceb5702d21 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -135,7 +135,15 @@ static ssize_t efi_pstore_read(struct pstore_record *record) efi_status_t status; for (;;) { - varname_size = 1024; + /* + * A small set of old UEFI implementations reject sizes + * above a certain threshold, the lowest seen in the wild + * is 512. + * + * TODO: Commonize with the iteration implementation in + * fs/efivarfs to keep all the quirks in one place. + */ + varname_size = 512; /* * If this is the first read() call in the pstore enumeration,
Work around a quirk in a few old (2011-ish) UEFI implementations, where a call to `GetNextVariableName` with a buffer size larger than 512 bytes will always return EFI_INVALID_PARAMETER. This was already done to efivarfs in f45812cc23fb ("efivarfs: Request at most 512 bytes for variable names"), but the second copy of the variable iteration implementation was overlooked. Signed-off-by: Tim Schumacher <timschumi@gmx.de> --- I CC'd the pstore people and linux-hardening mailing list because get_maintainer.pl suggested to do so. Apologies in case this was the incorrect decision, this is a very non-pstore-specific patch after all. I have taken the liberty of adding a TODO for the future, the actual refactor can follow at some point down the line. --- drivers/firmware/efi/efi-pstore.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) -- 2.44.0