Re: [RFC 1/2] efi/memattr: Use desc_size instead of total size to check for corruption

[Usama Arif <usamaarif642@gmail.com>]

On 09/01/2025 15:45, Ard Biesheuvel wrote:
> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>
>> The commit in [1] introduced a check to see if EFI memory attributes
>> table was corrupted. It assumed that efi.memmap.nr_map remains
>> constant, but it changes during late boot.
>> Hence, the check is valid during cold boot, but not in the subsequent
>> kexec boot.
>>
>> This is best explained with an exampled. At cold boot, for a test
>> machine:
>> efi.memmap.nr_map=91,
>> memory_attributes_table->num_entries=48,
>> desc_size = 48
>> Hence, the check introduced in [1] where 3x the size of the
>> entire EFI memory map is a reasonable upper bound for the size of this
>> table is valid.
>>
>> In late boot __efi_enter_virtual_mode calls 2 functions that updates
>> efi.memmap.nr_map:
>> - efi_map_regions which reduces the `count` of map entries
>>   (for e.g. if should_map_region returns false) and which is reflected
>>   in efi.memmap by __efi_memmap_init.
>>   At this point efi.memmap.nr_map becomes 46 in the test machine.
>> - efi_free_boot_services which also reduces the number of memory regions
>>   available (for e.g. if md->type or md->attribute is not the right value).
>>   At this point efi.memmap.nr_map becomes 9 in the test machine.
>> Hence when you kexec into a new kernel and pass efi.memmap, the
>> paramaters that are compared are:
>> efi.memmap.nr_map=9,
>> memory_attributes_table->num_entries=48,
>> desc_size = 48
>> where the check in [1] is no longer valid with such a low efi.memmap.nr_map
>> as it was reduced due to efi_map_regions and efi_free_boot_services.
>>
>> A more appropriate check is to see if the description size reported by
>> efi and memory attributes table is the same.
>>
>> [1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@google.com/
>>
>> Fixes: 8fbe4c49c0cc ("efi/memattr: Ignore table if the size is clearly bogus")
>> Reported-by: Breno Leitao <leitao@debian.org>
>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>> ---
>>  drivers/firmware/efi/memattr.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>
> 
> The more I think about this, the more I feel that kexec on x86 should
> simply discard this table, and run with the firmware code RWX (which
> is not the end of the world).


By discard this table, do you mean kexec not use e820_table_firmware?
Also a very basic question, what do you mean by run with the firmware RWX?

Sorry for the very basic questions above!


> 
> The main reason is that the EFI memory map and the EFI memory
> attributes table are supposed to be a matched pair, where each RTcode
> entry in the former is broken down into multiple code and data
> segments in the latter. The amount of mangling that the x86 arch code
> does of the EFI memory map makes it intractable to ensure that they
> remain in sync, and so it is better not to bother.
> 
> 
>> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
>> index c38b1a335590..d3bc161361fb 100644
>> --- a/drivers/firmware/efi/memattr.c
>> +++ b/drivers/firmware/efi/memattr.c
>> @@ -40,21 +40,17 @@ int __init efi_memattr_init(void)
>>                 goto unmap;
>>         }
>>
>> -
>>         /*
>> -        * Sanity check: the Memory Attributes Table contains up to 3 entries
>> -        * for each entry of type EfiRuntimeServicesCode in the EFI memory map.
>> -        * So if the size of the table exceeds 3x the size of the entire EFI
>> -        * memory map, there is clearly something wrong, and the table should
>> -        * just be ignored altogether.
>> +        * Sanity check: the Memory Attributes Table desc_size and
>> +        * efi.memmap.desc_size should match.
>>          */
>> -       size = tbl->num_entries * tbl->desc_size;
>> -       if (size > 3 * efi.memmap.nr_map * efi.memmap.desc_size) {
>> -               pr_warn(FW_BUG "Corrupted EFI Memory Attributes Table detected! (version == %u, desc_size == %u, num_entries == %u)\n",
>> -                       tbl->version, tbl->desc_size, tbl->num_entries);
>> +       if (efi.memmap.desc_size != tbl->desc_size) {
>> +               pr_warn(FW_BUG "Corrupted EFI Memory Attributes Table detected! (version == %u, table desc_size == %u, efi.memmap.desc_size == %lu, table num_entries == %u)\n",
>> +                       tbl->version, tbl->desc_size, efi.memmap.desc_size, tbl->num_entries);
>>                 goto unmap;
>>         }
>>
>> +       size = tbl->num_entries * tbl->desc_size;
>>         tbl_size = sizeof(*tbl) + size;
>>         memblock_reserve(efi_mem_attr_table, tbl_size);
>>         set_bit(EFI_MEM_ATTR, &efi.flags);
>> --
>> 2.43.5
>>