efi/memattr: Fix thinko in table size sanity check

While it is true that each PE/COFF runtime driver in memory can
generally be split into 3 different regions (the header, the code/rodata
region and the data/bss region), each with different permissions, it
does not mean that 3x the size of the memory map is a suitable upper
bound. This is due to the fact that all runtime drivers could be
coalesced into a single EFI runtime code region by the firmware, and if
the firmware does a good job of keeping the fragmentation down, it is
conceivable that the memory attributes table has more entries than the
EFI memory map itself.

So instead, base the sanity check on whether the descriptor size matches
the EFI memory map's descriptor size closely enough (which is not
mandated by the spec but extremely unlikely to differ in practice), and
whether the size of the whole table does not exceed 64k entries.

Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Ard Biesheuvel 1 month ago 5d0faa8e 56e2ef8f

+28 -9

1 changed file

expand all

drivers

firmware

efi

memattr.c

+28 -9

drivers/firmware/efi/memattr.c

··· 22 22 void __init efi_memattr_init(void) 23 23 { 24 24 efi_memory_attributes_table_t *tbl; 25 - unsigned long size; 26 25 27 26 if (efi_mem_attr_table == EFI_INVALID_TABLE_ADDR) 28 27 return; ··· 39 40 goto unmap; 40 41 } 41 42 43 + /* 44 + * The EFI memory attributes table descriptors might potentially be 45 + * smaller than those used by the EFI memory map, as long as they can 46 + * fit a efi_memory_desc_t. However, a larger descriptor size makes no 47 + * sense, and might be an indication that the table is corrupted. 48 + * 49 + * The only exception is kexec_load(), where the EFI memory map is 50 + * reconstructed by user space, and may use a smaller descriptor size 51 + * than the original. Given that, ignoring this companion table is 52 + * still the right thing to do here, but don't complain too loudly when 53 + * this happens. 54 + */ 55 + if (tbl->desc_size < sizeof(efi_memory_desc_t) || 56 + tbl->desc_size > efi.memmap.desc_size) { 57 + pr_warn("Unexpected EFI Memory Attributes descriptor size %u (expected: %lu)\n", 58 + tbl->desc_size, efi.memmap.desc_size); 59 + goto unmap; 60 + } 42 61 43 62 /* 44 - * Sanity check: the Memory Attributes Table contains up to 3 entries 45 - * for each entry of type EfiRuntimeServicesCode in the EFI memory map. 46 - * So if the size of the table exceeds 3x the size of the entire EFI 47 - * memory map, there is clearly something wrong, and the table should 48 - * just be ignored altogether. 63 + * Sanity check: the Memory Attributes Table contains multiple entries 64 + * for each EFI runtime services code or data region in the EFI memory 65 + * map, each with the permission attributes that may be applied when 66 + * mapping the region. There is no upper bound for the number of 67 + * entries, as it could conceivably contain more entries than the EFI 68 + * memory map itself. So pick an arbitrary limit of 64k, which is 69 + * ludicrously high. This prevents a corrupted table from eating all 70 + * system RAM. 49 71 */ 50 - size = tbl->num_entries * tbl->desc_size; 51 - if (size > 3 * efi.memmap.nr_map * efi.memmap.desc_size) { 72 + if (tbl->num_entries > SZ_64K) { 52 73 pr_warn(FW_BUG "Corrupted EFI Memory Attributes Table detected! (version == %u, desc_size == %u, num_entries == %u)\n", 53 74 tbl->version, tbl->desc_size, tbl->num_entries); 54 75 goto unmap; 55 76 } 56 77 57 - tbl_size = sizeof(*tbl) + size; 78 + tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size; 58 79 memblock_reserve(efi_mem_attr_table, tbl_size); 59 80 set_bit(EFI_MEM_ATTR, &efi.flags); 60 81

Configure Feed

Configure Feed