diff mbox series

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

Message ID 20240315002616.422802-1-timschumi@gmx.de
State Superseded
Headers show
Series [1/3] efi: pstore: Request at most 512 bytes for variable names | expand

Commit Message

Tim Schumacher March 15, 2024, 12:25 a.m. UTC 
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

Comments

Ard Biesheuvel March 15, 2024, 9:16 a.m. UTC | #1 
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
>
Ard Biesheuvel March 15, 2024, 9:20 a.m. UTC | #2 
On Fri, 15 Mar 2024 at 01:27, Tim Schumacher <timschumi@gmx.de> wrote:
>
> The UEFI specification does not make any mention of a maximum variable
> name size, so the headers and implementation shouldn't claim that one
> exists either.
>
> Comments referring to this limit have been removed or rewritten, as this
> is an implementation detail local to the Linux kernel.
>
> Where appropriate, the magic value of 1024 has been replaced with
> EFI_VAR_NAME_LEN, as this is used for the efi_variable struct
> definition. This in itself does not change any behavior, but should
> serve as points of interest when making future changes in the same area.
>
> A related build-time check has been added to ensure that the special
> 512 byte sized buffer will not overflow with a potentially decreased
> EFI_VAR_NAME_LEN.
>
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>

Shouldn't we switch to 512 everywhere while at it?

> ---
>  drivers/firmware/efi/vars.c | 2 +-
>  fs/efivarfs/vars.c          | 5 +++--
>  include/linux/efi.h         | 9 ++++-----
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index f654e6f6af87..4056ba7f3440 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -215,7 +215,7 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
>
>         if (data_size > 0) {
>                 status = check_var_size(nonblocking, attr,
> -                                       data_size + ucs2_strsize(name, 1024));
> +                                       data_size + ucs2_strsize(name, EFI_VAR_NAME_LEN));
>                 if (status != EFI_SUCCESS)
>                         return status;
>         }
> diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
> index 4d722af1014f..3cc89bb624f0 100644
> --- a/fs/efivarfs/vars.c
> +++ b/fs/efivarfs/vars.c
> @@ -295,9 +295,9 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor,
>         unsigned long strsize1, strsize2;
>         bool found = false;
>
> -       strsize1 = ucs2_strsize(variable_name, 1024);
> +       strsize1 = ucs2_strsize(variable_name, EFI_VAR_NAME_LEN);
>         list_for_each_entry_safe(entry, n, head, list) {
> -               strsize2 = ucs2_strsize(entry->var.VariableName, 1024);
> +               strsize2 = ucs2_strsize(entry->var.VariableName, EFI_VAR_NAME_LEN);
>                 if (strsize1 == strsize2 &&
>                         !memcmp(variable_name, &(entry->var.VariableName),
>                                 strsize2) &&
> @@ -396,6 +396,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
>
>         do {
>                 variable_name_size = 512;
> +               BUILD_BUG_ON(EFI_VAR_NAME_LEN < 512);
>
>                 status = efivar_get_next_variable(&variable_name_size,
>                                                   variable_name,
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index c74f47711f0b..62f552057b06 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1065,12 +1065,11 @@ static inline u64 efivar_reserved_space(void) { return 0; }
>  #endif
>
>  /*
> - * The maximum size of VariableName + Data = 1024
> - * Therefore, it's reasonable to save that much
> - * space in each part of the structure,
> - * and we use a page for reading/writing.
> + * There is no actual upper limit specified for the variable name size.
> + *
> + * This limit exists only for practical purposes, since name conversions
> + * are bounds-checked and name data is occasionally stored in-line.
>   */
> -
>  #define EFI_VAR_NAME_LEN       1024
>
>  int efivars_register(struct efivars *efivars,
> --
> 2.44.0
>
Tim Schumacher March 15, 2024, 7:02 p.m. UTC | #3 
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.
Tim Schumacher March 15, 2024, 7:45 p.m. UTC | #4 
On 15.03.24 10:20, Ard Biesheuvel wrote:
> On Fri, 15 Mar 2024 at 01:27, Tim Schumacher <timschumi@gmx.de> wrote:
>>
>> The UEFI specification does not make any mention of a maximum variable
>> name size, so the headers and implementation shouldn't claim that one
>> exists either.
>>
>> Comments referring to this limit have been removed or rewritten, as this
>> is an implementation detail local to the Linux kernel.
>>
>> Where appropriate, the magic value of 1024 has been replaced with
>> EFI_VAR_NAME_LEN, as this is used for the efi_variable struct
>> definition. This in itself does not change any behavior, but should
>> serve as points of interest when making future changes in the same area.
>>
>> A related build-time check has been added to ensure that the special
>> 512 byte sized buffer will not overflow with a potentially decreased
>> EFI_VAR_NAME_LEN.
>>
>> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
>
> Shouldn't we switch to 512 everywhere while at it?
>

We probably could, but my general aim for this is to eventually get rid of a
predetermined data storage size anyways.

The only part that needs guesswork is the sizing of the buffer for the
initial read (be it either by estimating a constant or by doing a
challenge-response thing), after that we can just measure the string
once (with an upper bound at the buffer size, similar to what
`var_name_strnsize` already does) and hold on to that length going forward.

The variable name storage situation still isn't entirely clear, so I
just wanted to get rid of most of the magic numbers for now, and guarded the
one "this would lead to a memory corruption with changed values" case with
`BUILD_BUG_ON`. I don't consider the define to be freely changeable at the
moment, but in case it seems like a good idea to someone else, we can at
least save them from one potential headache.
Guilherme G. Piccoli March 15, 2024, 7:45 p.m. UTC | #5 
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
Ard Biesheuvel March 29, 2024, 7:34 a.m. UTC | #6 
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.
Ard Biesheuvel March 29, 2024, 7:42 a.m. UTC | #7 
On Thu, 28 Mar 2024 at 22:51, Tim Schumacher <timschumi@gmx.de> wrote:
>
> The UEFI specification does not make any mention of a maximum variable
> name size, so the headers and implementation shouldn't claim that one
> exists either.
>
> Comments referring to this limit have been removed or rewritten, as this
> is an implementation detail local to the Linux kernel.
>
> Where appropriate, the magic value of 1024 has been replaced with
> EFI_VAR_NAME_LEN, as this is used for the efi_variable struct
> definition. This in itself does not change any behavior, but should
> serve as points of interest when making future changes in the same area.
>
> A related build-time check has been added to ensure that the special
> 512 byte sized buffer will not overflow with a potentially decreased
> EFI_VAR_NAME_LEN.
>
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>

Thanks for resending. I've queued these up now.

> ---
> Changes from v1:
>  - None, resubmitted as part of a patch chain
> ---
>  drivers/firmware/efi/vars.c | 2 +-
>  fs/efivarfs/vars.c          | 5 +++--
>  include/linux/efi.h         | 9 ++++-----
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index f654e6f6af873..4056ba7f34408 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -215,7 +215,7 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
>
>         if (data_size > 0) {
>                 status = check_var_size(nonblocking, attr,
> -                                       data_size + ucs2_strsize(name, 1024));
> +                                       data_size + ucs2_strsize(name, EFI_VAR_NAME_LEN));
>                 if (status != EFI_SUCCESS)
>                         return status;
>         }
> diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
> index 4d722af1014f2..3cc89bb624f07 100644
> --- a/fs/efivarfs/vars.c
> +++ b/fs/efivarfs/vars.c
> @@ -295,9 +295,9 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor,
>         unsigned long strsize1, strsize2;
>         bool found = false;
>
> -       strsize1 = ucs2_strsize(variable_name, 1024);
> +       strsize1 = ucs2_strsize(variable_name, EFI_VAR_NAME_LEN);
>         list_for_each_entry_safe(entry, n, head, list) {
> -               strsize2 = ucs2_strsize(entry->var.VariableName, 1024);
> +               strsize2 = ucs2_strsize(entry->var.VariableName, EFI_VAR_NAME_LEN);
>                 if (strsize1 == strsize2 &&
>                         !memcmp(variable_name, &(entry->var.VariableName),
>                                 strsize2) &&
> @@ -396,6 +396,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
>
>         do {
>                 variable_name_size = 512;
> +               BUILD_BUG_ON(EFI_VAR_NAME_LEN < 512);
>
>                 status = efivar_get_next_variable(&variable_name_size,
>                                                   variable_name,
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index d59b0947fba08..418e555459da7 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1072,12 +1072,11 @@ static inline u64 efivar_reserved_space(void) { return 0; }
>  #endif
>
>  /*
> - * The maximum size of VariableName + Data = 1024
> - * Therefore, it's reasonable to save that much
> - * space in each part of the structure,
> - * and we use a page for reading/writing.
> + * There is no actual upper limit specified for the variable name size.
> + *
> + * This limit exists only for practical purposes, since name conversions
> + * are bounds-checked and name data is occasionally stored in-line.
>   */
> -
>  #define EFI_VAR_NAME_LEN       1024
>
>  int efivars_register(struct efivars *efivars,
> --
> 2.44.0
>
Guilherme G. Piccoli March 29, 2024, 9:32 p.m. UTC | #8 
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 mbox series

Patch

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,