Re: [PATCH v2 4/6] efi: memmap: Disregard bogus entries instead of returning them

[Demi Marie Obenour <demi@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

On Mon, Oct 03, 2022 at 01:26:23PM +0200, Ard Biesheuvel wrote:
> The ESRT code currently contains some sanity checks on the memory
> descriptor it obtains, but these can only trigger when the descriptor is
> invalid (if at all).
> 
> So let's drop these checks, and instead, disregard descriptors entirely
> if the start address is misaligned, or the number of pages reaches
> beyond the end of the address space. Note that the memory map as a whole
> could still be inconsistent, i.e., multiple entries might cover the same
> area, or the address could be outside of the addressable VA space, but
> validating that goes beyond the scope of these helpers.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/efi.c  | 13 +++++++------
>  drivers/firmware/efi/esrt.c | 18 +-----------------
>  2 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 11857af72859..55bd3f4aab28 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  	efi_memory_desc_t *md;
>  
>  	if (!efi_enabled(EFI_MEMMAP)) {
> -		pr_err_once("EFI_MEMMAP is not enabled.\n");
> +		pr_warn_once("EFI_MEMMAP is not enabled.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (!out_md) {
> -		pr_err_once("out_md is null.\n");
> -		return -EINVAL;
> -        }
> -

Nit: this seems unrelated.

>  	for_each_efi_memory_desc(md) {
>  		u64 size;
>  		u64 end;
>  
> +		/* skip bogus entries */
> +		if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
> +		    (md->phys_addr > 0 &&
> +		     (md->num_pages > (U64_MAX - md->phys_addr + 1) >> EFI_PAGE_SHIFT)))
> +			continue;

Should this also check if md->num_pages is 0?  Also, should this check
be part of for_each_efi_memory_desc()?

> +
>  		size = md->num_pages << EFI_PAGE_SHIFT;
>  		end = md->phys_addr + size;
>  		if (phys_addr >= md->phys_addr && phys_addr < end) {
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 2a2f52b017e7..8f86f2b0734b 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -247,9 +247,6 @@ void __init efi_esrt_init(void)
>  	int rc;
>  	phys_addr_t end;
>  
> -	if (!efi_enabled(EFI_MEMMAP))
> -		return;
> -
>  	pr_debug("esrt-init: loading.\n");
>  	if (!esrt_table_exists())
>  		return;
> @@ -263,21 +260,8 @@ void __init efi_esrt_init(void)
>  		return;
>  	}
>  
> -	max = efi_mem_desc_end(&md);
> -	if (max < efi.esrt) {
> -		pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> -		       (void *)efi.esrt, (void *)max);
> -		return;
> -	}
> -
> +	max = efi_mem_desc_end(&md) - efi.esrt;
>  	size = sizeof(*esrt);
> -	max -= efi.esrt;
> -
> -	if (max < size) {
> -		pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
> -		       size, max);
> -		return;
> -	}

This can still happen if the ESRT pointer is very very close to the end
of a memory map entry, unless there is another check that handles
such cases.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]