[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

[PATCH 2/3] efivarfs: Remove unused internal struct members

[Tim Schumacher]

The structure was moved to the efivarfs internals in commit 2d82e6227ea1
("efi: vars: Move efivar caching layer into efivarfs") after previously
being used as the data ABI for efivars until its removal in commit
0f5b2c69a4cb ("efi: vars: Remove deprecated 'efivars' sysfs interface").

As efivarfs only uses the structure for the variable name caching layer,
the data-related members were never in use. Remove them to avoid
implying that efivarfs is bound by the same restrictions that efivars
once had.

Since we are changing the last copy of "struct efi_variable", document
the former layout in the ABI documentation of /sys/firmware/efi/vars
that is still left over.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
I'm unsure if this is how documentation of removed interfaces is/should
be handled, input on this would be greatly appreciated. Of course, the
alternative to what I did here is to remove the documentation
completely. If someone is running a kernel old enough to have this
interface, then the matching kernel source will still contain said
documentation.
---
 Documentation/ABI/stable/sysfs-firmware-efi-vars | 12 ++++++++++--
 fs/efivarfs/internal.h                           |  3 ---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-firmware-efi-vars b/Documentation/ABI/stable/sysfs-firmware-efi-vars
index 46ccd233e359..461b9139cedb 100644
--- a/Documentation/ABI/stable/sysfs-firmware-efi-vars
+++ b/Documentation/ABI/stable/sysfs-firmware-efi-vars
@@ -41,8 +41,16 @@ Description:
 		raw_var:	A binary file that can be read to obtain
 				a structure that contains everything
 				there is to know about the variable.
-				For structure definition see "struct
-				efi_variable" in the kernel sources.
+
+				The structure is defined as follows:
+				struct efi_variable {
+					efi_char16_t VariableName[512];
+					efi_guid_t VendorGuid;
+					unsigned long DataSize;
+					__u8 Data[1024];
+					efi_status_t Status;
+					__u32 Attributes;
+				} __attribute__((packed));

 				This file can also be written to in
 				order to update the value of a variable.
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index f7206158ee81..971560a01320 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -24,9 +24,6 @@ struct efivarfs_fs_info {
 struct efi_variable {
 	efi_char16_t  VariableName[EFI_VAR_NAME_LEN/sizeof(efi_char16_t)];
 	efi_guid_t    VendorGuid;
-	unsigned long DataSize;
-	__u8          Data[1024];
-	efi_status_t  Status;
 	__u32         Attributes;
 } __attribute__((packed));

--
2.44.0

Re: [PATCH 2/3] efivarfs: Remove unused internal struct members

[Ard Biesheuvel]

On Fri, 15 Mar 2024 at 01:27, Tim Schumacher <timschumi@gmx.de> wrote:
>
> The structure was moved to the efivarfs internals in commit 2d82e6227ea1
> ("efi: vars: Move efivar caching layer into efivarfs") after previously
> being used as the data ABI for efivars until its removal in commit
> 0f5b2c69a4cb ("efi: vars: Remove deprecated 'efivars' sysfs interface").
>
> As efivarfs only uses the structure for the variable name caching layer,
> the data-related members were never in use. Remove them to avoid
> implying that efivarfs is bound by the same restrictions that efivars
> once had.
>
> Since we are changing the last copy of "struct efi_variable", document
> the former layout in the ABI documentation of /sys/firmware/efi/vars
> that is still left over.
>
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
> ---
> I'm unsure if this is how documentation of removed interfaces is/should
> be handled, input on this would be greatly appreciated. Of course, the
> alternative to what I did here is to remove the documentation
> completely. If someone is running a kernel old enough to have this
> interface, then the matching kernel source will still contain said
> documentation.
> ---
>  Documentation/ABI/stable/sysfs-firmware-efi-vars | 12 ++++++++++--
>  fs/efivarfs/internal.h                           |  3 ---
>  2 files changed, 10 insertions(+), 5 deletions(-)
>

Please just rip out the doc (but in a separate patch and cc the
Documentation maintainers)


> diff --git a/Documentation/ABI/stable/sysfs-firmware-efi-vars b/Documentation/ABI/stable/sysfs-firmware-efi-vars
> index 46ccd233e359..461b9139cedb 100644
> --- a/Documentation/ABI/stable/sysfs-firmware-efi-vars
> +++ b/Documentation/ABI/stable/sysfs-firmware-efi-vars
> @@ -41,8 +41,16 @@ Description:
>                 raw_var:        A binary file that can be read to obtain
>                                 a structure that contains everything
>                                 there is to know about the variable.
> -                               For structure definition see "struct
> -                               efi_variable" in the kernel sources.
> +
> +                               The structure is defined as follows:
> +                               struct efi_variable {
> +                                       efi_char16_t VariableName[512];
> +                                       efi_guid_t VendorGuid;
> +                                       unsigned long DataSize;
> +                                       __u8 Data[1024];
> +                                       efi_status_t Status;
> +                                       __u32 Attributes;
> +                               } __attribute__((packed));
>
>                                 This file can also be written to in
>                                 order to update the value of a variable.
> diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
> index f7206158ee81..971560a01320 100644
> --- a/fs/efivarfs/internal.h
> +++ b/fs/efivarfs/internal.h
> @@ -24,9 +24,6 @@ struct efivarfs_fs_info {
>  struct efi_variable {
>         efi_char16_t  VariableName[EFI_VAR_NAME_LEN/sizeof(efi_char16_t)];
>         efi_guid_t    VendorGuid;
> -       unsigned long DataSize;
> -       __u8          Data[1024];
> -       efi_status_t  Status;
>         __u32         Attributes;
>  } __attribute__((packed));
>

I suppose we can drop the packed attribute too, given that this is no
longer external ABI.

Re: [PATCH 2/3] efivarfs: Remove unused internal struct members

[Tim Schumacher]

On 15.03.24 10:19, Ard Biesheuvel wrote:
> On Fri, 15 Mar 2024 at 01:27, Tim Schumacher <timschumi@gmx.de> wrote:
>>
>> The structure was moved to the efivarfs internals in commit 2d82e6227ea1
>> ("efi: vars: Move efivar caching layer into efivarfs") after previously
>> being used as the data ABI for efivars until its removal in commit
>> 0f5b2c69a4cb ("efi: vars: Remove deprecated 'efivars' sysfs interface").
>>
>> As efivarfs only uses the structure for the variable name caching layer,
>> the data-related members were never in use. Remove them to avoid
>> implying that efivarfs is bound by the same restrictions that efivars
>> once had.
>>
>> Since we are changing the last copy of "struct efi_variable", document
>> the former layout in the ABI documentation of /sys/firmware/efi/vars
>> that is still left over.
>>
>> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
>> ---
>> I'm unsure if this is how documentation of removed interfaces is/should
>> be handled, input on this would be greatly appreciated. Of course, the
>> alternative to what I did here is to remove the documentation
>> completely. If someone is running a kernel old enough to have this
>> interface, then the matching kernel source will still contain said
>> documentation.
>> ---
>>   Documentation/ABI/stable/sysfs-firmware-efi-vars | 12 ++++++++++--
>>   fs/efivarfs/internal.h                           |  3 ---
>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>
>
> Please just rip out the doc (but in a separate patch and cc the
> Documentation maintainers)

It appears that Documentation/ABI is explicitly excluded by the Documentation
maintainers. Scrolling through MAINTAINERS seems to imply that ABI documentation
is usually maintained by the relevant subsystem, and the docs-next tree indeed
does not currently carry any ABI documentation changes. A previous attempt to
remove the documentation [1] also yielded no response.

On that note, I found that documentation on removed ABI functionality is generally
boiled down to a deprecation notice (or kept as-is) and gets moved to
`Documentation/ABI/removed` instead. I can't find where the efivars interface has
been declared deprecated, so for the purpose of a deprecation notice I'll assume
that the "lock to Intel architectures" commit [2] was that point in time.

[1] https://lore.kernel.org/lkml/20230123081905.27283-1-johan+linaro@kernel.org/
[2] https://lore.kernel.org/all/20200923161404.17811-1-ardb@kernel.org/

[PATCH 3/3] efi: Clear up misconceptions about a maximum variable name size

[Tim Schumacher]

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

Re: [PATCH 3/3] efi: Clear up misconceptions about a maximum variable name size

[Ard Biesheuvel]

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
>

Re: [PATCH 3/3] efi: Clear up misconceptions about a maximum variable name size

[Tim Schumacher]

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.

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

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

[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

[PATCH v2 2/4] Documentation: Mark the 'efivars' sysfs interface as removed

[Tim Schumacher]

The 'efivars' sysfs interface was removed in commit 0f5b2c69a4cb ("efi:
vars: Remove deprecated 'efivars' sysfs interface"), but the ABI
documentation was not updated properly.

Strip down the documentation file for /sys/firmware/efi/vars to a very
basic description of what the interface was about, add a section about
the rough removal timeline, and inform the reader about the intended
replacement.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
Changes from v1 (semi-non-existing):
 - Removed large parts of the now unneeded description, as well as the
   contact.
 - Added a short documentation about the deprecation, removal, and
   replacement.
 - Split out into separate patch as per request, part of a patch chain.
---
 .../ABI/removed/sysfs-firmware-efi-vars       | 12 +++
 .../ABI/stable/sysfs-firmware-efi-vars        | 79 -------------------
 2 files changed, 12 insertions(+), 79 deletions(-)
 create mode 100644 Documentation/ABI/removed/sysfs-firmware-efi-vars
 delete mode 100644 Documentation/ABI/stable/sysfs-firmware-efi-vars

diff --git a/Documentation/ABI/removed/sysfs-firmware-efi-vars b/Documentation/ABI/removed/sysfs-firmware-efi-vars
new file mode 100644
index 0000000000000..8d97368b149bb
--- /dev/null
+++ b/Documentation/ABI/removed/sysfs-firmware-efi-vars
@@ -0,0 +1,12 @@
+What:		/sys/firmware/efi/vars
+Date:		April 2004, removed March 2023
+Description:
+		This directory exposed interfaces for interacting with
+		EFI variables.  For more information on EFI variables,
+		see 'Variable Services' in the UEFI specification
+		(section 7.2 in specification version 2.3 Errata D).
+
+		The 'efivars' sysfs interface was removed in March of 2023,
+		after being considered deprecated no later than September
+		of 2020. Its functionality has been replaced by the
+		'efivarfs' filesystem.
diff --git a/Documentation/ABI/stable/sysfs-firmware-efi-vars b/Documentation/ABI/stable/sysfs-firmware-efi-vars
deleted file mode 100644
index 46ccd233e3594..0000000000000
--- a/Documentation/ABI/stable/sysfs-firmware-efi-vars
+++ /dev/null
@@ -1,79 +0,0 @@
-What:		/sys/firmware/efi/vars
-Date:		April 2004
-Contact:	Matt Domsch <Matt_Domsch@dell.com>
-Description:
-		This directory exposes interfaces for interactive with
-		EFI variables.  For more information on EFI variables,
-		see 'Variable Services' in the UEFI specification
-		(section 7.2 in specification version 2.3 Errata D).
-
-		In summary, EFI variables are named, and are classified
-		into separate namespaces through the use of a vendor
-		GUID.  They also have an arbitrary binary value
-		associated with them.
-
-		The efivars module enumerates these variables and
-		creates a separate directory for each one found.  Each
-		directory has a name of the form "<key>-<vendor guid>"
-		and contains the following files:
-
-		=============== ========================================
-		attributes:	A read-only text file enumerating the
-				EFI variable flags.  Potential values
-				include:
-
-				EFI_VARIABLE_NON_VOLATILE
-				EFI_VARIABLE_BOOTSERVICE_ACCESS
-				EFI_VARIABLE_RUNTIME_ACCESS
-				EFI_VARIABLE_HARDWARE_ERROR_RECORD
-				EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS
-
-				See the EFI documentation for an
-				explanation of each of these variables.
-
-		data:		A read-only binary file that can be read
-				to attain the value of the EFI variable
-
-		guid:		The vendor GUID of the variable.  This
-				should always match the GUID in the
-				variable's name.
-
-		raw_var:	A binary file that can be read to obtain
-				a structure that contains everything
-				there is to know about the variable.
-				For structure definition see "struct
-				efi_variable" in the kernel sources.
-
-				This file can also be written to in
-				order to update the value of a variable.
-				For this to work however, all fields of
-				the "struct efi_variable" passed must
-				match byte for byte with the structure
-				read out of the file, save for the value
-				portion.
-
-				**Note** the efi_variable structure
-				read/written with this file contains a
-				'long' type that may change widths
-				depending on your underlying
-				architecture.
-
-		size:		As ASCII representation of the size of
-				the variable's value.
-		=============== ========================================
-
-
-		In addition, two other magic binary files are provided
-		in the top-level directory and are used for adding and
-		removing variables:
-
-		=============== ========================================
-		new_var:	Takes a "struct efi_variable" and
-				instructs the EFI firmware to create a
-				new variable.
-
-		del_var:	Takes a "struct efi_variable" and
-				instructs the EFI firmware to remove any
-				variable that has a matching vendor GUID
-				and variable key name.
-		=============== ========================================
--
2.44.0

[PATCH v2 3/4] efivarfs: Remove unused internal struct members

[Tim Schumacher]

The structure was moved to the efivarfs internals in commit 2d82e6227ea1
("efi: vars: Move efivar caching layer into efivarfs") after previously
being used as the data ABI for efivars until its removal in commit
0f5b2c69a4cb ("efi: vars: Remove deprecated 'efivars' sysfs interface").

As efivarfs only uses the structure for the variable name caching layer,
the data-related members were never in use. Remove them to avoid
implying that efivarfs is bound by the same restrictions that efivars
once had. While at it, remove the packed attribute, since we no longer
have to guarantee a stable layout.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
Changes from v1:
 - Split out documentation changes into a separate patch
 - Remove the packed attribute of the struct
---
 fs/efivarfs/internal.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index f7206158ee813..d71d2e08422f0 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -24,11 +24,8 @@ struct efivarfs_fs_info {
 struct efi_variable {
 	efi_char16_t  VariableName[EFI_VAR_NAME_LEN/sizeof(efi_char16_t)];
 	efi_guid_t    VendorGuid;
-	unsigned long DataSize;
-	__u8          Data[1024];
-	efi_status_t  Status;
 	__u32         Attributes;
-} __attribute__((packed));
+};

 struct efivar_entry {
 	struct efi_variable var;
--
2.44.0

[PATCH v2 4/4] efi: Clear up misconceptions about a maximum variable name size

[Tim Schumacher]

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

Re: [PATCH v2 4/4] efi: Clear up misconceptions about a maximum variable name size

[Ard Biesheuvel]

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
>