[PATCH v2 1/5] efi/memattr: Fix thinko in table size sanity check

[Ard Biesheuvel <ardb+git@google.com>]

From: Ard Biesheuvel <ardb@kernel.org>

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.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/memattr.c | 37 +++++++++++++++-----
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index e727cc5909cb..b83f1c5a9164 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -22,7 +22,6 @@ unsigned long __ro_after_init efi_mem_attr_table = EFI_INVALID_TABLE_ADDR;
 void __init efi_memattr_init(void)
 {
 	efi_memory_attributes_table_t *tbl;
-	unsigned long size;
 
 	if (efi_mem_attr_table == EFI_INVALID_TABLE_ADDR)
 		return;
@@ -40,22 +39,42 @@ void __init efi_memattr_init(void)
 		goto unmap;
 	}
 
+	/*
+	 * The EFI memory attributes table descriptors might potentially be
+	 * smaller than those used by the EFI memory map, as long as they can
+	 * fit a efi_memory_desc_t. However, a larger descriptor size makes no
+	 * sense, and might be an indication that the table is corrupted.
+	 *
+	 * The only exception is kexec_load(), where the EFI memory map is
+	 * reconstructed by user space, and may use a smaller descriptor size
+	 * than the original. Given that, ignoring this companion table is
+	 * still the right thing to do here, but don't complain too loudly when
+	 * this happens.
+	 */
+	if (tbl->desc_size < sizeof(efi_memory_desc_t) ||
+	    tbl->desc_size > efi.memmap.desc_size) {
+		pr_warn("Unexpected EFI Memory Attributes descriptor size %d (expected: %ld)\n",
+			tbl->desc_size, efi.memmap.desc_size);
+		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 contains multiple entries
+	 * for each EFI runtime services code or data region in the EFI memory
+	 * map, each with the permission attributes that may be applied when
+	 * mapping the region.  There is no upper bound for the number of
+	 * entries, as it could conceivably contain more entries than the EFI
+	 * memory map itself. So pick an arbitrary limit of 64k, which is
+	 * ludicrously high. This prevents a corrupted table from eating all
+	 * system RAM.
 	 */
-	size = tbl->num_entries * tbl->desc_size;
-	if (size > 3 * efi.memmap.nr_map * efi.memmap.desc_size) {
+	if (tbl->num_entries > SZ_64K) {
 		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);
 		goto unmap;
 	}
 
-	tbl_size = sizeof(*tbl) + size;
+	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
 	memblock_reserve(efi_mem_attr_table, tbl_size);
 	set_bit(EFI_MEM_ATTR, &efi.flags);
 
-- 
2.53.0.1118.gaef5881109-goog