| Message ID | 20240122231507.1307793-1-timschumi@gmx.de |
|---|---|
| State | New |
| Headers | show |
| Series | efivarfs: Iterate variables with increasing name buffer sizes | expand |
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 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.
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.
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);
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(-) -- 2.43.0