On Fri, 26 Jan 2024 at 17:25, Tim Schumacher <timschumi@xxxxxx> 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@xxxxxxxxxxxxxxx # 6.1+
> Signed-off-by: Tim Schumacher <timschumi@xxxxxx>
> ---
> 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
>
