linux-kernel - Re: [PATCH AUTOSEL 4.19 17/49] efi/x86/Add missing error handling to old

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+Gu9ZJ42=NJWDX4+DgkMWaSEakNw-yYiUtsUE48D-V6=7-w@mail.gmail.com>
Date:   Sun, 9 Jun 2019 20:14:29 +0200
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Sasha Levin <sashal@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        Gen Zhang <blackgod016574@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Rob Bradford <robert.bradford@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-efi <linux-efi@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH AUTOSEL 4.19 17/49] efi/x86/Add missing error handling to
 old_memmap 1:1 mapping code

On Sat, 8 Jun 2019 at 13:43, Sasha Levin <sashal@...nel.org> wrote:
>
> From: Gen Zhang <blackgod016574@...il.com>
>
> [ Upstream commit 4e78921ba4dd0aca1cc89168f45039add4183f8e ]
>
> The old_memmap flow in efi_call_phys_prolog() performs numerous memory
> allocations, and either does not check for failure at all, or it does
> but fails to propagate it back to the caller, which may end up calling
> into the firmware with an incomplete 1:1 mapping.
>
> So let's fix this by returning NULL from efi_call_phys_prolog() on
> memory allocation failures only, and by handling this condition in the
> caller. Also, clean up any half baked sets of page tables that we may
> have created before returning with a NULL return value.
>
> Note that any failure at this level will trigger a panic() two levels
> up, so none of this makes a huge difference, but it is a nice cleanup
> nonetheless.
>
> [ardb: update commit log, add efi_call_phys_epilog() call on error path]
>
> Signed-off-by: Gen Zhang <blackgod016574@...il.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Rob Bradford <robert.bradford@...el.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: linux-efi@...r.kernel.org
> Link: http://lkml.kernel.org/r/20190525112559.7917-2-ard.biesheuvel@linaro.org
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> Signed-off-by: Sasha Levin <sashal@...nel.org>

This was already discussed in the thread that proposed this patch for
stable: please don't queue this right now, the patches are more likely
to harm than hurt, and they certainly don't fix a security
vulnerability, as has been claimed.


> ---
>  arch/x86/platform/efi/efi.c    | 2 ++
>  arch/x86/platform/efi/efi_64.c | 9 ++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 9061babfbc83..353019d4e6c9 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -86,6 +86,8 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
>         pgd_t *save_pgd;
>
>         save_pgd = efi_call_phys_prolog();
> +       if (!save_pgd)
> +               return EFI_ABORTED;
>
>         /* Disable interrupts around EFI calls: */
>         local_irq_save(flags);
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index ee5d08f25ce4..dfc809b31c7c 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -84,13 +84,15 @@ pgd_t * __init efi_call_phys_prolog(void)
>
>         if (!efi_enabled(EFI_OLD_MEMMAP)) {
>                 efi_switch_mm(&efi_mm);
> -               return NULL;
> +               return efi_mm.pgd;
>         }
>
>         early_code_mapping_set_exec(1);
>
>         n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
>         save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL);
> +       if (!save_pgd)
> +               return NULL;
>
>         /*
>          * Build 1:1 identity mapping for efi=old_map usage. Note that
> @@ -138,10 +140,11 @@ pgd_t * __init efi_call_phys_prolog(void)
>                 pgd_offset_k(pgd * PGDIR_SIZE)->pgd &= ~_PAGE_NX;
>         }
>
> -out:
>         __flush_tlb_all();
> -
>         return save_pgd;
> +out:
> +       efi_call_phys_epilog(save_pgd);
> +       return NULL;
>  }
>
>  void __init efi_call_phys_epilog(pgd_t *save_pgd)
> --
> 2.20.1
>