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: [email protected] # 6.1+
Signed-off-by: Tim Schumacher <[email protected]>
---
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 <[email protected]> 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: [email protected] # 6.1+
> Signed-off-by: Tim Schumacher <[email protected]>
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 <[email protected]> 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 <[email protected]> wrote:
>
> On 23.01.24 12:24, Ard Biesheuvel wrote:
> > On Tue, 23 Jan 2024 at 00:15, Tim Schumacher <[email protected]> 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 <[email protected]> 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).
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: [email protected] # 6.1+
Signed-off-by: Tim Schumacher <[email protected]>
---
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
On Tue, Jan 23, 2024 at 12:33 PM Tim Schumacher <[email protected]> wrote:
>
> On 23.01.24 15:09, Ard Biesheuvel wrote:
> > On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <[email protected]> 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 Wed, 24 Jan 2024 at 22:25, Peter Jones <[email protected]> wrote:
>
> On Tue, Jan 23, 2024 at 12:33 PM Tim Schumacher <[email protected]> wrote:
> >
> > On 23.01.24 15:09, Ard Biesheuvel wrote:
> > > On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <[email protected]> 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.
On 24.01.24 22:25, Peter Jones wrote:
> On Tue, Jan 23, 2024 at 12:33 PM Tim Schumacher <[email protected]> wrote:
>>
>> On 23.01.24 15:09, Ard Biesheuvel wrote:
>>> On Tue, 23 Jan 2024 at 14:55, Tim Schumacher <[email protected]> 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.