fs/efivarfs/vars.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
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.0On 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.
© 2016 - 2025 Red Hat, Inc.