Re: [PATCH] Validate EFI memory descriptors

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

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

On Thu, Dec 08, 2022 at 09:02:57AM +0100, Jan Beulich wrote:
> On 07.12.2022 23:23, Demi Marie Obenour wrote:
> > 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@kernel.org>
> >>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>
> >> 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.
> 
> Much here depends on what brokenness we find in practice. I've been
> flamed more than once for insisting on strict spec compliance. I'd
> be fine with you making things more strict here, but then - as said -
> the risks need to be called out in the description, and once you've
> done so you'll likely realize yourself that hence there then needs
> to be a way out for systems where the new checking turns out too
> strict.

The purpose of these checks is actually to work around broken firmware.
If firmware were bug-free, there would be no point in validating the
data it provides.  That said, I am not sure if ignoring the broken
entries is the correct answer.  Could Xen fall back to getting the
information from ACPI?  Or could Xen somehow try to make sense of the
broken table?  For instance, Xen could sort the entries by start address
and force the end of each entry to at or before the start of the next.
This patch has the advantage that it closely matches what Linux will be
doing.  I trust Ard’s judgement on this better than my own.

> Tainting the hypervisor in the event of finding an issue is certainly
> an option.

I probably will not add such a mechanism, but if one exists I would be
happy to use it.

> >>> --- 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.
> 
> paddr_bits may or may not be available yet; it certainly is ...
> 
> >>> --- 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);
> 
> ... for this use.
> 
> >>>              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.
> 
> I don't see what "no longer" refers to - nothing changes in this regard
> before and after your patch, afaics.

The purpose of this check is to catch the case where info->mem.size +
info->mem.addr overflows.  In the new code, there is no need to
explicitly check for this situation, because info->mem.size +
info->mem.addr is never actually computed.  Instead, the size is
computed first, and if info->mem.size is larger than this, it is clamped
to the actual value.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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