Re: [PATCH] Validate EFI memory descriptors

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Dec 07, 2022 at 11:04:05AM +0100, Jan Beulich wrote:
> 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.

I decided to match Ard’s kernel patch, except for ignoring (as opposed
to using) descriptors that cover the entire 64-bit address space (and
thus have a length in bytes that overflows uint64_t).

What do you propose Xen do instead?  A broken memory map is a rather
serious problem; it means that the actual physical address space is
unknown.  For Linux I am considering tainting the kernel (with
TAINT_FIRMWARE_WORKAROUND) if it detects an invalid memory descriptor.

> > --- 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.

paddr_bits is not available yet, but I can use PADDR_BITS.  That does
require an explicit overflow check, but that is fine.

> > --- 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.

You are correct, but the new version is more idiomatic, IMO.

> >              {
> >                  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.

The justification is that info->mem.size is no longer used except in
comparison and assignment, so the overflow check is now redundant.

> > -                     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.

There should be no functional change here, but IMO the new version is
much easier to read: first compute the actual size, and if the provided
size is larger, clamp it.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature