linux-hardening - Re: [PATCH 1/3] efi: pstore: 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, 15 Mar 2024 10:16:59 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Tim Schumacher <timschumi@....de>
Cc: linux-efi@...r.kernel.org, Kees Cook <keescook@...omium.org>, 
	Tony Luck <tony.luck@...el.com>, "Guilherme G. Piccoli" <gpiccoli@...lia.com>, 
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH 1/3] efi: pstore: Request at most 512 bytes for variable names

Hi Tim,

On Fri, 15 Mar 2024 at 01:27, Tim Schumacher <timschumi@....de> wrote:
>
> Work around 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.
>
> This was already done to efivarfs in f45812cc23fb ("efivarfs: Request at
> most 512 bytes for variable names"), but the second copy of the variable
> iteration implementation was overlooked.
>
> Signed-off-by: Tim Schumacher <timschumi@....de>

Thanks for the patch. I'll take it as a fix.

As an aside, you really want to avoid EFI pstore in general, and
specifically on such old systems with quirky UEFI implementations.

> ---
> I CC'd the pstore people and linux-hardening mailing list because
> get_maintainer.pl suggested to do so. Apologies in case this was the
> incorrect decision, this is a very non-pstore-specific patch after all.
>

If any of the linux-hardening/pstore people give you grief, just send
them to me :-)

(I am part of the linux-hardening group myself, and work closely with Kees)


> I have taken the liberty of adding a TODO for the future, the actual
> refactor can follow at some point down the line.
> ---
>  drivers/firmware/efi/efi-pstore.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index e7b9ec6f8a86..f0ceb5702d21 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -135,7 +135,15 @@ static ssize_t efi_pstore_read(struct pstore_record *record)
>         efi_status_t status;
>
>         for (;;) {
> -               varname_size = 1024;
> +               /*
> +                * A small set of old UEFI implementations reject sizes
> +                * above a certain threshold, the lowest seen in the wild
> +                * is 512.
> +                *
> +                * TODO: Commonize with the iteration implementation in
> +                *       fs/efivarfs to keep all the quirks in one place.
> +                */
> +               varname_size = 512;
>
>                 /*
>                  * If this is the first read() call in the pstore enumeration,
> --
> 2.44.0
>