| Message ID | 20241031175822.2952471-2-ardb+git@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] efi/memattr: Ignore table if the size is clearly bogus | expand |
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
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?
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,
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.
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>
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
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);