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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250108215957.3437660-2-usamaarif642@gmail.com>
Date: Wed,  8 Jan 2025 21:53:36 +0000
From: Usama Arif <usamaarif642@...il.com>
To: linux-efi@...r.kernel.org,
	devel@...2.groups.io,
	kexec@...ts.infradead.org
Cc: ardb@...nel.org,
	hannes@...xchg.org,
	dyoung@...hat.com,
	x86@...nel.org,
	linux-kernel@...r.kernel.org,
	leitao@...ian.org,
	gourry@...rry.net,
	kernel-team@...a.com,
	Usama Arif <usamaarif642@...il.com>
Subject: [RFC 1/2] efi/memattr: Use desc_size instead of total size to check for corruption

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@...ian.org>
Signed-off-by: Usama Arif <usamaarif642@...il.com>
---
 drivers/firmware/efi/memattr.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

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