Re: [PATCH 1/2] x86/efi: Map EFI memmap entries in-order at runtime

[Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 25 September 2015 at 22:56, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> So this commit worries me.
>
> This bug is a good find, and the fix is obviously needed and urgent, but I'm not
> sure about the implementation at all. (I've Cc:-ed a few more x86 low level
> gents.)
>
> * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>
>>  /*
>> + * Iterate the EFI memory map in reverse order because the regions
>> + * will be mapped top-down. The end result is the same as if we had
>> + * mapped things forward, but doesn't require us to change the
>> + * existing implementation of efi_map_region().
>> + */
>> +static inline void *efi_map_next_entry_reverse(void *entry)
>> +{
>> +     /* Initial call */
>> +     if (!entry)
>> +             return memmap.map_end - memmap.desc_size;
>> +
>> +     entry -= memmap.desc_size;
>> +     if (entry < memmap.map)
>> +             return NULL;
>> +
>> +     return entry;
>> +}
>> +
>> +/*
>> + * efi_map_next_entry - Return the next EFI memory map descriptor
>> + * @entry: Previous EFI memory map descriptor
>> + *
>> + * This is a helper function to iterate over the EFI memory map, which
>> + * we do in different orders depending on the current configuration.
>> + *
>> + * To begin traversing the memory map @entry must be %NULL.
>> + *
>> + * Returns %NULL when we reach the end of the memory map.
>> + */
>> +static void *efi_map_next_entry(void *entry)
>> +{
>> +     if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {
>> +             /*
>> +              * Starting in UEFI v2.5 the EFI_PROPERTIES_TABLE
>> +              * config table feature requires us to map all entries
>> +              * in the same order as they appear in the EFI memory
>> +              * map. That is to say, entry N must have a lower
>> +              * virtual address than entry N+1. This is because the
>> +              * firmware toolchain leaves relative references in
>> +              * the code/data sections, which are split and become
>> +              * separate EFI memory regions. Mapping things
>> +              * out-of-order leads to the firmware accessing
>> +              * unmapped addresses.
>> +              *
>> +              * Since we need to map things this way whether or not
>> +              * the kernel actually makes use of
>> +              * EFI_PROPERTIES_TABLE, let's just switch to this
>> +              * scheme by default for 64-bit.
>
> The thing is, if relative accesses between these 'sections' do happen then the
> requirement is much stronger than just 'ordered by addresses' - then we must map
> them continuously and as a single block!
>

The primary difference between pre-2.5 and 2.5 with this feature
enabled is that formerly, each PE/COFF image in memory would be
covered by at most a single EfiRuntimeServicesCode region, and now, a
single PE/COFF image may be split into different regions. It is only
relative references *inside* such a PE/COFF image that we are
concerned about, since no symbol references exist between separate
ones.

Also, it is not only relative references inside the PE/COFF image that
cause the problem. Another aspect is that the offset that is applied
to all absolute references at relocation time is derived from the
offset of the base of the PE/COFF image. If part of the PE/COFF image
(the .data section) is moved relatively to the code section, these
absolute references are fixed up incorrectly. This is actually a
problem that we could solve at the firmware side, but since PE/COFF
does not really tolerate being split up like that, the correct fix is
to keep all regions belonging to a single PE/COFF image adjacent.
Since we can't tell which those regions are, the next best approach is
to keep all adjacent regions with the EFI_MEMORY_RUNTIME attribute
adjacent in the VA mapping.

-- 
Ard.