Re: [PATCH] Validate EFI memory descriptors

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.12.2022 00:57, Demi Marie Obenour wrote:
> It turns out that these can be invalid in various ways.  Based on code
> Ard Biesheuvel contributed for Linux.
> 
> Co-developed-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>

This comes with the risk of breaking something that was previously working,
despite a descriptor being bogus. This _may_ be deemed acceptable, but it
needs calling out and reasoning about in the description. Even more so that
elsewhere we're aiming at relaxing things (by default or via command line
overrides) to remain compatible with all kinds of flawed implementations.

> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -51,3 +51,17 @@ void free_ebmalloc_unused_mem(void);
>  
>  const void *pe_find_section(const void *image_base, const size_t image_size,
>                              const CHAR16 *section_name, UINTN *size_out);
> +
> +static inline UINT64
> +efi_memory_descriptor_len(const EFI_MEMORY_DESCRIPTOR *desc)
> +{
> +    uint64_t remaining_space = UINT64_MAX - desc->PhysicalStart;

This wants to derive from PADDR_BITS (or even paddr_bits) rather than
using UINT64_MAX.

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -270,18 +270,17 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info 
> *info)
>          for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>          {
>              EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> -            u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> +            uint64_t size, len = efi_memory_descriptor_len(desc);
>  
>              if ( info->mem.addr >= desc->PhysicalStart &&
> -                 info->mem.addr < desc->PhysicalStart + len )
> +                 info->mem.addr - desc->PhysicalStart < len )

With what efi_memory_descriptor_len() does I don't see why this expression
would need transformation - there's no overflow possible anymore.

>              {
>                  info->mem.type = desc->Type;
>                  info->mem.attr = desc->Attribute;
> -                if ( info->mem.addr + info->mem.size < info->mem.addr ||

This overflow check is not superseded by the use of
efi_memory_descriptor_len(), so if you think it is not (or no longer)
needed, you will need to justify that in the description.

> -                     info->mem.addr + info->mem.size >
> -                     desc->PhysicalStart + len )
> -                    info->mem.size = desc->PhysicalStart + len -
> -                                     info->mem.addr;
> +                size = desc->PhysicalStart + len - info->mem.addr;
> +                if ( info->mem.size > size )
> +                    info->mem.size = size;
> +
>                  return 0;
>              }

Is there any functional change in here that I'm overlooking, or are you
merely converting this code to meet your personal taste? In the latter
case I'd prefer if you left the code untouched, with the 2nd best option
being to split it to a separate (style only) patch, and the 3rd option
being to at least mention this as a no-op (style only) transformation in
the description.

Jan