[edk2-devel] [RFC 0/2] efi/memattr: Fix memory corruption and warning issues

[edk2-devel] [RFC 0/2] efi/memattr: Fix memory corruption and warning issues

[Usama Arif via groups.io]

Since the patch with the warning in [1] was merged, a very significant
number of kexec boots are producing the warning in our (Meta) fleet.

I believe there are 2 problems, the warning itself might not be
triggered on the right condition, and memory attributes
table is getting corrupted.
An example of the warning when its not triggered correctly and
is fixed by patch 1:
efi: memattr: [Firmware Bug]: Corrupted EFI Memory Attributes Table detected! (version == 2, desc_size == 48, num_entries == 48)
An example of the warning when memory attributes table
is getting corrupted and might possibly be fixed by patch 2:
efi: memattr: [Firmware Bug]: Corrupted EFI Memory Attributes Table detected! (version == 1, desc_size == 2072184435, num_entries == 3248688968)
Its clear that the desc size and num_entries are wrong.

The logic behind patch 1 is explained in its commit message.

The memory corruption is looking very similar to the problem that was
fixed by 77d48d39e99170 ("efistub/tpm: Use ACPI reclaim memory for
event log to avoid corruption"), but this time with memattr table,
where it might not be preserved during kexec. I have
not been able to reproduce this in the test machine I have
over the past couple of days (hence marked as RFC) , but its
happening often in our prod.
When this area is not reserved, it comes up as usable in
/sys/firmware/memmap. This means that kexec, which uses that memmap
to find usable memory regions, can select the region where
efi_mem_attr_table is and overwrite it and relocate_kernel.

Having a fix in firmware can be difficult to get through.
The next ideal place would be in libstub. However, it looks like
InstallMemoryAttributesTable [2] is not available as a boot service
call option [3], [4], I tried to use install_configuration_table as
a substitute, but its not valid and corrupts the MemoryAttributesTable.
The prints I got from the below code in coverletter were:
EFI stub: ERROR: KKK tbl 5f19e018 tbl_>version=1, tbl->num_entries 48 tbl->desc_size 48
EFI stub: ERROR: KKK2 tbl 67184018 tbl_>version=2048, tbl->num_entries 0 tbl->desc_size 0
which shows the table got corrupted. This can bee seen in the kernel boot as well after
(with the version showing up as 2048).

As a last option for a fix, the patch marks that region as reserved in
e820_table_firmware if it is currently E820_TYPE_RAM so that kexec doesn't
use it for kernel segments.

[1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@google.com/
[2] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c#L100
[3] https://github.com/tianocore/edk2/blob/42a141800c0c26a09d2344e84a89ce4097a263ae/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c#L41
[4] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/firmware/efi/libstub/efistub.h#L327


diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index d33ccbc4a2c6..a1a956f2d963 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -1143,6 +1143,7 @@ efi_enable_reset_attack_mitigation(void) { }
 #endif
 
 void efi_retrieve_eventlog(void);
+void efi_mem_attr_init(void);
 
 struct screen_info *alloc_screen_info(void);
 struct screen_info *__alloc_screen_info(void);
diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
index 4f1fa302234d..c5b60aea342e 100644
--- a/drivers/firmware/efi/libstub/mem.c
+++ b/drivers/firmware/efi/libstub/mem.c
@@ -128,3 +128,35 @@ void efi_free(unsigned long size, unsigned long addr)
        nr_pages = round_up(size, EFI_ALLOC_ALIGN) / EFI_PAGE_SIZE;
        efi_bs_call(free_pages, addr, nr_pages);
 }
+
+void efi_mem_attr_init(void)
+{
+       efi_guid_t linux_mem_attr_guid = EFI_MEMORY_ATTRIBUTES_TABLE_GUID;
+       efi_memory_attributes_table_t *tbl = NULL;
+       efi_status_t status;
+       unsigned long size;
+
+       tbl = get_efi_config_table(linux_mem_attr_guid);
+       efi_err("KKK tbl %lx tbl_>version=%d, tbl->num_entries %d tbl->desc_size %d\n", tbl, tbl->version, tbl->num_entries, tbl->desc_size);
+
+       size = tbl->num_entries * tbl->desc_size;
+       status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
+                            sizeof(*tbl) + size, (void **)&tbl);
+
+       if (status != EFI_SUCCESS) {
+               efi_err("Unable to allocate memory for event log\n");
+               return;
+       }
+
+       status = efi_bs_call(install_configuration_table,
+                            &linux_mem_attr_guid, tbl);
+
+       if (status != EFI_SUCCESS)
+               efi_err("Unable to install configuration table to update memory type\n");
+       efi_bs_call(free_pool, tbl);
+
+       /* verify if its the same table */
+       tbl = get_efi_config_table(linux_mem_attr_guid);
+       efi_err("KKK2 tbl %lx tbl_>version=%d, tbl->num_entries %d tbl->desc_size %d\n", tbl, tbl->version, tbl->num_entries, tbl->desc_size);
+
+}
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f8e465da344d..c0c3d278451d 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -1036,6 +1036,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 
        efi_retrieve_eventlog();
 
+       efi_mem_attr_init();
+
        setup_graphics(boot_params);
 
        setup_efi_pci(boot_params);
 
Usama Arif (2):
  efi/memattr: Use desc_size instead of total size to check for
    corruption
  efi/memattr: add efi_mem_attr_table as a reserved region in
    820_table_firmware

 arch/x86/include/asm/e820/api.h |  2 ++
 arch/x86/kernel/e820.c          |  6 ++++++
 arch/x86/platform/efi/efi.c     |  9 +++++++++
 drivers/firmware/efi/memattr.c  | 17 +++++++----------
 include/linux/efi.h             |  7 +++++++
 5 files changed, 31 insertions(+), 10 deletions(-)

-- 
2.43.5



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120999): https://edk2.groups.io/g/devel/message/120999
Mute This Topic: https://groups.io/mt/110633384/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Usama Arif via groups.io]

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@debian.org>
Signed-off-by: Usama Arif <usamaarif642@gmail.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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121000): https://edk2.groups.io/g/devel/message/121000
Mute This Topic: https://groups.io/mt/110633385/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Usama Arif via groups.io]

When this area is not reserved, it comes up as usable in
/sys/firmware/memmap. This means that kexec, which uses that memmap
to find usable memory regions, can select the region where
efi_mem_attr_table is and overwrite it and relocate_kernel.

Since the patch in [1] was merged, all boots after kexec
are producing the warning that it introduced.

Having a fix in firmware can be difficult to get through.
The next ideal place would be in libstub. However, it looks like
InstallMemoryAttributesTable [2] is not available as a boot service
call option [3], [4], and install_configuration_table does not
seem to work as a valid substitute.

As a last option for a fix, this patch marks that region as reserved in
e820_table_firmware if it is currently E820_TYPE_RAM so that kexec doesn't
use it for kernel segments.

[1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@google.com/
[2] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c#L100
[3] https://github.com/tianocore/edk2/blob/42a141800c0c26a09d2344e84a89ce4097a263ae/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c#L41
[4] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/firmware/efi/libstub/efistub.h#L327

Reported-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 arch/x86/include/asm/e820/api.h | 2 ++
 arch/x86/kernel/e820.c          | 6 ++++++
 arch/x86/platform/efi/efi.c     | 9 +++++++++
 drivers/firmware/efi/memattr.c  | 1 +
 include/linux/efi.h             | 7 +++++++
 5 files changed, 25 insertions(+)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index 2e74a7f0e935..4e9aa24f03bd 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -16,6 +16,8 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
 
 extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
 extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
+extern u64  e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
+					enum e820_type new_type);
 extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
 extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
 
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 82b96ed9890a..01d7d3c0d299 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -538,6 +538,12 @@ u64 __init e820__range_update_table(struct e820_table *t, u64 start, u64 size,
 	return __e820__range_update(t, start, size, old_type, new_type);
 }
 
+u64 __init e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
+				       enum e820_type new_type)
+{
+	return __e820__range_update(e820_table_firmware, start, size, old_type, new_type);
+}
+
 /* Remove a range of memory from the E820 table: */
 u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type)
 {
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index a7ff189421c3..13684c5d7c05 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -168,6 +168,15 @@ static void __init do_add_efi_memmap(void)
 	e820__update_table(e820_table);
 }
 
+/* Reserve firmware area if it was marked as RAM */
+void arch_update_firmware_area(u64 addr, u64 size)
+{
+	if (e820__get_entry_type(addr, addr + size) == E820_TYPE_RAM) {
+		e820__range_update_firmware(addr, size, E820_TYPE_RAM, E820_TYPE_RESERVED);
+		e820__update_table(e820_table_firmware);
+	}
+}
+
 /*
  * Given add_efi_memmap defaults to 0 and there is no alternative
  * e820 mechanism for soft-reserved memory, import the full EFI memory
diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index d3bc161361fb..d131781e2d7b 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -53,6 +53,7 @@ int __init efi_memattr_init(void)
 	size = tbl->num_entries * tbl->desc_size;
 	tbl_size = sizeof(*tbl) + size;
 	memblock_reserve(efi_mem_attr_table, tbl_size);
+	arch_update_firmware_area(efi_mem_attr_table, tbl_size);
 	set_bit(EFI_MEM_ATTR, &efi.flags);
 
 unmap:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e5815867aba9..8eb9698bd6a4 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1358,4 +1358,11 @@ extern struct blocking_notifier_head efivar_ops_nh;
 void efivars_generic_ops_register(void);
 void efivars_generic_ops_unregister(void);
 
+#ifdef CONFIG_X86_64
+void __init arch_update_firmware_area(u64 addr, u64 size);
+#else
+static inline void __init arch_update_firmware_area(u64 addr, u64 size)
+{
+}
+#endif
 #endif /* _LINUX_EFI_H */
-- 
2.43.5



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121001): https://edk2.groups.io/g/devel/message/121001
Mute This Topic: https://groups.io/mt/110633386/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel via groups.io]

On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>
> 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@debian.org>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  drivers/firmware/efi/memattr.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>

The more I think about this, the more I feel that kexec on x86 should
simply discard this table, and run with the firmware code RWX (which
is not the end of the world).

The main reason is that the EFI memory map and the EFI memory
attributes table are supposed to be a matched pair, where each RTcode
entry in the former is broken down into multiple code and data
segments in the latter. The amount of mangling that the x86 arch code
does of the EFI memory map makes it intractable to ensure that they
remain in sync, and so it is better not to bother.


> 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
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120975): https://edk2.groups.io/g/devel/message/120975
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Ard Biesheuvel via groups.io]

On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>
> When this area is not reserved, it comes up as usable in
> /sys/firmware/memmap. This means that kexec, which uses that memmap
> to find usable memory regions, can select the region where
> efi_mem_attr_table is and overwrite it and relocate_kernel.
>
> Since the patch in [1] was merged, all boots after kexec
> are producing the warning that it introduced.
>
> Having a fix in firmware can be difficult to get through.

I don't follow. I don't think there is anything wrong with the
firmware here. Could you elaborate?


> The next ideal place would be in libstub. However, it looks like
> InstallMemoryAttributesTable [2] is not available as a boot service
> call option [3], [4], and install_configuration_table does not
> seem to work as a valid substitute.
>

To do what, exactly?

> As a last option for a fix, this patch marks that region as reserved in
> e820_table_firmware if it is currently E820_TYPE_RAM so that kexec doesn't
> use it for kernel segments.
>
> [1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@google.com/
> [2] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c#L100
> [3] https://github.com/tianocore/edk2/blob/42a141800c0c26a09d2344e84a89ce4097a263ae/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c#L41
> [4] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/firmware/efi/libstub/efistub.h#L327
>
> Reported-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  arch/x86/include/asm/e820/api.h | 2 ++
>  arch/x86/kernel/e820.c          | 6 ++++++
>  arch/x86/platform/efi/efi.c     | 9 +++++++++
>  drivers/firmware/efi/memattr.c  | 1 +
>  include/linux/efi.h             | 7 +++++++
>  5 files changed, 25 insertions(+)
>
> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> index 2e74a7f0e935..4e9aa24f03bd 100644
> --- a/arch/x86/include/asm/e820/api.h
> +++ b/arch/x86/include/asm/e820/api.h
> @@ -16,6 +16,8 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
>
>  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
> +extern u64  e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
> +                                       enum e820_type new_type);
>  extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
>  extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 82b96ed9890a..01d7d3c0d299 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -538,6 +538,12 @@ u64 __init e820__range_update_table(struct e820_table *t, u64 start, u64 size,
>         return __e820__range_update(t, start, size, old_type, new_type);
>  }
>
> +u64 __init e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
> +                                      enum e820_type new_type)
> +{
> +       return __e820__range_update(e820_table_firmware, start, size, old_type, new_type);
> +}
> +
>  /* Remove a range of memory from the E820 table: */
>  u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type)
>  {
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index a7ff189421c3..13684c5d7c05 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -168,6 +168,15 @@ static void __init do_add_efi_memmap(void)
>         e820__update_table(e820_table);
>  }
>
> +/* Reserve firmware area if it was marked as RAM */
> +void arch_update_firmware_area(u64 addr, u64 size)
> +{
> +       if (e820__get_entry_type(addr, addr + size) == E820_TYPE_RAM) {
> +               e820__range_update_firmware(addr, size, E820_TYPE_RAM, E820_TYPE_RESERVED);
> +               e820__update_table(e820_table_firmware);
> +       }
> +}
> +
>  /*
>   * Given add_efi_memmap defaults to 0 and there is no alternative
>   * e820 mechanism for soft-reserved memory, import the full EFI memory
> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> index d3bc161361fb..d131781e2d7b 100644
> --- a/drivers/firmware/efi/memattr.c
> +++ b/drivers/firmware/efi/memattr.c
> @@ -53,6 +53,7 @@ int __init efi_memattr_init(void)
>         size = tbl->num_entries * tbl->desc_size;
>         tbl_size = sizeof(*tbl) + size;
>         memblock_reserve(efi_mem_attr_table, tbl_size);
> +       arch_update_firmware_area(efi_mem_attr_table, tbl_size);
>         set_bit(EFI_MEM_ATTR, &efi.flags);
>
>  unmap:
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index e5815867aba9..8eb9698bd6a4 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1358,4 +1358,11 @@ extern struct blocking_notifier_head efivar_ops_nh;
>  void efivars_generic_ops_register(void);
>  void efivars_generic_ops_unregister(void);
>
> +#ifdef CONFIG_X86_64
> +void __init arch_update_firmware_area(u64 addr, u64 size);
> +#else
> +static inline void __init arch_update_firmware_area(u64 addr, u64 size)
> +{
> +}
> +#endif
>  #endif /* _LINUX_EFI_H */
> --
> 2.43.5
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120976): https://edk2.groups.io/g/devel/message/120976
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Usama Arif via groups.io]

On 09/01/2025 16:15, Ard Biesheuvel wrote:
> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>
>> When this area is not reserved, it comes up as usable in
>> /sys/firmware/memmap. This means that kexec, which uses that memmap
>> to find usable memory regions, can select the region where
>> efi_mem_attr_table is and overwrite it and relocate_kernel.
>>
>> Since the patch in [1] was merged, all boots after kexec
>> are producing the warning that it introduced.
>>
>> Having a fix in firmware can be difficult to get through.
> 
> I don't follow. I don't think there is anything wrong with the
> firmware here. Could you elaborate?
> 

So the problem is, kexec sees this memory as System RAM, and decides
it can be used to place an image here.

I guess the question is (and I actually don't know the answer here),
whose responsibility is it to mark this region as reserved so that 
its not touched by anyone else. I would have thought it should be
firmware?

Maybe its not the firmwares' job to mark it as reserved, but just pass
it to kernel and the kernel is supposed to make sure it gets reserved
in a proper way, even across kexecs.

I think in the end whoevers' responsibility it is, the easiest path forward
seems to be in kernel? (and not firmware or libstub)

> 
>> The next ideal place would be in libstub. However, it looks like
>> InstallMemoryAttributesTable [2] is not available as a boot service
>> call option [3], [4], and install_configuration_table does not
>> seem to work as a valid substitute.
>>
> 
> To do what, exactly?
> 

To change the memory type from System RAM to either reserved or
something more appropriate, i.e. any type that is not touched by
kexec or any other userspace.

Basically the example code I attached at the end of the cover letter in 
https://lore.kernel.org/all/20250108215957.3437660-1-usamaarif642@gmail.com/
It could be EFI_ACPI_RECLAIM_MEMORY or EFI_RESERVED_TYPE, both of which aren't
touched by kexec.

>> As a last option for a fix, this patch marks that region as reserved in
>> e820_table_firmware if it is currently E820_TYPE_RAM so that kexec doesn't
>> use it for kernel segments.
>>
>> [1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@google.com/
>> [2] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c#L100
>> [3] https://github.com/tianocore/edk2/blob/42a141800c0c26a09d2344e84a89ce4097a263ae/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c#L41
>> [4] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/firmware/efi/libstub/efistub.h#L327
>>
>> Reported-by: Breno Leitao <leitao@debian.org>
>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>> ---
>>  arch/x86/include/asm/e820/api.h | 2 ++
>>  arch/x86/kernel/e820.c          | 6 ++++++
>>  arch/x86/platform/efi/efi.c     | 9 +++++++++
>>  drivers/firmware/efi/memattr.c  | 1 +
>>  include/linux/efi.h             | 7 +++++++
>>  5 files changed, 25 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
>> index 2e74a7f0e935..4e9aa24f03bd 100644
>> --- a/arch/x86/include/asm/e820/api.h
>> +++ b/arch/x86/include/asm/e820/api.h
>> @@ -16,6 +16,8 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
>>
>>  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
>>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
>> +extern u64  e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
>> +                                       enum e820_type new_type);
>>  extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
>>  extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 82b96ed9890a..01d7d3c0d299 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -538,6 +538,12 @@ u64 __init e820__range_update_table(struct e820_table *t, u64 start, u64 size,
>>         return __e820__range_update(t, start, size, old_type, new_type);
>>  }
>>
>> +u64 __init e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
>> +                                      enum e820_type new_type)
>> +{
>> +       return __e820__range_update(e820_table_firmware, start, size, old_type, new_type);
>> +}
>> +
>>  /* Remove a range of memory from the E820 table: */
>>  u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type)
>>  {
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index a7ff189421c3..13684c5d7c05 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -168,6 +168,15 @@ static void __init do_add_efi_memmap(void)
>>         e820__update_table(e820_table);
>>  }
>>
>> +/* Reserve firmware area if it was marked as RAM */
>> +void arch_update_firmware_area(u64 addr, u64 size)
>> +{
>> +       if (e820__get_entry_type(addr, addr + size) == E820_TYPE_RAM) {
>> +               e820__range_update_firmware(addr, size, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +               e820__update_table(e820_table_firmware);
>> +       }
>> +}
>> +
>>  /*
>>   * Given add_efi_memmap defaults to 0 and there is no alternative
>>   * e820 mechanism for soft-reserved memory, import the full EFI memory
>> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
>> index d3bc161361fb..d131781e2d7b 100644
>> --- a/drivers/firmware/efi/memattr.c
>> +++ b/drivers/firmware/efi/memattr.c
>> @@ -53,6 +53,7 @@ int __init efi_memattr_init(void)
>>         size = tbl->num_entries * tbl->desc_size;
>>         tbl_size = sizeof(*tbl) + size;
>>         memblock_reserve(efi_mem_attr_table, tbl_size);
>> +       arch_update_firmware_area(efi_mem_attr_table, tbl_size);
>>         set_bit(EFI_MEM_ATTR, &efi.flags);
>>
>>  unmap:
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index e5815867aba9..8eb9698bd6a4 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1358,4 +1358,11 @@ extern struct blocking_notifier_head efivar_ops_nh;
>>  void efivars_generic_ops_register(void);
>>  void efivars_generic_ops_unregister(void);
>>
>> +#ifdef CONFIG_X86_64
>> +void __init arch_update_firmware_area(u64 addr, u64 size);
>> +#else
>> +static inline void __init arch_update_firmware_area(u64 addr, u64 size)
>> +{
>> +}
>> +#endif
>>  #endif /* _LINUX_EFI_H */
>> --
>> 2.43.5
>>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121004): https://edk2.groups.io/g/devel/message/121004
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Usama Arif via groups.io]

On 09/01/2025 15:45, Ard Biesheuvel wrote:
> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>
>> 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@debian.org>
>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>> ---
>>  drivers/firmware/efi/memattr.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>
> 
> The more I think about this, the more I feel that kexec on x86 should
> simply discard this table, and run with the firmware code RWX (which
> is not the end of the world).


By discard this table, do you mean kexec not use e820_table_firmware?
Also a very basic question, what do you mean by run with the firmware RWX?

Sorry for the very basic questions above!


> 
> The main reason is that the EFI memory map and the EFI memory
> attributes table are supposed to be a matched pair, where each RTcode
> entry in the former is broken down into multiple code and data
> segments in the latter. The amount of mangling that the x86 arch code
> does of the EFI memory map makes it intractable to ensure that they
> remain in sync, and so it is better not to bother.
> 
> 
>> 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
>>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121005): https://edk2.groups.io/g/devel/message/121005
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Gregory Price]

On Thu, Jan 09, 2025 at 04:32:10PM +0000, Usama Arif wrote:
> 
> 
> On 09/01/2025 16:15, Ard Biesheuvel wrote:
> > On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
> >>
> >> When this area is not reserved, it comes up as usable in
> >> /sys/firmware/memmap. This means that kexec, which uses that memmap
> >> to find usable memory regions, can select the region where
> >> efi_mem_attr_table is and overwrite it and relocate_kernel.
> >>
> >> Since the patch in [1] was merged, all boots after kexec
> >> are producing the warning that it introduced.
> >>
> >> Having a fix in firmware can be difficult to get through.
> > 
> > I don't follow. I don't think there is anything wrong with the
> > firmware here. Could you elaborate?
> > 
> 
> So the problem is, kexec sees this memory as System RAM, and decides
> it can be used to place an image here.
> 
> I guess the question is (and I actually don't know the answer here),
> whose responsibility is it to mark this region as reserved so that 
> its not touched by anyone else. I would have thought it should be
> firmware?
> 
> Maybe its not the firmwares' job to mark it as reserved, but just pass
> it to kernel and the kernel is supposed to make sure it gets reserved
> in a proper way, even across kexecs.

Reservation is a kernel concept, the firmware/EFI etc give the kernel
guidance - but the actual maps here are largely informational and the
kernel can do whatever it wants (within reason).

So what you're really saying is either
  a) the hardware is mis-reporting the memory map configurations
     (e.g. some EFI_* bit is missing), or
  b) the kernel isn't doing the right thing.

I believe Ard is pointing out that the additional map abstraction is just
trying to retain a "current" versus "original" configuration - and this
is more of a headache than it is worth as it appears to be causing
confusion as to which one should be respected across kexec.

So the separate maps should just be one, and we should just smash the
"original" configuration rather than keeping a copy.

Please correct me if i'm misunderstanding anything.

> 
> I think in the end whoevers' responsibility it is, the easiest path forward
> seems to be in kernel? (and not firmware or libstub)
> 
> > 
> >> The next ideal place would be in libstub. However, it looks like
> >> InstallMemoryAttributesTable [2] is not available as a boot service
> >> call option [3], [4], and install_configuration_table does not
> >> seem to work as a valid substitute.
> >>
> > 
> > To do what, exactly?
> > 
> 
> To change the memory type from System RAM to either reserved or
> something more appropriate, i.e. any type that is not touched by
> kexec or any other userspace.
> 
> Basically the example code I attached at the end of the cover letter in 
> https://lore.kernel.org/all/20250108215957.3437660-1-usamaarif642@gmail.com/
> It could be EFI_ACPI_RECLAIM_MEMORY or EFI_RESERVED_TYPE, both of which aren't
> touched by kexec.
> 
> >> As a last option for a fix, this patch marks that region as reserved in
> >> e820_table_firmware if it is currently E820_TYPE_RAM so that kexec doesn't
> >> use it for kernel segments.
> >>
> >> [1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@google.com/
> >> [2] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c#L100
> >> [3] https://github.com/tianocore/edk2/blob/42a141800c0c26a09d2344e84a89ce4097a263ae/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c#L41
> >> [4] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/firmware/efi/libstub/efistub.h#L327
> >>
> >> Reported-by: Breno Leitao <leitao@debian.org>
> >> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> >> ---
> >>  arch/x86/include/asm/e820/api.h | 2 ++
> >>  arch/x86/kernel/e820.c          | 6 ++++++
> >>  arch/x86/platform/efi/efi.c     | 9 +++++++++
> >>  drivers/firmware/efi/memattr.c  | 1 +
> >>  include/linux/efi.h             | 7 +++++++
> >>  5 files changed, 25 insertions(+)
> >>
> >> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> >> index 2e74a7f0e935..4e9aa24f03bd 100644
> >> --- a/arch/x86/include/asm/e820/api.h
> >> +++ b/arch/x86/include/asm/e820/api.h
> >> @@ -16,6 +16,8 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
> >>
> >>  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
> >>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
> >> +extern u64  e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
> >> +                                       enum e820_type new_type);
> >>  extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
> >>  extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
> >>
> >> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> >> index 82b96ed9890a..01d7d3c0d299 100644
> >> --- a/arch/x86/kernel/e820.c
> >> +++ b/arch/x86/kernel/e820.c
> >> @@ -538,6 +538,12 @@ u64 __init e820__range_update_table(struct e820_table *t, u64 start, u64 size,
> >>         return __e820__range_update(t, start, size, old_type, new_type);
> >>  }
> >>
> >> +u64 __init e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
> >> +                                      enum e820_type new_type)
> >> +{
> >> +       return __e820__range_update(e820_table_firmware, start, size, old_type, new_type);
> >> +}
> >> +
> >>  /* Remove a range of memory from the E820 table: */
> >>  u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type)
> >>  {
> >> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> >> index a7ff189421c3..13684c5d7c05 100644
> >> --- a/arch/x86/platform/efi/efi.c
> >> +++ b/arch/x86/platform/efi/efi.c
> >> @@ -168,6 +168,15 @@ static void __init do_add_efi_memmap(void)
> >>         e820__update_table(e820_table);
> >>  }
> >>
> >> +/* Reserve firmware area if it was marked as RAM */
> >> +void arch_update_firmware_area(u64 addr, u64 size)
> >> +{
> >> +       if (e820__get_entry_type(addr, addr + size) == E820_TYPE_RAM) {
> >> +               e820__range_update_firmware(addr, size, E820_TYPE_RAM, E820_TYPE_RESERVED);
> >> +               e820__update_table(e820_table_firmware);
> >> +       }
> >> +}
> >> +
> >>  /*
> >>   * Given add_efi_memmap defaults to 0 and there is no alternative
> >>   * e820 mechanism for soft-reserved memory, import the full EFI memory
> >> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> >> index d3bc161361fb..d131781e2d7b 100644
> >> --- a/drivers/firmware/efi/memattr.c
> >> +++ b/drivers/firmware/efi/memattr.c
> >> @@ -53,6 +53,7 @@ int __init efi_memattr_init(void)
> >>         size = tbl->num_entries * tbl->desc_size;
> >>         tbl_size = sizeof(*tbl) + size;
> >>         memblock_reserve(efi_mem_attr_table, tbl_size);
> >> +       arch_update_firmware_area(efi_mem_attr_table, tbl_size);
> >>         set_bit(EFI_MEM_ATTR, &efi.flags);
> >>
> >>  unmap:
> >> diff --git a/include/linux/efi.h b/include/linux/efi.h
> >> index e5815867aba9..8eb9698bd6a4 100644
> >> --- a/include/linux/efi.h
> >> +++ b/include/linux/efi.h
> >> @@ -1358,4 +1358,11 @@ extern struct blocking_notifier_head efivar_ops_nh;
> >>  void efivars_generic_ops_register(void);
> >>  void efivars_generic_ops_unregister(void);
> >>
> >> +#ifdef CONFIG_X86_64
> >> +void __init arch_update_firmware_area(u64 addr, u64 size);
> >> +#else
> >> +static inline void __init arch_update_firmware_area(u64 addr, u64 size)
> >> +{
> >> +}
> >> +#endif
> >>  #endif /* _LINUX_EFI_H */
> >> --
> >> 2.43.5
> >>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121006): https://edk2.groups.io/g/devel/message/121006
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Dave Young via groups.io]

Hi Usama,

On Thu, 9 Jan 2025 at 06:00, Usama Arif <usamaarif642@gmail.com> wrote:
>
> When this area is not reserved, it comes up as usable in
> /sys/firmware/memmap. This means that kexec, which uses that memmap
> to find usable memory regions, can select the region where
> efi_mem_attr_table is and overwrite it and relocate_kernel.

Is the attr table BOOT SERVICE DATA?  If so, does efi_mem_reserve()
work for you?
Just refer to esrt.c.

Thanks
Dave



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120996): https://edk2.groups.io/g/devel/message/120996
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel via groups.io]

On Thu, 9 Jan 2025 at 17:36, Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 09/01/2025 15:45, Ard Biesheuvel wrote:
> > On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
> >>
> >> 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@debian.org>
> >> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> >> ---
> >>  drivers/firmware/efi/memattr.c | 16 ++++++----------
> >>  1 file changed, 6 insertions(+), 10 deletions(-)
> >>
> >
> > The more I think about this, the more I feel that kexec on x86 should
> > simply discard this table, and run with the firmware code RWX (which
> > is not the end of the world).
>
>
> By discard this table, do you mean kexec not use e820_table_firmware?

No, I mean kexec ignores the memory attributes table.

> Also a very basic question, what do you mean by run with the firmware RWX?
>

The memory attributes table is an overlay for the EFI memory map that
describes which runtime code regions may be mapped with restricted
permissions. Without this table, everything will be mapped writable as
well as executable, but only in the EFI page tables, which are only
active when an EFI call is in progress.

> Sorry for the very basic questions above!
>

No worries.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120977): https://edk2.groups.io/g/devel/message/120977
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Ard Biesheuvel via groups.io]

On Thu, 9 Jan 2025 at 17:32, Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 09/01/2025 16:15, Ard Biesheuvel wrote:
> > On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
> >>
> >> When this area is not reserved, it comes up as usable in
> >> /sys/firmware/memmap. This means that kexec, which uses that memmap
> >> to find usable memory regions, can select the region where
> >> efi_mem_attr_table is and overwrite it and relocate_kernel.
> >>
> >> Since the patch in [1] was merged, all boots after kexec
> >> are producing the warning that it introduced.
> >>
> >> Having a fix in firmware can be difficult to get through.
> >
> > I don't follow. I don't think there is anything wrong with the
> > firmware here. Could you elaborate?
> >
>
> So the problem is, kexec sees this memory as System RAM, and decides
> it can be used to place an image here.
>
> I guess the question is (and I actually don't know the answer here),
> whose responsibility is it to mark this region as reserved so that
> its not touched by anyone else. I would have thought it should be
> firmware?
>

No, it is the OS. The firmware only reserves regions that are needed
for its own correct operation at runtime. For informational tables
such as this one, it is up to the OS whether it wants to reserve it
and keep it in place, consume it and release the memory, or ignore it
altogether.

> Maybe its not the firmwares' job to mark it as reserved, but just pass
> it to kernel and the kernel is supposed to make sure it gets reserved
> in a proper way, even across kexecs.
>

Indeed.

> I think in the end whoevers' responsibility it is, the easiest path forward
> seems to be in kernel? (and not firmware or libstub)
>

Agreed. But as I pointed out in the other thread, the memory
attributes table only augments the memory map with permission
information, and can be disregarded, and given how badly we mangle the
memory map on x86, maybe this is the right choice here.

> >
> >> The next ideal place would be in libstub. However, it looks like
> >> InstallMemoryAttributesTable [2] is not available as a boot service
> >> call option [3], [4], and install_configuration_table does not
> >> seem to work as a valid substitute.
> >>
> >
> > To do what, exactly?
> >
>
> To change the memory type from System RAM to either reserved or
> something more appropriate, i.e. any type that is not touched by
> kexec or any other userspace.
>
> Basically the example code I attached at the end of the cover letter in
> https://lore.kernel.org/all/20250108215957.3437660-1-usamaarif642@gmail.com/
> It could be EFI_ACPI_RECLAIM_MEMORY or EFI_RESERVED_TYPE, both of which aren't
> touched by kexec.
>

This is a kexec problem (on x86 only) so let's fix it there.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120978): https://edk2.groups.io/g/devel/message/120978
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Usama Arif via groups.io]

On 10/01/2025 07:21, Ard Biesheuvel wrote:
> On Thu, 9 Jan 2025 at 17:36, Usama Arif <usamaarif642@gmail.com> wrote:
>>
>>
>>
>> On 09/01/2025 15:45, Ard Biesheuvel wrote:
>>> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>>>
>>>> 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@debian.org>
>>>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>>>> ---
>>>>  drivers/firmware/efi/memattr.c | 16 ++++++----------
>>>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>>>
>>>
>>> The more I think about this, the more I feel that kexec on x86 should
>>> simply discard this table, and run with the firmware code RWX (which
>>> is not the end of the world).
>>
>>
>> By discard this table, do you mean kexec not use e820_table_firmware?
> 
> No, I mean kexec ignores the memory attributes table.
> 
>> Also a very basic question, what do you mean by run with the firmware RWX?
>>
> 
> The memory attributes table is an overlay for the EFI memory map that
> describes which runtime code regions may be mapped with restricted
> permissions. Without this table, everything will be mapped writable as
> well as executable, but only in the EFI page tables, which are only
> active when an EFI call is in progress.
> 

Thanks for explaining!

So basically get rid of memattr.c :)

Do you mean get rid of it only for kexec, or not do it for any
boot (including cold boot)?
I do like this idea! I couldn't find this in the git history,
but do you know if this was added in the linux kernel just
because EFI spec added support for it, or if there was a
specific security problem?

Thanks!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121007): https://edk2.groups.io/g/devel/message/121007
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Usama Arif via groups.io]

On 10/01/2025 02:50, Dave Young wrote:
> Hi Usama,
> 
> On Thu, 9 Jan 2025 at 06:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>
>> When this area is not reserved, it comes up as usable in
>> /sys/firmware/memmap. This means that kexec, which uses that memmap
>> to find usable memory regions, can select the region where
>> efi_mem_attr_table is and overwrite it and relocate_kernel.
> 
> Is the attr table BOOT SERVICE DATA?  If so, does efi_mem_reserve()
> work for you?
> Just refer to esrt.c.
> 

Hi Dave,

Its a bit difficult to reproduce the problem and therefore test the fix, but
we are seeing it a lot in production. Ard proposed the same thing in 
https://lore.kernel.org/all/6b4780a5-ada0-405e-9f0a-4d2186177f29@gmail.com/
but as I mentioned there, I dont think that efi_mem_reserve would help,
as efi_mem_reserve changes e820_table, while kexec looks at
/sys/firmware/memmap which uses e820_table_firmware.

Thanks,
Usama

> Thanks
> Dave
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121008): https://edk2.groups.io/g/devel/message/121008
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Dave Young via groups.io]

On Fri, 10 Jan 2025 at 19:12, Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 10/01/2025 02:50, Dave Young wrote:
> > Hi Usama,
> >
> > On Thu, 9 Jan 2025 at 06:00, Usama Arif <usamaarif642@gmail.com> wrote:
> >>
> >> When this area is not reserved, it comes up as usable in
> >> /sys/firmware/memmap. This means that kexec, which uses that memmap
> >> to find usable memory regions, can select the region where
> >> efi_mem_attr_table is and overwrite it and relocate_kernel.
> >
> > Is the attr table BOOT SERVICE DATA?  If so, does efi_mem_reserve()
> > work for you?
> > Just refer to esrt.c.
> >
>
> Hi Dave,
>
> Its a bit difficult to reproduce the problem and therefore test the fix, but
> we are seeing it a lot in production. Ard proposed the same thing in
> https://lore.kernel.org/all/6b4780a5-ada0-405e-9f0a-4d2186177f29@gmail.com/
> but as I mentioned there, I dont think that efi_mem_reserve would help,
> as efi_mem_reserve changes e820_table, while kexec looks at
> /sys/firmware/memmap which uses e820_table_firmware.

I sent a question to pm people, if the sysfs memmap comes from
e820_table then it will be fine. Let's see:
https://lore.kernel.org/all/CALu+AoS-nk4u=9UYP7BLS=diOxjJRf+vfv7KHXG=uXozoYazsw@mail.gmail.com/

>
> Thanks,
> Usama
>
> > Thanks
> > Dave
> >
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121009): https://edk2.groups.io/g/devel/message/121009
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Dave Young via groups.io]

On Fri, 10 Jan 2025 at 19:18, Dave Young <dyoung@redhat.com> wrote:
>
> On Fri, 10 Jan 2025 at 19:12, Usama Arif <usamaarif642@gmail.com> wrote:
> >
> >
> >
> > On 10/01/2025 02:50, Dave Young wrote:
> > > Hi Usama,
> > >
> > > On Thu, 9 Jan 2025 at 06:00, Usama Arif <usamaarif642@gmail.com> wrote:
> > >>
> > >> When this area is not reserved, it comes up as usable in
> > >> /sys/firmware/memmap. This means that kexec, which uses that memmap
> > >> to find usable memory regions, can select the region where
> > >> efi_mem_attr_table is and overwrite it and relocate_kernel.
> > >
> > > Is the attr table BOOT SERVICE DATA?  If so, does efi_mem_reserve()
> > > work for you?
> > > Just refer to esrt.c.
> > >
> >
> > Hi Dave,
> >
> > Its a bit difficult to reproduce the problem and therefore test the fix, but
> > we are seeing it a lot in production. Ard proposed the same thing in
> > https://lore.kernel.org/all/6b4780a5-ada0-405e-9f0a-4d2186177f29@gmail.com/
> > but as I mentioned there, I dont think that efi_mem_reserve would help,
> > as efi_mem_reserve changes e820_table, while kexec looks at
> > /sys/firmware/memmap which uses e820_table_firmware.
>
> I sent a question to pm people, if the sysfs memmap comes from
> e820_table then it will be fine. Let's see:
s/e820_table/e820_table_kexec

> https://lore.kernel.org/all/CALu+AoS-nk4u=9UYP7BLS=diOxjJRf+vfv7KHXG=uXozoYazsw@mail.gmail.com/
>
> >
> > Thanks,
> > Usama
> >
> > > Thanks
> > > Dave
> > >
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121010): https://edk2.groups.io/g/devel/message/121010
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Breno Leitao]

Hello Ard,

On Fri, Jan 10, 2025 at 08:32:08AM +0100, Ard Biesheuvel wrote:
> On Thu, 9 Jan 2025 at 17:32, Usama Arif <usamaarif642@gmail.com> wrote:

> > I think in the end whoevers' responsibility it is, the easiest path forward
> > seems to be in kernel? (and not firmware or libstub)
> >
> 
> Agreed. But as I pointed out in the other thread, the memory
> attributes table only augments the memory map with permission
> information, and can be disregarded, and given how badly we mangle the
> memory map on x86, maybe this is the right choice here.

If this augmented memory is not preserved accross kexec, then the next
kexec'ed kernel will be able to find the original table?

I understand that the memattr region(s) need to be always (in each kexec
instances) `memblocked_reserved` to protect it from being used as a
System RAM, right?

Thus, if it is not passed throught e820, kexec'ed kernel needs to fetch
it again from original EFI table at kexec/boot time. 

This brings me another question.

If the kexec'ed kernel sees the original memory, why can't it
augment/update the RX permissions *again*, instead of passing the
previous augmented version from previous kernel in this crazy dance.

> This is a kexec problem (on x86 only) so let's fix it there.

Would you mind explaining what kexec needs to be done differently?
Should it preserve the augmented memattr table independently if it is
mapped in e820?

Thank you!
--breno


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121011): https://edk2.groups.io/g/devel/message/121011
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Usama Arif via groups.io]

On 10/01/2025 11:20, Dave Young wrote:
> On Fri, 10 Jan 2025 at 19:18, Dave Young <dyoung@redhat.com> wrote:
>>
>> On Fri, 10 Jan 2025 at 19:12, Usama Arif <usamaarif642@gmail.com> wrote:
>>>
>>>
>>>
>>> On 10/01/2025 02:50, Dave Young wrote:
>>>> Hi Usama,
>>>>
>>>> On Thu, 9 Jan 2025 at 06:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>>>>
>>>>> When this area is not reserved, it comes up as usable in
>>>>> /sys/firmware/memmap. This means that kexec, which uses that memmap
>>>>> to find usable memory regions, can select the region where
>>>>> efi_mem_attr_table is and overwrite it and relocate_kernel.
>>>>
>>>> Is the attr table BOOT SERVICE DATA?  If so, does efi_mem_reserve()
>>>> work for you?
>>>> Just refer to esrt.c.
>>>>
>>>
>>> Hi Dave,
>>>
>>> Its a bit difficult to reproduce the problem and therefore test the fix, but
>>> we are seeing it a lot in production. Ard proposed the same thing in
>>> https://lore.kernel.org/all/6b4780a5-ada0-405e-9f0a-4d2186177f29@gmail.com/
>>> but as I mentioned there, I dont think that efi_mem_reserve would help,
>>> as efi_mem_reserve changes e820_table, while kexec looks at
>>> /sys/firmware/memmap which uses e820_table_firmware.
>>
>> I sent a question to pm people, if the sysfs memmap comes from
>> e820_table then it will be fine. Let's see:
> s/e820_table/e820_table_kexec
> 

Do you mean change /sys/firmware/memmap to point to e820_table_kexec instead
of e820_table_firmware [1]? 
I thought of doing this when the first bug was encountered last year, but
didn't do it as I thought it would be frowned upon to change what sysfs file
exposes to userspace.

[1] https://elixir.bootlin.com/linux/v6.12.6/source/arch/x86/kernel/e820.c#L31


>> https://lore.kernel.org/all/CALu+AoS-nk4u=9UYP7BLS=diOxjJRf+vfv7KHXG=uXozoYazsw@mail.gmail.com/
>>
>>>
>>> Thanks,
>>> Usama
>>>
>>>> Thanks
>>>> Dave
>>>>
>>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121012): https://edk2.groups.io/g/devel/message/121012
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Usama Arif via groups.io]

On 10/01/2025 07:32, Ard Biesheuvel wrote:
> On Thu, 9 Jan 2025 at 17:32, Usama Arif <usamaarif642@gmail.com> wrote:
>>
>>
>>
>> On 09/01/2025 16:15, Ard Biesheuvel wrote:

>> I think in the end whoevers' responsibility it is, the easiest path forward
>> seems to be in kernel? (and not firmware or libstub)
>>
> 
> Agreed. But as I pointed out in the other thread, the memory
> attributes table only augments the memory map with permission
> information, and can be disregarded, and given how badly we mangle the
> memory map on x86, maybe this is the right choice here.
> 
>>>
>>>> The next ideal place would be in libstub. However, it looks like
>>>> InstallMemoryAttributesTable [2] is not available as a boot service
>>>> call option [3], [4], and install_configuration_table does not
>>>> seem to work as a valid substitute.
>>>>
>>>
>>> To do what, exactly?
>>>
>>
>> To change the memory type from System RAM to either reserved or
>> something more appropriate, i.e. any type that is not touched by
>> kexec or any other userspace.
>>
>> Basically the example code I attached at the end of the cover letter in
>> https://lore.kernel.org/all/20250108215957.3437660-1-usamaarif642@gmail.com/
>> It could be EFI_ACPI_RECLAIM_MEMORY or EFI_RESERVED_TYPE, both of which aren't
>> touched by kexec.
>>
> 
> This is a kexec problem (on x86 only) so let's fix it there.


I don't believe we can accurately tell if we are booting from a cold boot or kexec.
There is bootloader_type available for x86, but not sure if we should rely on
that. I think a way forward would be to move it behind a Kconfig option, something like
below, which defaults to n for x86. Anyone who needs it can enable it. What do you think?


diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index aa95f77d7a30..31deb0a5371e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -83,7 +83,9 @@ static const unsigned long * const efi_tables[] = {
        &efi_config_table,
        &efi.esrt,
        &prop_phys,
+#ifdef CONFIG_EFI_MEMATTR
        &efi_mem_attr_table,
+#endif
 #ifdef CONFIG_EFI_RCI2_TABLE
        &rci2_table_phys,
 #endif
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 72f2537d90ca..b8ecb318768c 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -287,6 +287,13 @@ config EFI_EMBEDDED_FIRMWARE
        bool
        select CRYPTO_LIB_SHA256
 
+config EFI_MEMATTR
+       bool "EFI Memory attributes table"
+       default n if X86_64
+       help
+          EFI Memory Attributes table describes memory protections that may
+          be applied to the EFI Runtime code and data regions by the kernel.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index a2d0009560d0..c593ec0d9940 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -11,7 +11,9 @@
 KASAN_SANITIZE_runtime-wrappers.o      := n
 
 obj-$(CONFIG_ACPI_BGRT)                += efi-bgrt.o
-obj-$(CONFIG_EFI)                      += efi.o vars.o reboot.o memattr.o tpm.o
+obj-$(CONFIG_EFI)                      += efi.o vars.o reboot.o tpm.o
+obj-$(CONFIG_EFI_MEMATTR)              += memattr.o
+
 obj-$(CONFIG_EFI)                      += memmap.o
 ifneq ($(CONFIG_EFI_CAPSULE_LOADER),)
 obj-$(CONFIG_EFI)                      += capsule.o
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index fdf07dd6f459..f359179083d5 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -596,7 +596,9 @@ static const efi_config_table_type_t common_tables[] __initconst = {
        {SMBIOS_TABLE_GUID,                     &efi.smbios,            "SMBIOS"        },
        {SMBIOS3_TABLE_GUID,                    &efi.smbios3,           "SMBIOS 3.0"    },
        {EFI_SYSTEM_RESOURCE_TABLE_GUID,        &efi.esrt,              "ESRT"          },
+#ifdef CONFIG_EFI_MEMATTR
        {EFI_MEMORY_ATTRIBUTES_TABLE_GUID,      &efi_mem_attr_table,    "MEMATTR"       },
+#endif
        {LINUX_EFI_RANDOM_SEED_TABLE_GUID,      &efi_rng_seed,          "RNG"           },
        {LINUX_EFI_TPM_EVENT_LOG_GUID,          &efi.tpm_log,           "TPMEventLog"   },
        {EFI_TCG2_FINAL_EVENTS_TABLE_GUID,      &efi.tpm_final_log,     "TPMFinalLog"   },
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9c239cdff771..4cf5ebe014e2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -783,9 +783,21 @@ extern unsigned long efi_mem_attr_table;
  */
 typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *, bool);
 
+#ifdef CONFIG_EFI_MEMATTR
 extern int efi_memattr_init(void);
 extern int efi_memattr_apply_permissions(struct mm_struct *mm,
                                         efi_memattr_perm_setter fn);
+#else
+static inline int efi_memattr_init(void)
+{
+       return 0;
+}
+static inline int efi_memattr_apply_permissions(struct mm_struct *mm,
+                                        efi_memattr_perm_setter fn)
+{
+       return 0;
+}
+#endif
 
 /*
  * efi_memdesc_ptr - get the n-th EFI memmap descriptor


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121013): https://edk2.groups.io/g/devel/message/121013
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Usama Arif via groups.io]

On 10/01/2025 14:31, Usama Arif wrote:
> 
> 
> On 10/01/2025 07:32, Ard Biesheuvel wrote:
>> On Thu, 9 Jan 2025 at 17:32, Usama Arif <usamaarif642@gmail.com> wrote:
>>>
>>>
>>>
>>> On 09/01/2025 16:15, Ard Biesheuvel wrote:
> 
>>> I think in the end whoevers' responsibility it is, the easiest path forward
>>> seems to be in kernel? (and not firmware or libstub)
>>>
>>
>> Agreed. But as I pointed out in the other thread, the memory
>> attributes table only augments the memory map with permission
>> information, and can be disregarded, and given how badly we mangle the
>> memory map on x86, maybe this is the right choice here.
>>
>>>>
>>>>> The next ideal place would be in libstub. However, it looks like
>>>>> InstallMemoryAttributesTable [2] is not available as a boot service
>>>>> call option [3], [4], and install_configuration_table does not
>>>>> seem to work as a valid substitute.
>>>>>
>>>>
>>>> To do what, exactly?
>>>>
>>>
>>> To change the memory type from System RAM to either reserved or
>>> something more appropriate, i.e. any type that is not touched by
>>> kexec or any other userspace.
>>>
>>> Basically the example code I attached at the end of the cover letter in
>>> https://lore.kernel.org/all/20250108215957.3437660-1-usamaarif642@gmail.com/
>>> It could be EFI_ACPI_RECLAIM_MEMORY or EFI_RESERVED_TYPE, both of which aren't
>>> touched by kexec.
>>>
>>
>> This is a kexec problem (on x86 only) so let's fix it there.
> 
> 
> I don't believe we can accurately tell if we are booting from a cold boot or kexec.
> There is bootloader_type available for x86, but not sure if we should rely on
> that. I think a way forward would be to move it behind a Kconfig option, something like
> below, which defaults to n for x86. Anyone who needs it can enable it. What do you think?
> 

Or we can do something like below?

diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index d131781e2d7b..4add694b18d0 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -24,6 +24,15 @@ int __init efi_memattr_init(void)
        efi_memory_attributes_table_t *tbl;
        unsigned long size;
 
+#ifdef CONFIG_X86_64
+       /*
+        * On x86_64, do not initialize memory attributes table
+        * if booting from kexec
+        */
+       if (bootloader_type >> 4 == 0xd)
+               return 0;
+#endif
+
        if (efi_mem_attr_table == EFI_INVALID_TABLE_ADDR)
                return 0;


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121014): https://edk2.groups.io/g/devel/message/121014
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel via groups.io]

On Fri, 10 Jan 2025 at 11:53, Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 10/01/2025 07:21, Ard Biesheuvel wrote:
> > On Thu, 9 Jan 2025 at 17:36, Usama Arif <usamaarif642@gmail.com> wrote:
> >>
> >>
> >>
> >> On 09/01/2025 15:45, Ard Biesheuvel wrote:
> >>> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
> >>>>
> >>>> 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@debian.org>
> >>>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> >>>> ---
> >>>>  drivers/firmware/efi/memattr.c | 16 ++++++----------
> >>>>  1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>
> >>>
> >>> The more I think about this, the more I feel that kexec on x86 should
> >>> simply discard this table, and run with the firmware code RWX (which
> >>> is not the end of the world).
> >>
> >>
> >> By discard this table, do you mean kexec not use e820_table_firmware?
> >
> > No, I mean kexec ignores the memory attributes table.
> >
> >> Also a very basic question, what do you mean by run with the firmware RWX?
> >>
> >
> > The memory attributes table is an overlay for the EFI memory map that
> > describes which runtime code regions may be mapped with restricted
> > permissions. Without this table, everything will be mapped writable as
> > well as executable, but only in the EFI page tables, which are only
> > active when an EFI call is in progress.
> >
>
> Thanks for explaining!
>
> So basically get rid of memattr.c :)
>
> Do you mean get rid of it only for kexec, or not do it for any
> boot (including cold boot)?

Only for kexec, and only on x86

> I do like this idea! I couldn't find this in the git history,
> but do you know if this was added in the linux kernel just
> because EFI spec added support for it, or if there was a
> specific security problem?
>

Mapping memory RWX is generally a bad idea, so we should avoid it if
possible. But EFI runtime memory regions are only mapped during a EFI
runtime call, and these don't happen often at all, so the benefit is
only marginal. (In the early days of EFI, it was more common for the
OS to map these regions permanently, but we stopped doing that a long
time ago)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120980): https://edk2.groups.io/g/devel/message/120980
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC 2/2] efi/memattr: add efi_mem_attr_table as a reserved region in 820_table_firmware

[Ard Biesheuvel via groups.io]

On Fri, 10 Jan 2025 at 12:36, Breno Leitao <leitao@debian.org> wrote:
>
> Hello Ard,
>
> On Fri, Jan 10, 2025 at 08:32:08AM +0100, Ard Biesheuvel wrote:
> > On Thu, 9 Jan 2025 at 17:32, Usama Arif <usamaarif642@gmail.com> wrote:
>
> > > I think in the end whoevers' responsibility it is, the easiest path forward
> > > seems to be in kernel? (and not firmware or libstub)
> > >
> >
> > Agreed. But as I pointed out in the other thread, the memory
> > attributes table only augments the memory map with permission
> > information, and can be disregarded, and given how badly we mangle the
> > memory map on x86, maybe this is the right choice here.
>
> If this augmented memory is not preserved accross kexec, then the next
> kexec'ed kernel will be able to find the original table?
>
> I understand that the memattr region(s) need to be always (in each kexec
> instances) `memblocked_reserved` to protect it from being used as a
> System RAM, right?
>
> Thus, if it is not passed throught e820, kexec'ed kernel needs to fetch
> it again from original EFI table at kexec/boot time.
>

Not sure what 'fetching' means here.

> This brings me another question.
>
> If the kexec'ed kernel sees the original memory, why can't it
> augment/update the RX permissions *again*, instead of passing the
> previous augmented version from previous kernel in this crazy dance.
>

I don't understand what original memory means. I think we're talking
past each other tbh.

> > This is a kexec problem (on x86 only) so let's fix it there.
>
> Would you mind explaining what kexec needs to be done differently?
> Should it preserve the augmented memattr table independently if it is
> mapped in e820?
>

I don't know what 'mapped in e820' means.

Let's forget about what the memory attributes table actually contains,
and just assume we can live without it, ok?

So when booting x86 via kexec (which is already detected in
arch/x86/platform/efi/efi.c), the kernel should pretend that the table
does not exist.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120981): https://edk2.groups.io/g/devel/message/120981
Mute This Topic: https://groups.io/mt/110518541/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Dave Young via groups.io]

On Fri, 10 Jan 2025 at 18:54, Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 10/01/2025 07:21, Ard Biesheuvel wrote:
> > On Thu, 9 Jan 2025 at 17:36, Usama Arif <usamaarif642@gmail.com> wrote:
> >>
> >>
> >>
> >> On 09/01/2025 15:45, Ard Biesheuvel wrote:
> >>> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
> >>>>
> >>>> 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@debian.org>
> >>>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> >>>> ---
> >>>>  drivers/firmware/efi/memattr.c | 16 ++++++----------
> >>>>  1 file changed, 6 insertions(+), 10 deletions(-)
> >>>>
> >>>
> >>> The more I think about this, the more I feel that kexec on x86 should
> >>> simply discard this table, and run with the firmware code RWX (which
> >>> is not the end of the world).
> >>
> >>
> >> By discard this table, do you mean kexec not use e820_table_firmware?
> >
> > No, I mean kexec ignores the memory attributes table.
> >
> >> Also a very basic question, what do you mean by run with the firmware RWX?
> >>
> >
> > The memory attributes table is an overlay for the EFI memory map that
> > describes which runtime code regions may be mapped with restricted
> > permissions. Without this table, everything will be mapped writable as
> > well as executable, but only in the EFI page tables, which are only
> > active when an EFI call is in progress.
> >
>
> Thanks for explaining!
>
> So basically get rid of memattr.c :)
>
> Do you mean get rid of it only for kexec, or not do it for any
> boot (including cold boot)?
> I do like this idea! I couldn't find this in the git history,
> but do you know if this was added in the linux kernel just
> because EFI spec added support for it, or if there was a
> specific security problem?
>

Usama, can you try the patch below?
[ format is wrong due to webmail corruption.  But if it works I can
send a formal patch later ]

$ git diff arch/x86
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 846bf49f2508..58dc77c5210e 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -561,6 +561,11 @@ int __init efi_reuse_config(u64 tables, int nr_tables)

                if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
                        ((efi_config_table_64_t *)p)->table = data->smbios;
+
+               /* Not bother to play with mem attr table across kexec */
+               if (!efi_guidcmp(guid, EFI_MEMORY_ATTRIBUTES_TABLE_GUID))
+                       ((efi_config_table_64_t *)p)->table =
EFI_INVALID_TABLE_ADDR;
+
                p += sz;
        }



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121015): https://edk2.groups.io/g/devel/message/121015
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Usama Arif via groups.io]

On 13/01/2025 02:33, Dave Young wrote:
> On Fri, 10 Jan 2025 at 18:54, Usama Arif <usamaarif642@gmail.com> wrote:
>>
>>
>>
>> On 10/01/2025 07:21, Ard Biesheuvel wrote:
>>> On Thu, 9 Jan 2025 at 17:36, Usama Arif <usamaarif642@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 09/01/2025 15:45, Ard Biesheuvel wrote:
>>>>> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>>>>>
>>>>>> 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@debian.org>
>>>>>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>>>>>> ---
>>>>>>  drivers/firmware/efi/memattr.c | 16 ++++++----------
>>>>>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>>
>>>>>
>>>>> The more I think about this, the more I feel that kexec on x86 should
>>>>> simply discard this table, and run with the firmware code RWX (which
>>>>> is not the end of the world).
>>>>
>>>>
>>>> By discard this table, do you mean kexec not use e820_table_firmware?
>>>
>>> No, I mean kexec ignores the memory attributes table.
>>>
>>>> Also a very basic question, what do you mean by run with the firmware RWX?
>>>>
>>>
>>> The memory attributes table is an overlay for the EFI memory map that
>>> describes which runtime code regions may be mapped with restricted
>>> permissions. Without this table, everything will be mapped writable as
>>> well as executable, but only in the EFI page tables, which are only
>>> active when an EFI call is in progress.
>>>
>>
>> Thanks for explaining!
>>
>> So basically get rid of memattr.c :)
>>
>> Do you mean get rid of it only for kexec, or not do it for any
>> boot (including cold boot)?
>> I do like this idea! I couldn't find this in the git history,
>> but do you know if this was added in the linux kernel just
>> because EFI spec added support for it, or if there was a
>> specific security problem?
>>
> 
> Usama, can you try the patch below?
> [ format is wrong due to webmail corruption.  But if it works I can
> send a formal patch later ]
> 
> $ git diff arch/x86
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 846bf49f2508..58dc77c5210e 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -561,6 +561,11 @@ int __init efi_reuse_config(u64 tables, int nr_tables)
> 
>                 if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
>                         ((efi_config_table_64_t *)p)->table = data->smbios;
> +
> +               /* Not bother to play with mem attr table across kexec */
> +               if (!efi_guidcmp(guid, EFI_MEMORY_ATTRIBUTES_TABLE_GUID))
> +                       ((efi_config_table_64_t *)p)->table =
> EFI_INVALID_TABLE_ADDR;
> +
>                 p += sz;
>         }
> 

This would work, I am guessing it will have a similar effect to what I sent
last week in 
https://lore.kernel.org/all/fd63613c-fd26-42de-b5ed-cc734f72eb36@gmail.com/

I think it needs to be wrapped in ifdef CONFIG_X86_64.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121017): https://edk2.groups.io/g/devel/message/121017
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Usama Arif via groups.io]

On 13/01/2025 11:27, Usama Arif wrote:
> 
> 
> On 13/01/2025 02:33, Dave Young wrote:
>> On Fri, 10 Jan 2025 at 18:54, Usama Arif <usamaarif642@gmail.com> wrote:
>>>
>>>
>>>
>>> On 10/01/2025 07:21, Ard Biesheuvel wrote:
>>>> On Thu, 9 Jan 2025 at 17:36, Usama Arif <usamaarif642@gmail.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 09/01/2025 15:45, Ard Biesheuvel wrote:
>>>>>> On Wed, 8 Jan 2025 at 23:00, Usama Arif <usamaarif642@gmail.com> wrote:
>>>>>>>
>>>>>>> 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@debian.org>
>>>>>>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/firmware/efi/memattr.c | 16 ++++++----------
>>>>>>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>
>>>>>> The more I think about this, the more I feel that kexec on x86 should
>>>>>> simply discard this table, and run with the firmware code RWX (which
>>>>>> is not the end of the world).
>>>>>
>>>>>
>>>>> By discard this table, do you mean kexec not use e820_table_firmware?
>>>>
>>>> No, I mean kexec ignores the memory attributes table.
>>>>
>>>>> Also a very basic question, what do you mean by run with the firmware RWX?
>>>>>
>>>>
>>>> The memory attributes table is an overlay for the EFI memory map that
>>>> describes which runtime code regions may be mapped with restricted
>>>> permissions. Without this table, everything will be mapped writable as
>>>> well as executable, but only in the EFI page tables, which are only
>>>> active when an EFI call is in progress.
>>>>
>>>
>>> Thanks for explaining!
>>>
>>> So basically get rid of memattr.c :)
>>>
>>> Do you mean get rid of it only for kexec, or not do it for any
>>> boot (including cold boot)?
>>> I do like this idea! I couldn't find this in the git history,
>>> but do you know if this was added in the linux kernel just
>>> because EFI spec added support for it, or if there was a
>>> specific security problem?
>>>
>>
>> Usama, can you try the patch below?
>> [ format is wrong due to webmail corruption.  But if it works I can
>> send a formal patch later ]
>>
>> $ git diff arch/x86
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 846bf49f2508..58dc77c5210e 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -561,6 +561,11 @@ int __init efi_reuse_config(u64 tables, int nr_tables)
>>
>>                 if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
>>                         ((efi_config_table_64_t *)p)->table = data->smbios;
>> +
>> +               /* Not bother to play with mem attr table across kexec */
>> +               if (!efi_guidcmp(guid, EFI_MEMORY_ATTRIBUTES_TABLE_GUID))
>> +                       ((efi_config_table_64_t *)p)->table =
>> EFI_INVALID_TABLE_ADDR;
>> +
>>                 p += sz;
>>         }
>>
> 
> This would work, I am guessing it will have a similar effect to what I sent
> last week in 
> https://lore.kernel.org/all/fd63613c-fd26-42de-b5ed-cc734f72eb36@gmail.com/
> 
> I think it needs to be wrapped in ifdef CONFIG_X86_64.
> 

IMO we should consider the 2 patches in this series first before disabling it for
kexec. These patches actually fix the issue.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121018): https://edk2.groups.io/g/devel/message/121018
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel via groups.io]

On Mon, 20 Jan 2025 at 11:27, Usama Arif <usamaarif642@gmail.com> wrote:
>
>
...
> Hi Ard,
>
> Just wanted to check how should we proceed forward? Should we try and fix the warning
> and corruption during kexec as done in this series or not initialize memory attributes
> table at all in kexec boot? I would prefer fixing the issues as in this series.
>

I would prefer kexec boot on x86 to disregard the memory attributes
table entirely.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121031): https://edk2.groups.io/g/devel/message/121031
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel via groups.io]

On Mon, 20 Jan 2025 at 11:50, Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 20/01/2025 10:32, Ard Biesheuvel wrote:
> > On Mon, 20 Jan 2025 at 11:27, Usama Arif <usamaarif642@gmail.com> wrote:
> >>
> >>
> > ...
> >> Hi Ard,
> >>
> >> Just wanted to check how should we proceed forward? Should we try and fix the warning
> >> and corruption during kexec as done in this series or not initialize memory attributes
> >> table at all in kexec boot? I would prefer fixing the issues as in this series.
> >>
> >
> > I would prefer kexec boot on x86 to disregard the memory attributes
> > table entirely.
>
> Would you like Dave to send something like
> https://lore.kernel.org/all/CALu+AoS8tb=HgaybDw5OG4A1QbOXHvuidpu0ynmz-nE1nBqzTA@mail.gmail.com/
> on the mailing list (wrapped in ifdef CONFIG_X86_64)
>

I prefer this approach. and no need for the ifdef, this is x86
specific code, and the memory attributes table is already ignored
entirely on 32-bit x86 iirc


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121032): https://edk2.groups.io/g/devel/message/121032
Mute This Topic: https://groups.io/mt/110517813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-