linux-kernel - Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <75db5f5e-9e0c-4c48-a3c8-034414276036@siemens.com>
Date: Wed, 20 Aug 2025 17:05:43 +0200
From: Jan Kiszka <jan.kiszka@...mens.com>
To: Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc: Ard Biesheuvel <ardb@...nel.org>, linux-efi@...r.kernel.org,
 linux-kernel@...r.kernel.org, Jens Wiklander <jens.wiklander@...aro.org>,
 Masahisa Kojima <kojima.masahisa@...ionext.com>,
 Sumit Garg <sumit.garg@...nel.org>
Subject: Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

On 20.08.25 09:29, Ilias Apalodimas wrote:
> (++cc Sumit and Kojima-san on their updated emails)
> 
> On Fri, 15 Aug 2025 at 22:12, Jan Kiszka <jan.kiszka@...mens.com> wrote:
>>
>> From: Jan Kiszka <jan.kiszka@...mens.com>
>>
>> The communication buffer allocated by setup_mm_hdr is later on passed to
>> tee_shm_register_kernel_buf. The latter expects those buffers to be
>> contiguous pages, but setup_mm_hdr just uses kmalloc. That can cause
>> various corruptions or BUGs, specifically since 9aec2fb0fd5e, though it
>> was broken before as well.
>>
>> Fix this by using alloc_pages_exact instead of kmalloc.
>>
>> Fixes: c44b6be62e8d ("efi: Add tee-based EFI variable driver")
>> Signed-off-by: Jan Kiszka <jan.kiszka@...mens.com>
>> ---
> 
> [...]
> 
>>         const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
>>         struct efi_mm_communicate_header *mm_hdr;
>> @@ -173,9 +174,12 @@ static void *setup_mm_hdr(u8 **dptr, size_t payload_size, size_t func,
>>                 return NULL;
>>         }
>>
>> -       comm_buf = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
>> -                                  MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
>> -                          GFP_KERNEL);
>> +       *nr_pages = roundup(MM_COMMUNICATE_HEADER_SIZE +
>> +                           MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
>> +                           PAGE_SIZE) / PAGE_SIZE;
>> +
>> +       comm_buf = alloc_pages_exact(*nr_pages * PAGE_SIZE,
>> +                                    GFP_KERNEL | __GFP_ZERO);
> 
> Rename nr_pages to something else and skip division, multiplying.
> Unless there's a reason I am missing?
> Also doesn't alloc_pages_exact() already rounds things up?

I was looking at tee_dyn_shm_alloc_helper and the dance it does to
calculate the pages from the size parameter. Digging into
alloc_pages_exact, though, suggests that the get_order(size) there will
already do what we need. Let me rework this.

Jan

> 
>>         if (!comm_buf) {
>>                 *ret = EFI_OUT_OF_RESOURCES;
>>                 return NULL;
>> @@ -205,13 +209,14 @@ static efi_status_t get_max_payload(size_t *size)
>>         struct smm_variable_payload_size *var_payload = NULL;
>>         size_t payload_size;
>>         u8 *comm_buf = NULL;
> 
> [...]
> 
> Thanks
> /Ilias


-- 
Siemens AG, Foundational Technologies
Linux Expert Center