This sidesteps 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`.

It is currently unknown whether this is just a botched check or if the
length is interpreted differently, so the underlying buffer is still
sized for 1024 bytes, even if we communicate a smaller size to the
runtime service.

Cc: stable@vger.kernel.org # 6.1+
Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
linux-stable is CC'd because this issue (together with a few other
unfortunate non-kernel edge-cases) resulted in semi-bricked machines
when installing various Linux distributions across the last 10+ years.

The short explanation is that efibootmgr created an entirely new list
of boot options when not being able to read the existing one, which
wiped the device-based entries from `BootOrder` and overwrote the "BIOS
Setup" entry that was stored in `Boot0000`, making the setup menu
completely inaccessible on the hardware that I'm testing on.

Matching bug reports exist for Ubuntu [1] and Debian [2], they just
don't mention the root issue because I finished tracking this down only
yesterday.

As mentioned in the commit message and comment, the reason for rejecting
buffers larger than 512 bytes is still unknown, but I plan to continue
looking at the UEFI binary once I have a bit more time. Depending on the
results of that, the accommodations we make here could be toned down
again.

[1] https://bugs.launchpad.net/ubuntu/+source/efibootmgr/+bug/1082418
[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=887139
---
 fs/efivarfs/vars.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
index 9e4f47808bd5..55a1468af3fa 100644
--- a/fs/efivarfs/vars.c
+++ b/fs/efivarfs/vars.c
@@ -372,13 +372,15 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
 int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		void *data, bool duplicates, struct list_head *head)
 {
-	unsigned long variable_name_size = 1024;
+	unsigned long variable_name_allocation_size = 1024;
+	unsigned long variable_name_advertised_size = 512;
 	efi_char16_t *variable_name;
+	efi_char16_t *variable_name_tmp;
 	efi_status_t status;
 	efi_guid_t vendor_guid;
 	int err = 0;

-	variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+	variable_name = kzalloc(variable_name_allocation_size, GFP_KERNEL);
 	if (!variable_name) {
 		printk(KERN_ERR "efivars: Memory allocation failed.\n");
 		return -ENOMEM;
@@ -391,10 +393,16 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
 	 * the variable name and variable data is 1024 bytes.
+	 *
+	 * However, a small set of old UEFI implementations
+	 * reject large sizes, so we start off with advertising
+	 * something that is more digestible. The underlying
+	 * buffer is still 1024 bytes in size, in case the implementation
+	 * interprets the size differently.
 	 */

 	do {
-		variable_name_size = 1024;
+		unsigned long variable_name_size = variable_name_advertised_size;

 		status = efivar_get_next_variable(&variable_name_size,
 						  variable_name,
@@ -431,6 +439,22 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			break;
 		case EFI_NOT_FOUND:
 			break;
+		case EFI_BUFFER_TOO_SMALL:
+			variable_name_advertised_size = variable_name_size;
+			if (variable_name_size <= variable_name_allocation_size)
+				break;
+
+			variable_name_tmp = krealloc(variable_name, variable_name_size, GFP_KERNEL);
+
+			if (!variable_name_tmp) {
+				printk(KERN_ERR "efivars: Memory reallocation failed.\n");
+				err = -ENOMEM;
+				status = EFI_NOT_FOUND;
+				break;
+			}
+			variable_name = variable_name_tmp;
+			variable_name_allocation_size = variable_name_size;
+			break;
 		default:
 			printk(KERN_WARNING "efivars: get_next_variable: status=%lx\n",
 				status);
--
2.43.0

On Tue, 23 Jan 2024 at 00:15, Tim Schumacher <timschumi@gmx.de> wrote:
>
> This sidesteps 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`.
>
> It is currently unknown whether this is just a botched check or if the
> length is interpreted differently, so the underlying buffer is still
> sized for 1024 bytes, even if we communicate a smaller size to the
> runtime service.
>
> Cc: stable@vger.kernel.org # 6.1+
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>

Hello Tim,

I wonder if we might just reduce this to 512 and be done with it.
Presumably, Windows boots fine in UEFI mode on these machines, which
suggests that it passes a value <= 512 too, and I don't recall ever
encountering systems using extremely long variable names (i.e., longer
than 512 byte)

On 23.01.24 12:24, Ard Biesheuvel wrote:
> On Tue, 23 Jan 2024 at 00:15, Tim Schumacher <timschumi@gmx.de> wrote:
>>
>> This sidesteps 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`.
>
> I wonder if we might just reduce this to 512 and be done with it.
> Presumably, Windows boots fine in UEFI mode on these machines, which
> suggests that it passes a value <= 512 too, and I don't recall ever
> encountering systems using extremely long variable names (i.e., longer
> than 512 byte)

I'd rather avoid introducing deviations from the specifications on the
kernel side as well. Someone or something might legitimately set a large
variable name, so we'd have to have resize logic anyways (to resize from
512 to 512+). Also, as mentioned on the patch, I'm entirely unsure what
the size ends up being used for, so I'd rather err on the side of
caution (most importantly in regards to the buffer size).

Windows _does_ boot fine (and is able to read all the variables), so
they presumably start off with 512 or smaller. FreeBSD definitely starts
from 512, but they also implement additional resize logic.

In regards to complexity of the proposed solution, how about we approach
this from the other side? Start off with advertising 1024 bytes of
buffer storage, and cut that value in half (without actually resizing
the buffer) as long as we get `EFI_INVALID_PARAMETER` while on the first
run.

If we ever get `EFI_BUFFER_TOO_SMALL`, we know that something is wrong
with the UEFI implementation (because that either means that something
claims to be larger than 1024 bytes, or that our assumptions about the
quirk don't hold up) and can bail out and log as appropriate. That would
limit the complexity to the machines that need it, completely omit the
need for resize logic, and would still be specification compliant.

On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <timschumi@gmx.de> wrote:
>
> On 23.01.24 12:24, Ard Biesheuvel wrote:
> > On Tue, 23 Jan 2024 at 00:15, Tim Schumacher <timschumi@gmx.de> wrote:
> >>
> >> This sidesteps 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`.
> >
> > I wonder if we might just reduce this to 512 and be done with it.
> > Presumably, Windows boots fine in UEFI mode on these machines, which
> > suggests that it passes a value <= 512 too, and I don't recall ever
> > encountering systems using extremely long variable names (i.e., longer
> > than 512 byte)
>
> I'd rather avoid introducing deviations from the specifications on the
> kernel side as well.

Which specification would this deviate from?

> Someone or something might legitimately set a large
> variable name, so we'd have to have resize logic anyways (to resize from
> 512 to 512+). Also, as mentioned on the patch, I'm entirely unsure what
> the size ends up being used for, so I'd rather err on the side of
> caution (most importantly in regards to the buffer size).
>
> Windows _does_ boot fine (and is able to read all the variables), so
> they presumably start off with 512 or smaller. FreeBSD definitely starts
> from 512, but they also implement additional resize logic.
>
> In regards to complexity of the proposed solution, how about we approach
> this from the other side? Start off with advertising 1024 bytes of
> buffer storage, and cut that value in half (without actually resizing
> the buffer) as long as we get `EFI_INVALID_PARAMETER` while on the first
> run.
>
> If we ever get `EFI_BUFFER_TOO_SMALL`, we know that something is wrong
> with the UEFI implementation (because that either means that something
> claims to be larger than 1024 bytes, or that our assumptions about the
> quirk don't hold up) and can bail out and log as appropriate. That would
> limit the complexity to the machines that need it, completely omit the
> need for resize logic, and would still be specification compliant.

Yes, I would prefer to keep this as simple as possible.

On 23.01.24 15:09, Ard Biesheuvel wrote:
> On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <timschumi@gmx.de> wrote:
>>
>> I'd rather avoid introducing deviations from the specifications on the
>> kernel side as well.
>
> Which specification would this deviate from?

The preexisting comment claims "Per EFI spec", and it appears that I got
mislead by that. Neither the UEFI specification, nor the newest revision
of the EFI specification (which I guess is what would have been current
back in 2004, when this comment was introduced) seem to make any mention
of a maximum length for the variable name.

I'll look into updating it appropriately when doing my changes (or remove
it entirely, since it seems to serve no other purpose than justifying the
starting buffer size).

>> In regards to complexity of the proposed solution, how about we approach
>> this from the other side? Start off with advertising 1024 bytes of
>> buffer storage, and cut that value in half (without actually resizing
>> the buffer) as long as we get `EFI_INVALID_PARAMETER` while on the first
>> run.
>>
>> If we ever get `EFI_BUFFER_TOO_SMALL`, we know that something is wrong
>> with the UEFI implementation (because that either means that something
>> claims to be larger than 1024 bytes, or that our assumptions about the
>> quirk don't hold up) and can bail out and log as appropriate. That would
>> limit the complexity to the machines that need it, completely omit the
>> need for resize logic, and would still be specification compliant.
>
> Yes, I would prefer to keep this as simple as possible.

I'll prepare a v2 with those changes then. The 1024 bytes may not be
an actual limit, but I'll keep it as the default size for the second
revision to ensure that we don't break any existing setups.

If this is still considered not simple enough, we can go back to looking
at just doing s/1024/512/ for the static buffer size.

Thank you for the feedback, I greatly appreciate it.

Tim

PS: Apologies in case my previous message ended up with a messed up line
wrap. Thunderbird apparently shows the message with automatic line wrap
while composing, but doesn't actually send it like that (or lines are
magically unwrapped again when displaying the message afterwards).

On Tue, Jan 23, 2024 at 12:33 PM Tim Schumacher <timschumi@gmx.de> wrote:
>
> On 23.01.24 15:09, Ard Biesheuvel wrote:
> > On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <timschumi@gmx.de> wrote:
> >>
> >> I'd rather avoid introducing deviations from the specifications on the
> >> kernel side as well.
> >
> > Which specification would this deviate from?
>
> The preexisting comment claims "Per EFI spec", and it appears that I got
> mislead by that. Neither the UEFI specification, nor the newest revision
> of the EFI specification (which I guess is what would have been current
> back in 2004, when this comment was introduced) seem to make any mention
> of a maximum length for the variable name.

Curiously, I can't find it in the 1.02 spec (the oldest I can find)
either.  When I inherited efibootmgr around 2013, this was a
limitation there, but I don't see anything in that tree that claims
it's a spec limitation either.  My suspicion is this is a former
Itanium firmware limit that got promoted to "the spec says" by word of
mouth, or was in some very early ia64 implementation spec.

-- Peter

On 24.01.24 22:25, Peter Jones wrote:
> On Tue, Jan 23, 2024 at 12:33 PM Tim Schumacher <timschumi@gmx.de> wrote:
>>
>> On 23.01.24 15:09, Ard Biesheuvel wrote:
>>> On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <timschumi@gmx.de> wrote:
>>>>
>>>> I'd rather avoid introducing deviations from the specifications on the
>>>> kernel side as well.
>>>
>>> Which specification would this deviate from?
>>
>> The preexisting comment claims "Per EFI spec", and it appears that I got
>> mislead by that. Neither the UEFI specification, nor the newest revision
>> of the EFI specification (which I guess is what would have been current
>> back in 2004, when this comment was introduced) seem to make any mention
>> of a maximum length for the variable name.
>
> Curiously, I can't find it in the 1.02 spec (the oldest I can find)
> either.  When I inherited efibootmgr around 2013, this was a
> limitation there, but I don't see anything in that tree that claims
> it's a spec limitation either.  My suspicion is this is a former
> Itanium firmware limit that got promoted to "the spec says" by word of
> mouth, or was in some very early ia64 implementation spec.

In case anyone is still curious about this, I managed to track down where
the supposed limit actually came from.

The efivarfs documentation claims that "The old sysfs EFI variables code only
supported variables of up to 1024 bytes. This limitation existed in version
0.99 of the EFI specification, but was removed before any full releases."

With some effort I managed to track down a copy of EFI v0.99 [1], which
indeed says the following:

"The size of the VariableName, including the Unicode Null in bytes plus the
  DataSize is limited to a maximum size of 1024 bytes."

This note was there at least in version 0.92 and 0.99, and gone in at least
version 1.02. I haven't been able to find a copy of version 1.01, but it most
likely never even existed online, given that 1.02 happened only 12 days later
(and for the sole reason of "legal and trademarking requirements").
The EFI 0.99 errata (which might have included more details) sadly doesn't seem
to have been backed up anywhere by third-parties.

Tim

[1] Searching for "EFISpec_V099" on your preferred search engine should
     find it. I doubt that Intel will care about copyright assignments for
     feedback on 0.99 now, but the agreement prompt sadly prevented the Web
     Archive from reaching it.

On Wed, 24 Jan 2024 at 22:25, Peter Jones <pjones@redhat.com> wrote:
>
> On Tue, Jan 23, 2024 at 12:33 PM Tim Schumacher <timschumi@gmx.de> wrote:
> >
> > On 23.01.24 15:09, Ard Biesheuvel wrote:
> > > On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <timschumi@gmx.de> wrote:
> > >>
> > >> I'd rather avoid introducing deviations from the specifications on the
> > >> kernel side as well.
> > >
> > > Which specification would this deviate from?
> >
> > The preexisting comment claims "Per EFI spec", and it appears that I got
> > mislead by that. Neither the UEFI specification, nor the newest revision
> > of the EFI specification (which I guess is what would have been current
> > back in 2004, when this comment was introduced) seem to make any mention
> > of a maximum length for the variable name.
>
> Curiously, I can't find it in the 1.02 spec (the oldest I can find)
> either.  When I inherited efibootmgr around 2013, this was a
> limitation there, but I don't see anything in that tree that claims
> it's a spec limitation either.  My suspicion is this is a former
> Itanium firmware limit that got promoted to "the spec says" by word of
> mouth, or was in some very early ia64 implementation spec.
>

Also, the comment (and similar ones I've seen in the past) seem to
refer to the entire variable (name + payload) rather than just the
name.

So I am still leaning towards simply reducing this to 512 bytes.

This sidesteps 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`.

Additionally, remove a related but outdated comment that claims the
maximum variable name size to be 1024. The default buffer size of 1024
is kept to ensure that existing setups keep working.

Cc: stable@vger.kernel.org # 6.1+
Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
Changes since v1 ("efivarfs: Iterate variables with increasing name buffer sizes"):

- Redid the logic to start with the current limit of 1024 and continuously
  halve it until we get a successful result.
- Added a warning line in case we find anything that is bigger than the limit.
- Removed an outdated (or never accurate?) comment about a specification-imposed
  length limit.
---
 fs/efivarfs/vars.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
index 9e4f47808bd5..26a10ed4a116 100644
--- a/fs/efivarfs/vars.c
+++ b/fs/efivarfs/vars.c
@@ -372,13 +372,14 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
 int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		void *data, bool duplicates, struct list_head *head)
 {
-	unsigned long variable_name_size = 1024;
+	unsigned long maximum_variable_name_size = 1024;
 	efi_char16_t *variable_name;
 	efi_status_t status;
 	efi_guid_t vendor_guid;
 	int err = 0;
+	bool successful_once = false;

-	variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+	variable_name = kzalloc(maximum_variable_name_size, GFP_KERNEL);
 	if (!variable_name) {
 		printk(KERN_ERR "efivars: Memory allocation failed.\n");
 		return -ENOMEM;
@@ -388,19 +389,16 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 	if (err)
 		goto free;

-	/*
-	 * Per EFI spec, the maximum storage allocated for both
-	 * the variable name and variable data is 1024 bytes.
-	 */
-
 	do {
-		variable_name_size = 1024;
+		unsigned long variable_name_size = maximum_variable_name_size;

 		status = efivar_get_next_variable(&variable_name_size,
 						  variable_name,
 						  &vendor_guid);
 		switch (status) {
 		case EFI_SUCCESS:
+			successful_once = true;
+
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);

@@ -431,6 +429,23 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			break;
 		case EFI_NOT_FOUND:
 			break;
+		case EFI_BUFFER_TOO_SMALL:
+			printk(KERN_WARNING "efivars: Assumed maximum name size to be 0x%lx, got name of size 0x%lx\n",
+			       maximum_variable_name_size, variable_name_size);
+			status = EFI_NOT_FOUND;
+			break;
+		case EFI_INVALID_PARAMETER:
+			if (!successful_once && maximum_variable_name_size > 1) {
+				/*
+				 * A small set of old UEFI implementations reject sizes
+				 * above a certain threshold. Halve the advertised size
+				 * until we get the first successful response.
+				 */
+				maximum_variable_name_size /= 2;
+				break;
+			}
+
+			fallthrough;
 		default:
 			printk(KERN_WARNING "efivars: get_next_variable: status=%lx\n",
 				status);
--
2.43.0

This sidesteps 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`.

Cc: stable@vger.kernel.org # 6.1+
Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
Made a patch with just the size change (and a comment / error message
for good measure) as requested. I just hope that I don't have to come
back in a month to find out that there is a machine that only accepts up
to (e.g.) 256 bytes.

One thing that I just recently noticed is that properly processing
variables above 512 bytes in size is currently meaningless anyways,
since the VFS layer only allows file name sizes of up to 255 bytes,
and 512 bytes of UCS2 will end up being at least 256 bytes of
UTF-8.

If path creation errors are decoupled from the iteration process then
one could at least skip those entries, but the efivarfs implementation
seems to be in no shape to handle that currently.
---
Changes since v1 ("efivarfs: Iterate variables with increasing name buffer sizes"):

- Redid the logic to start with the current limit of 1024 and continuously
  halve it until we get a successful result.
- Added a warning line in case we find anything that is bigger than the limit.
- Removed an outdated (or never accurate?) comment about a specification-imposed
  length limit.

Changes since v2 ("efivarfs: Halve name buffer size until first successful response"):

- Removed all the complicated logic, only keeping the new limit, the
  comment change, and the error message in case a big variable is found.
---
 fs/efivarfs/vars.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
index 9e4f47808bd5..3f6385f2a4e5 100644
--- a/fs/efivarfs/vars.c
+++ b/fs/efivarfs/vars.c
@@ -372,7 +372,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
 int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		void *data, bool duplicates, struct list_head *head)
 {
-	unsigned long variable_name_size = 1024;
+	unsigned long variable_name_size = 512;
 	efi_char16_t *variable_name;
 	efi_status_t status;
 	efi_guid_t vendor_guid;
@@ -389,12 +389,13 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		goto free;

 	/*
-	 * Per EFI spec, the maximum storage allocated for both
-	 * the variable name and variable data is 1024 bytes.
+	 * A small set of old UEFI implementations reject sizes
+	 * above a certain threshold, the lowest seen in the wild
+	 * is 512.
 	 */

 	do {
-		variable_name_size = 1024;
+		variable_name_size = 512;

 		status = efivar_get_next_variable(&variable_name_size,
 						  variable_name,
@@ -431,6 +432,11 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			break;
 		case EFI_NOT_FOUND:
 			break;
+		case EFI_BUFFER_TOO_SMALL:
+			printk(KERN_WARNING "efivars: Assumed maximum name size to be 512, got name of size %lu\n",
+			       variable_name_size);
+			status = EFI_NOT_FOUND;
+			break;
 		default:
 			printk(KERN_WARNING "efivars: get_next_variable: status=%lx\n",
 				status);
--
2.43.0

On Fri, 26 Jan 2024 at 17:25, Tim Schumacher <timschumi@gmx.de> wrote:
>
> This sidesteps 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`.
>
> Cc: stable@vger.kernel.org # 6.1+
> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
> ---
> Made a patch with just the size change (and a comment / error message
> for good measure) as requested. I just hope that I don't have to come
> back in a month to find out that there is a machine that only accepts up
> to (e.g.) 256 bytes.
>

How about we add

static int varname_max_size = 512;
module_param(varname_max_size, int, 0644);
MODULE_PARM_DESC(varname_max_size,
                "Set the maximum number of bytes to expect for variable names");

and use varname_max_size to initialize the malloc and the input argument?


> One thing that I just recently noticed is that properly processing
> variables above 512 bytes in size is currently meaningless anyways,
> since the VFS layer only allows file name sizes of up to 255 bytes,
> and 512 bytes of UCS2 will end up being at least 256 bytes of
> UTF-8.
>

Interesting. Let's add this to the commit log - it makes the case much
stronger, given that it proves that it is impossible for anyone to be
relying on the current maximum being over 512 bytes.

> If path creation errors are decoupled from the iteration process then
> one could at least skip those entries, but the efivarfs implementation
> seems to be in no shape to handle that currently.
> ---
> Changes since v1 ("efivarfs: Iterate variables with increasing name buffer sizes"):
>
> - Redid the logic to start with the current limit of 1024 and continuously
>   halve it until we get a successful result.
> - Added a warning line in case we find anything that is bigger than the limit.
> - Removed an outdated (or never accurate?) comment about a specification-imposed
>   length limit.
>
> Changes since v2 ("efivarfs: Halve name buffer size until first successful response"):
>
> - Removed all the complicated logic, only keeping the new limit, the
>   comment change, and the error message in case a big variable is found.
> ---
>  fs/efivarfs/vars.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
> index 9e4f47808bd5..3f6385f2a4e5 100644
> --- a/fs/efivarfs/vars.c
> +++ b/fs/efivarfs/vars.c
> @@ -372,7 +372,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
>  int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>                 void *data, bool duplicates, struct list_head *head)
>  {
> -       unsigned long variable_name_size = 1024;
> +       unsigned long variable_name_size = 512;
>         efi_char16_t *variable_name;
>         efi_status_t status;
>         efi_guid_t vendor_guid;
> @@ -389,12 +389,13 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>                 goto free;
>
>         /*
> -        * Per EFI spec, the maximum storage allocated for both
> -        * the variable name and variable data is 1024 bytes.
> +        * A small set of old UEFI implementations reject sizes
> +        * above a certain threshold, the lowest seen in the wild
> +        * is 512.
>          */
>
>         do {
> -               variable_name_size = 1024;
> +               variable_name_size = 512;
>
>                 status = efivar_get_next_variable(&variable_name_size,
>                                                   variable_name,
> @@ -431,6 +432,11 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>                         break;
>                 case EFI_NOT_FOUND:
>                         break;
> +               case EFI_BUFFER_TOO_SMALL:
> +                       printk(KERN_WARNING "efivars: Assumed maximum name size to be 512, got name of size %lu\n",
> +                              variable_name_size);
> +                       status = EFI_NOT_FOUND;
> +                       break;
>                 default:
>                         printk(KERN_WARNING "efivars: get_next_variable: status=%lx\n",
>                                 status);
> --
> 2.43.0
>

On 26.01.24 17:35, Ard Biesheuvel wrote:
> On Fri, 26 Jan 2024 at 17:25, Tim Schumacher <timschumi@gmx.de> wrote:
>
>> Made a patch with just the size change (and a comment / error message
>> for good measure) as requested. I just hope that I don't have to come
>> back in a month to find out that there is a machine that only accepts up
>> to (e.g.) 256 bytes.
>>
>
> How about we add
>
> static int varname_max_size = 512;
> module_param(varname_max_size, int, 0644);
> MODULE_PARM_DESC(varname_max_size,
>                  "Set the maximum number of bytes to expect for variable names");
>
> and use varname_max_size to initialize the malloc and the input argument?

What I'm most concerned about either way is the default setting, because
at the point where users will start investigating why their EFI variables
can't be read, their setup menu and other boot entries will be long gone
already.

Making this configurable would only make sense if we actually disallowed
write (/read?) accesses completely in case anything is wrong (i.e. more
data than we can handle, or a buggy UEFI that needs an even smaller size
to work). Then users would actually notice something is off instead of
just making them believe that there are no more variables.

Additionally, We have a bunch of misguided "source of truths" across the
whole stack (`EFI_VAR_NAME_LEN` for the structs, a whole second iteration
implementation for `efi_pstore_read`, etc.), making sure all of these match
each other is probably beyond the scope of this patch (but a requirement
for making this a proper user-configurable setting).

>> One thing that I just recently noticed is that properly processing
>> variables above 512 bytes in size is currently meaningless anyways,
>> since the VFS layer only allows file name sizes of up to 255 bytes,
>> and 512 bytes of UCS2 will end up being at least 256 bytes of
>> UTF-8.
>>
>
> Interesting. Let's add this to the commit log - it makes the case much
> stronger, given that it proves that it is impossible for anyone to be
> relying on the current maximum being over 512 bytes.

It makes the case much stronger for why one wouldn't be able to _create_
variables of that length from Linux userspace, creating dentries internally
seems to have different restrictions (or at least their name size seems
unlimited to me). Therefore, anything external could have still created
such variables, and such a variable will also affect any variable that
follows, not just itself. They don't have to be processed properly, but
they still need to be processed (and they currently aren't processed at all).

For what it's worth, I was able get the quirky UEFI implementation (the very
same that can't request variable names longer than 512 bytes) to attempt to
return a variable name of length greater than 512 to the kernel by creating
it through SetVariable beforehand.

I'm sure you already noticed, but I don't want to argue in favor of this
patches correctness more than it really is.

On 26.01.24 19:02, Tim Schumacher wrote:
> On 26.01.24 17:35, Ard Biesheuvel wrote:
>> On Fri, 26 Jan 2024 at 17:25, Tim Schumacher <timschumi@gmx.de> wrote:
>>
>>> One thing that I just recently noticed is that properly processing
>>> variables above 512 bytes in size is currently meaningless anyways,
>>> since the VFS layer only allows file name sizes of up to 255 bytes,
>>> and 512 bytes of UCS2 will end up being at least 256 bytes of
>>> UTF-8.
>>>
>>
>> Interesting. Let's add this to the commit log - it makes the case much
>> stronger, given that it proves that it is impossible for anyone to be
>> relying on the current maximum being over 512 bytes.
>
> It makes the case much stronger for why one wouldn't be able to _create_
> variables of that length from Linux userspace, creating dentries internally
> seems to have different restrictions (or at least their name size seems
> unlimited to me). Therefore, anything external could have still created
> such variables, and such a variable will also affect any variable that
> follows, not just itself. They don't have to be processed properly, but
> they still need to be processed (and they currently aren't processed at all).
>

I was able to experimentally confirm that creating dentries internally is
_not_ restricted by the value of NAME_MAX. The test setup was as follows:

- Build and boot a kernel with NAME_MAX bumped to an artificially high
   value (e.g. 1024). This is supposed to simulate an external user.
- Create an UEFI variable with a name of length 254 (ends up at length 291
   with the appended GUID, which is above the normal NAME_MAX limit).
- Create a "sentinel" UEFI variable with a non-critical name size (e.g. 32)
   to determine whether iteration has been stopped early during the next boot.
- Reboot into the same kernel but with an unmodified NAME_MAX limit (i.e. 255).
- Observe that not only the sentinel variable shows up (i.e. iteration
   hasn't stopped early), but that even the variable with a file name length of
   291 shows up and continues to be readable and writable from userspace.

Notably (and unexpectedly), only the _creation_ of efivarfs files with length
larger than NAME_MAX (from inside userspace) seems to abide by the NAME_MAX
limit, and ends up bailing out with "File name too long" / ENAMETOOLONG.
Therefore, please disregard my earlier statement about "processing such
entries properly is meaningless" that I put into the patch-accompanying message.
I assumed it would be enforced across all/most common file operations instead
of just when creating files.

On Tue, 30 Jan 2024 at 17:00, Tim Schumacher <timschumi@gmx.de> wrote:
>
> On 26.01.24 19:02, Tim Schumacher wrote:
> > On 26.01.24 17:35, Ard Biesheuvel wrote:
> >> On Fri, 26 Jan 2024 at 17:25, Tim Schumacher <timschumi@gmx.de> wrote:
> >>
> >>> One thing that I just recently noticed is that properly processing
> >>> variables above 512 bytes in size is currently meaningless anyways,
> >>> since the VFS layer only allows file name sizes of up to 255 bytes,
> >>> and 512 bytes of UCS2 will end up being at least 256 bytes of
> >>> UTF-8.
> >>>
> >>
> >> Interesting. Let's add this to the commit log - it makes the case much
> >> stronger, given that it proves that it is impossible for anyone to be
> >> relying on the current maximum being over 512 bytes.
> >
> > It makes the case much stronger for why one wouldn't be able to _create_
> > variables of that length from Linux userspace, creating dentries internally
> > seems to have different restrictions (or at least their name size seems
> > unlimited to me). Therefore, anything external could have still created
> > such variables, and such a variable will also affect any variable that
> > follows, not just itself. They don't have to be processed properly, but
> > they still need to be processed (and they currently aren't processed at all).
> >
>
> I was able to experimentally confirm that creating dentries internally is
> _not_ restricted by the value of NAME_MAX. The test setup was as follows:
>
> - Build and boot a kernel with NAME_MAX bumped to an artificially high
>    value (e.g. 1024). This is supposed to simulate an external user.
> - Create an UEFI variable with a name of length 254 (ends up at length 291
>    with the appended GUID, which is above the normal NAME_MAX limit).
> - Create a "sentinel" UEFI variable with a non-critical name size (e.g. 32)
>    to determine whether iteration has been stopped early during the next boot.
> - Reboot into the same kernel but with an unmodified NAME_MAX limit (i.e. 255).
> - Observe that not only the sentinel variable shows up (i.e. iteration
>    hasn't stopped early), but that even the variable with a file name length of
>    291 shows up and continues to be readable and writable from userspace.
>
> Notably (and unexpectedly), only the _creation_ of efivarfs files with length
> larger than NAME_MAX (from inside userspace) seems to abide by the NAME_MAX
> limit, and ends up bailing out with "File name too long" / ENAMETOOLONG.
> Therefore, please disregard my earlier statement about "processing such
> entries properly is meaningless" that I put into the patch-accompanying message.
> I assumed it would be enforced across all/most common file operations instead
> of just when creating files.
>

Thanks for digging into this. I still think the change is reasonable:
Linux does not permit creating EFI variables that have names longer
than 512 bytes, and I have never seen any such names from any of the
firmware implementations that I have dealt with.

I have queued this up now. Thanks.