[PATCH] efi/memattr: Ignore table if the size is clearly bogus

[PATCH] efi/memattr: Ignore table if the size is clearly bogus

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

There are reports [0] of cases where a corrupt EFI Memory Attributes
Table leads to out of memory issues at boot because the descriptor size
and entry count in the table header are still used to reserve the entire
table in memory, even though the resulting region is gigabytes in size.

Given that the EFI Memory Attributes Table is supposed to carry up to 3
entries for each EfiRuntimeServicesCode region in the EFI memory map,
and given that there is no reason for the descriptor size used in the
table to exceed the one used in the EFI memory map, 3x the size of the
entire EFI memory map is a reasonable upper bound for the size of this
table. This means that sizes exceeding that are highly likely to be
based on corrupted data, and the table should just be ignored instead.

[0] https://bugzilla.suse.com/show_bug.cgi?id=1231465

Cc: Gregory Price <gourry@gourry.net>
Cc: Usama Arif <usamaarif642@gmail.com>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Breno Leitao <leitao@debian.org>
Link: https://lore.kernel.org/all/20240912155159.1951792-2-ardb+git@google.com/
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/memattr.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index 164203429fa7..d85363d0422a 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -40,6 +40,20 @@ int __init efi_memattr_init(void)
 	}
 
 	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
+
+	/*
+	 * 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.
+	 */
+	if (tbl_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);
+		goto unmap;
+	}
+
 	memblock_reserve(efi_mem_attr_table, tbl_size);
 	set_bit(EFI_MEM_ATTR, &efi.flags);
 
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH] efi/memattr: Ignore table if the size is clearly bogus

[Breno Leitao]

Hello Ard,

On Thu, Oct 31, 2024 at 11:44:00AM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> There are reports [0] of cases where a corrupt EFI Memory Attributes
> Table leads to out of memory issues at boot because the descriptor size
> and entry count in the table header are still used to reserve the entire
> table in memory, even though the resulting region is gigabytes in size.
> 
> Given that the EFI Memory Attributes Table is supposed to carry up to 3
> entries for each EfiRuntimeServicesCode region in the EFI memory map,
> and given that there is no reason for the descriptor size used in the
> table to exceed the one used in the EFI memory map, 3x the size of the
> entire EFI memory map is a reasonable upper bound for the size of this
> table. This means that sizes exceeding that are highly likely to be
> based on corrupted data, and the table should just be ignored instead.

First of all, *thank you* for the quick fix.

A few questions/thoughts that are still bugging me. If you know the
answer for any, I would appreciate hearing from you.

1) This patch protects the kernel from a broken firmware. It has nothing
to do with the kernel code. The kernel is being a victim here.

2) The bug reported in [0], clearly found that the problem was likely
introduced by 2e6871a632a99d9b9e2ce3a7847acabe99e5a26e[1] from a cold
boot. How it might be related to
2e6871a632a99d9b9e2ce3a7847acabe99e5a26e ?

3) Does having 2e6871a632a99d9b9e2ce3a7847acabe99e5a26e make the
firmware bug (EFI Memory Attribute Trable corruption) more visible by
any chance?

[1] https://bugzilla.suse.com/show_bug.cgi?id=1231465#c44
Thank you!
--breno

Re: [PATCH] efi/memattr: Ignore table if the size is clearly bogus

[Ard Biesheuvel]

On Thu, 31 Oct 2024 at 15:17, Breno Leitao <leitao@debian.org> wrote:
>
> Hello Ard,
>
> On Thu, Oct 31, 2024 at 11:44:00AM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > There are reports [0] of cases where a corrupt EFI Memory Attributes
> > Table leads to out of memory issues at boot because the descriptor size
> > and entry count in the table header are still used to reserve the entire
> > table in memory, even though the resulting region is gigabytes in size.
> >
> > Given that the EFI Memory Attributes Table is supposed to carry up to 3
> > entries for each EfiRuntimeServicesCode region in the EFI memory map,
> > and given that there is no reason for the descriptor size used in the
> > table to exceed the one used in the EFI memory map, 3x the size of the
> > entire EFI memory map is a reasonable upper bound for the size of this
> > table. This means that sizes exceeding that are highly likely to be
> > based on corrupted data, and the table should just be ignored instead.
>
> First of all, *thank you* for the quick fix.
>

My pleasure

> A few questions/thoughts that are still bugging me. If you know the
> answer for any, I would appreciate hearing from you.
>
> 1) This patch protects the kernel from a broken firmware. It has nothing
> to do with the kernel code. The kernel is being a victim here.
>

Indeed. And this is arguably something I should have added at the time
when support for this table was first added.

> 2) The bug reported in [0], clearly found that the problem was likely
> introduced by 2e6871a632a99d9b9e2ce3a7847acabe99e5a26e[1] from a cold
> boot. How it might be related to
> 2e6871a632a99d9b9e2ce3a7847acabe99e5a26e ?
>

To me, it does not seem related to the TPM event log handling at all.
The only change made there is the memory type, and none of the other
logic is modified.

What seems more likely to me is that allocating pool memory of type
'ACPI reclaim' triggers a page allocation in this case, to add enough
pages to the pool to satisfy the allocation request. Page allocations
modify the EFI memory map, and therefore trigger an update of the EFI
memory attributes table.

What I suspect is that this firmware doesn't handle this very well
when it happens late in the boot.

It would be useful to boot the affected system with 'efi=debug' to get
a look at the entire EFI memory map.

> 3) Does having 2e6871a632a99d9b9e2ce3a7847acabe99e5a26e make the
> firmware bug (EFI Memory Attribute Trable corruption) more visible by
> any chance?
>

Exactly.