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

[PATCH v2] 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>
---
v2: use unsigned temp variable to avoid signedness issues and to avoid
    assigning tbl_size in case of failure

 drivers/firmware/efi/memattr.c | 20 ++++++++++++++++++--
 1 file changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index 164203429fa7..cbc41935fe6c 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -22,6 +22,7 @@ unsigned long __ro_after_init efi_mem_attr_table = EFI_INVALID_TABLE_ADDR;
 int __init efi_memattr_init(void)
 {
 	efi_memory_attributes_table_t *tbl;
+	unsigned long size;
 
 	if (efi_mem_attr_table == EFI_INVALID_TABLE_ADDR)
 		return 0;
@@ -39,7 +40,22 @@ int __init efi_memattr_init(void)
 		goto unmap;
 	}
 
-	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.
+	 */
+	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);
+		goto unmap;
+	}
+
+	tbl_size = sizeof(*tbl) + size;
 	memblock_reserve(efi_mem_attr_table, tbl_size);
 	set_bit(EFI_MEM_ATTR, &efi.flags);
 
-- 
2.47.0.163.g1226f6d8fa-goog

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

[Breno Leitao]

Hello Ard,

On Thu, Oct 31, 2024 at 06:58:23PM +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.

I haven't seen this patch landing in net-next tree yet.
Do you have plan to have this merged into 6.13?

Thanks
--breno

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

[Ard Biesheuvel]

On Fri, 15 Nov 2024 at 11:10, Breno Leitao <leitao@debian.org> wrote:
>
> Hello Ard,
>
> On Thu, Oct 31, 2024 at 06:58:23PM +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.
>
> I haven't seen this patch landing in net-next tree yet.
> Do you have plan to have this merged into 6.13?
>

Nobody replied to it, so I wasn't going to.

Would you like this patch to be taken for v6.13? Does it fix the
issues you have been observing?

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

[Jiri Slaby]

On 15. 11. 24, 11:21, Ard Biesheuvel wrote:
> On Fri, 15 Nov 2024 at 11:10, Breno Leitao <leitao@debian.org> wrote:
>>
>> Hello Ard,
>>
>> On Thu, Oct 31, 2024 at 06:58:23PM +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.
>>
>> I haven't seen this patch landing in net-next tree yet.
>> Do you have plan to have this merged into 6.13?
>>
> 
> Nobody replied to it, so I wasn't going to.
> 
> Would you like this patch to be taken for v6.13? Does it fix the
> issues you have been observing?

For the reporter at:
   https://bugzilla.suse.com/show_bug.cgi?id=1231465#c50
definitely!

I was expected this to land in the tree too... (Without any further 
notifications to you.)

thanks,
-- 
js
suse labs

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

[Ard Biesheuvel]

On Fri, 15 Nov 2024 at 11:51, Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 15. 11. 24, 11:21, Ard Biesheuvel wrote:
> > On Fri, 15 Nov 2024 at 11:10, Breno Leitao <leitao@debian.org> wrote:
> >>
> >> Hello Ard,
> >>
> >> On Thu, Oct 31, 2024 at 06:58:23PM +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.
> >>
> >> I haven't seen this patch landing in net-next tree yet.
> >> Do you have plan to have this merged into 6.13?
> >>
> >
> > Nobody replied to it, so I wasn't going to.
> >
> > Would you like this patch to be taken for v6.13? Does it fix the
> > issues you have been observing?
>
> For the reporter at:
>    https://bugzilla.suse.com/show_bug.cgi?id=1231465#c50
> definitely!
>
> I was expected this to land in the tree too... (Without any further
> notifications to you.)
>

Excellent. I'll take this as an ack from both of you.

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

[Breno Leitao]

On Fri, Nov 15, 2024 at 12:01:38PM +0100, Ard Biesheuvel wrote:
> On Fri, 15 Nov 2024 at 11:51, Jiri Slaby <jirislaby@kernel.org> wrote:
> >
> > On 15. 11. 24, 11:21, Ard Biesheuvel wrote:
> > > On Fri, 15 Nov 2024 at 11:10, Breno Leitao <leitao@debian.org> wrote:
> > >>
> > >> Hello Ard,
> > >>
> > >> On Thu, Oct 31, 2024 at 06:58:23PM +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.
> > >>
> > >> I haven't seen this patch landing in net-next tree yet.
> > >> Do you have plan to have this merged into 6.13?
> > >>
> > >
> > > Nobody replied to it, so I wasn't going to.
> > >
> > > Would you like this patch to be taken for v6.13? Does it fix the
> > > issues you have been observing?
> >
> > For the reporter at:
> >    https://bugzilla.suse.com/show_bug.cgi?id=1231465#c50
> > definitely!
> >
> > I was expected this to land in the tree too... (Without any further
> > notifications to you.)
> >
> 
> Excellent. I'll take this as an ack from both of you.

Thanks! I've just send a "formal" ack to keep it registered.

Thanks again for solving it.
--breno

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

[Breno Leitao]

On Thu, Oct 31, 2024 at 06:58:23PM +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.
> 
> [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>

Acked-by: Breno Leitao <leitao@debian.org>