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

["Ard Biesheuvel via groups.io" <ardb=kernel.org@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]
-=-=-=-=-=-=-=-=-=-=-=-