[PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

[PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

[Tim Schumacher]

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(-)

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

Re: [PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

[Ard Biesheuvel]

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
>

Re: [PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

[Tim Schumacher]

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.

Re: [PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

[Guilherme G. Piccoli]

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

[PATCH v2 1/4] efi: pstore: Request at most 512 bytes for variable names

[Tim Schumacher]

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 commit 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>
---
Changes from v1:
 - None, resubmitted as a part of a chain.
---
 drivers/firmware/efi/efi-pstore.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 833cbb995dd3f..5b9dc26e6bcb9 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -162,7 +162,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

Re: [PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

[Ard Biesheuvel]

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.

Re: [PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

[Guilherme G. Piccoli]

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