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: <aKgzY27lDreFXxis@sumit-X1>
Date: Fri, 22 Aug 2025 14:37:47 +0530
From: Sumit Garg <sumit.garg@...nel.org>
To: Jan Kiszka <jan.kiszka@...mens.com>
Cc: Ilias Apalodimas <ilias.apalodimas@...aro.org>,
	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>
Subject: Re: [PATCH 1/3] efi: stmm: Fix incorrect buffer allocation method

On Thu, Aug 21, 2025 at 02:56:59PM +0200, Jan Kiszka wrote:
> On 21.08.25 11:35, Sumit Garg wrote:
> > Hi Jan,
> > 
> > On Wed, Aug 20, 2025 at 05:05:43PM +0200, Jan Kiszka wrote:
> >> 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.
> > 
> > The rework of tee_shm_register_kernel_buf() to directly accept kernel
> > pages instead of buffer pointers is already due. If you are willing to
> > fix existing TEE client drivers and the API then I will be happy to
> > review them.
> 
> I'm currently testing the stmm quite a bit but I have no setup/use case
> for the trusted_tee so far. Testing is eating most of the time,
> specifically with these seriously complex firmware security stacks.

For TEE based trusted keys, it is rather a bit straigtforward to run
tests using OP-TEE Qemu build setup where you would only need to patch
the kernel.

Test command:

$ make -j$(nproc) CHECK_TESTS="trusted-keys" check

-Sumit