ACK/Cmnt: [PATCH 1/1] efi/x86/Add missing error handling to old_memmap 1:1 mapping code

[Stefan Bader]

On 08.06.20 17:35, Benjamin M Romer wrote:
> From: Gen Zhang <blackgod016574 at gmail.com>
> 
> 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 at gmail.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Linus Torvalds <torvalds at linux-foundation.org>
> Cc: Peter Zijlstra <peterz at infradead.org>
> Cc: Rob Bradford <robert.bradford at intel.com>
> Cc: Thomas Gleixner <tglx at linutronix.de>
> Cc: linux-efi at vger.kernel.org
> Link: http://lkml.kernel.org/r/20190525112559.7917-2-ard.biesheuvel@linaro.org
> Signed-off-by: Ingo Molnar <mingo at kernel.org>
> 
> CVE-2019-12380
> (cherry picked from commit 4e78921ba4dd0aca1cc89168f45039add4183f8e)
> [ ben_r: backport drops nearly entire patch. The 4.4 kernel skips
>   restore of save_pgd if EFI_OLD_MEMMAP isn't set and returns NULL. We
>   should not fail out in this case, so we check both conditions instead,
>   and return an error code that exists in this tree. ]
> Signed-off-by: Benjamin M Romer <benjamin.romer at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---

Same (or even worse) that for the bionic backport. Should be changed into a
backported from when applying.

-Stefan

>  arch/x86/platform/efi/efi.c    | 2 ++
>  arch/x86/platform/efi/efi_64.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 105872617be0..ee95e64320d8 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -89,6 +89,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 && efi_enabled(EFI_OLD_MEMMAP))
> +		return EFI_BAD_BUFFER_SIZE;
>  
>  	/* 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 8067840818e9..8f9f9ba5ad5b 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -92,6 +92,8 @@ pgd_t * __init efi_call_phys_prolog(void)
>  
>  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
>  	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> +	if (!save_pgd)
> +		return NULL;
>  
>  	for (pgd = 0; pgd < n_pgds; pgd++) {
>  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200609/ff411e71/attachment.sig>