Re: [PATCH] Validate EFI memory descriptors

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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.

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

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

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

That's a matter of taste, as already indicated. My taste, for example, is
that you're introducing a new local variable for no good reason.

Jan