Re: [PATCH 3/3] efi/memattr: Include EFI memmap size in corruption warnings

[Usama Arif <usamaarif642@gmail.com>]

On 09/01/2025 14:48, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2025 at 13:05, Breno Leitao <leitao@debian.org> wrote:
>>
>> Hello Ard,
>>
>> On Tue, Jan 07, 2025 at 12:24:03PM +0100, Ard Biesheuvel wrote:
>>> On Mon, 6 Jan 2025 at 20:03, Breno Leitao <leitao@debian.org> wrote:
>>>>
>>>> Add EFI memory map size to warning messages when a corrupted Memory
>>>> Attributes Table is detected, making it easier to diagnose firmware issues.
>>>>
>>>> Signed-off-by: Breno Leitao <leitao@debian.org>
>>>> ---
>>>>  drivers/firmware/efi/memattr.c | 9 +++++----
>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
>>>> index 5f83cdea88b05cb325e9f90c14a0048131e53cfa..2c276bcc0df48352bec6cd96b69edf67a16f6069 100644
>>>> --- a/drivers/firmware/efi/memattr.c
>>>> +++ b/drivers/firmware/efi/memattr.c
>>>> @@ -22,7 +22,7 @@ unsigned long __ro_after_init efi_mem_attr_table = EFI_INVALID_TABLE_ADDR;
>>>>  void __init efi_memattr_init(void)
>>>>  {
>>>>         efi_memory_attributes_table_t *tbl;
>>>> -       unsigned long size;
>>>> +       unsigned long size, efi_map_size;
>>>>
>>>>         if (efi_mem_attr_table == EFI_INVALID_TABLE_ADDR)
>>>>                 return;
>>>> @@ -49,9 +49,10 @@ void __init efi_memattr_init(void)
>>>>          * just be ignored altogether.
>>>>          */
>>>>         size = tbl->num_entries * tbl->desc_size;
>>>> -       if (size > 3 * efi.memmap.nr_map * efi.memmap.desc_size) {
>>>> -               pr_warn(FW_BUG "Corrupted EFI Memory Attributes Table detected! (version == %u, desc_size == %u, num_entries == %u)\n",
>>>> -                       tbl->version, tbl->desc_size, tbl->num_entries);
>>>> +       efi_map_size = efi.memmap.nr_map * efi.memmap.desc_size;
>>>> +       if (size > 3 * efi_map_size) {
>>>> +               pr_warn(FW_BUG "Corrupted EFI Memory Attributes Table detected! (version == %u, desc_size == %u, num_entries == %u, efi_map_size == %lu)\n",
>>>> +                       tbl->version, tbl->desc_size, tbl->num_entries, efi_map_size);
>>>>                 goto unmap;
>>>>         }
>>>>
>>>>
>>>
>>> Hello Breno,
>>>
>>> I don't mind the patch per se, but I don't think it is terribly useful either.
>>>
>>> Could you explain how this helps?
>>
>> We are seeing a bunch of `Corrupted EFI Memory Attributes Table
>> detected!` in the Meta fleet, and this is something we are
>> investigating.
>>
>> We highly think this is related to some kexec overwrites, and when we
>> get here, the EFI table is completely garbage. I haven't seen this
>> problem on cold boot.
>>
> 
> It likely means the memory is not reserved correctly.
> 
> Could you check whether this
> 
> --- a/drivers/firmware/efi/memattr.c
> +++ b/drivers/firmware/efi/memattr.c
> @@ -56,7 +56,7 @@ int __init efi_memattr_init(void)
>         }
> 
>         tbl_size = sizeof(*tbl) + size;
> -       memblock_reserve(efi_mem_attr_table, tbl_size);
> +       efi_mem_reserve(efi_mem_attr_table, tbl_size);
>         set_bit(EFI_MEM_ATTR, &efi.flags);
> 
>  unmap:
> 
> 
> makes any difference?
> 
Hi Ard,

Thanks for the reply!

I have further explained the problems and possible solutions in
https://lore.kernel.org/all/20250108215957.3437660-1-usamaarif642@gmail.com/.

I am assuming the above diff is to solve problem 2 that I have described in the patches.
I haven't tested it, because its a bit difficult to reproduce problem 2, but I think
the above diff might not make a difference? efi_mem_reserve changes e820_table,
while /sys/firmware/memmap uses e820_table_firmware. An alternate solution might be
to change /sys/firmware/memmap to e820_table. I didnt go down that route because,
you would be changing what the kernel exposes to userspace, which might not be the
right thing.

Thanks,
Usama


> 
>> Here are sof the instances I see:
>>
>>         efi: memattr: [Firmware Bug]: Corrupted EFI Memory Attributes Table detected! (version == 0, desc_size == 18058, num_entries == 33554432)
>>         efi: memattr: [Firmware Bug]: Corrupted EFI Memory Attributes Table detected! (version == 1, desc_size == 2072184435, num_entries == 3248688968)
>>         efi: memattr: [Firmware Bug]: Corrupted EFI Memory Attributes Table detected! (version == 0, desc_size == 83886080, num_entries == 304)
>>         efi: memattr: [Firmware Bug]: Corrupted EFI Memory Attributes Table detected! (version == 2, desc_size == 48, num_entries == 40)
>>
> 
> The last one looks like a false positive: each of those values seems
> perfectly reasonable.
> 
> Any chance you could dump the memory map and this table (boot using
> efi=debug) on this system?
> 
> 
>> Anyway, back to you question, this patch helped us to narrow down and
>> find where the problem was, by printing all variables taken in
>> consideration to get the conclusion that the firmware is buggy.
>>
> 
> Fair enough.
> 
>> Regarding the problem, Usama and I are suspecting that it might be
>> related to some 77d48d39e99170 ("efistub/tpm: Use ACPI reclaim memory for
>> event log to avoid corruption"), but at this time with memattr table, where it
>> might not preserved during kexec(?).
>>
> 
> Please see the suggestion above.