linux-kernel - Re: [PATCH v3] efivarfs: Request at most 512 bytes for variable names

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXGOzk4OnsxL8T7Finx8RzNu23SriY7QokAvKD=BkEvpjw@mail.gmail.com>
Date: Fri, 26 Jan 2024 17:35:20 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Tim Schumacher <timschumi@....de>
Cc: linux-efi@...r.kernel.org, jk@...abs.org, mjg59@...f.ucam.org, 
	pjones@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] efivarfs: Request at most 512 bytes for variable names

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