linux-kernel - Re: [tip:efi/core] efi/x86: Ignore unrealistically large option ROMs

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: <CAKv+Gu94+dK1WM__thFup9AmPrEqJ-7Ojynnx4hO9picanwhcQ@mail.gmail.com>
Date:   Tue, 15 May 2018 11:18:29 +0200
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Matt Fleming <matt@...eblueprint.co.uk>
Cc:     linux-tip-commits@...r.kernel.org
Subject: Re: [tip:efi/core] efi/x86: Ignore unrealistically large option ROMs

On 14 May 2018 at 09:50, tip-bot for Hans de Goede <tipbot@...or.com> wrote:
> Commit-ID:  1de3a1be8a9345fd0c7d9bb1009b21afe6b6b10f
> Gitweb:     https://git.kernel.org/tip/1de3a1be8a9345fd0c7d9bb1009b21afe6b6b10f
> Author:     Hans de Goede <hdegoede@...hat.com>
> AuthorDate: Fri, 4 May 2018 08:00:01 +0200
> Committer:  Ingo Molnar <mingo@...nel.org>
> CommitDate: Mon, 14 May 2018 08:57:49 +0200
>
> efi/x86: Ignore unrealistically large option ROMs
>
> setup_efi_pci() tries to save a copy of each PCI option ROM as this may
> be necessary for the device driver for the PCI device to have access too.
>
> On some systems the efi_pci_io_protocol's romimage and romsize fields
> contain invalid data, which looks a bit like pointers pointing back into
> other EFI code or data. Interpreting these pointers as romsize leads to
> a very large value and if we then try to alloc this amount of memory to
> save a copy the alloc call fails.
>
> This leads to a "Failed to alloc mem for rom" error being printed on the
> EFI console for each PCI device.
>
> This commit avoids the printing of these errors, by checking romsize before
> doing the alloc and if it is larger then the EFI spec limit of 16 MiB
> silently ignore the ROM fields instead of trying to alloc mem and fail.
>
> Tested-by: Hans de Goede <hdegoede@...hat.com>
> [ardb: deduplicate 32/64 bit changes, use SZ_16M symbolic constant]
> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>

This looks odd now: I sent this out as

Signed-off-by: Hans de Goede <hdegoede@...hat.com>
[ardb: deduplicate 32/64 bit changes, use SZ_16M symbolic constant]
Tested-by: Hans de Goede <hdegoede@...hat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>

which clearly conveys that Hans tested the updated version of the patch.

In general, I don't think there is a need to reorder signoffs unless
there is anything wrong with them, no?

> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Matt Fleming <matt@...eblueprint.co.uk>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: linux-efi@...r.kernel.org
> Link: http://lkml.kernel.org/r/20180504060003.19618-16-ard.biesheuvel@linaro.org
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> ---
>  arch/x86/boot/compressed/eboot.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index dadf32312082..a8a8642d2b0b 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -123,10 +123,17 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
>         if (status != EFI_SUCCESS)
>                 return status;
>
> +       /*
> +        * Some firmware images contain EFI function pointers at the place where the
> +        * romimage and romsize fields are supposed to be. Typically the EFI
> +        * code is mapped at high addresses, translating to an unrealistically
> +        * large romsize. The UEFI spec limits the size of option ROMs to 16
> +        * MiB so we reject any ROMs over 16 MiB in size to catch this.
> +        */
>         romimage = (void *)(unsigned long)efi_table_attr(efi_pci_io_protocol,
>                                                          romimage, pci);
>         romsize = efi_table_attr(efi_pci_io_protocol, romsize, pci);
> -       if (!romimage || !romsize)
> +       if (!romimage || !romsize || romsize > SZ_16M)
>                 return EFI_INVALID_PARAMETER;
>
>         size = romsize + sizeof(*rom);