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]

Date: Fri, 26 Jan 2024 19:02:42 +0100
From: Tim Schumacher <timschumi@....de>
To: Ard Biesheuvel <ardb@...nel.org>
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 26.01.24 17:35, Ard Biesheuvel wrote:
> On Fri, 26 Jan 2024 at 17:25, Tim Schumacher <timschumi@....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.