linux-kernel - Re: [PATCH v2] tee: shm: fix slab page refcounting

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250822101522.wlwmdal5f4sqydjq@pengutronix.de>
Date: Fri, 22 Aug 2025 12:15:22 +0200
From: Marco Felsch <m.felsch@...gutronix.de>
To: Sumit Garg <sumit.garg@...nel.org>
Cc: Jens Wiklander <jens.wiklander@...aro.org>,
	Matthew Wilcox <willy@...radead.org>, vbabka@...e.cz,
	akpm@...ux-foundation.org, kernel@...gutronix.de,
	op-tee@...ts.trustedfirmware.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tee: shm: fix slab page refcounting

On 25-08-22, Sumit Garg wrote:
> On Thu, Aug 21, 2025 at 07:41:24PM +0200, Marco Felsch wrote:
> > Hi all,
> > 
> > is this issue fixed with 6.17? I ran:
> > 
> >   git log v6.14...v6.17-rc1 drivers/tee/tee_shm.c
> > 
> > and saw no changes.
> 
> Care to send a proper patch regarding what Matthew proposed in this
> thread?

I'm still not sure if the IOVs can be backed by other allocators too
because the OP-TEE API allows arbitrary sizes. Therefore my hope was
that one of the OP-TEE maintainers is taking care of this problem.

I also wonder why no one spotted/reported this issue too.

Regards,
  Marco


> 
> -Sumit
> 
> > 
> > Regards,
> >   Marco
> > 
> > On 25-04-28, Jens Wiklander wrote:
> > > On Thu, Mar 27, 2025 at 5:42 AM Sumit Garg <sumit.garg@...nel.org> wrote:
> > > >
> > > > On Wed, Mar 26, 2025 at 02:47:46PM +0100, Jens Wiklander wrote:
> > > > > On Wed, Mar 26, 2025 at 12:07 PM Marco Felsch <m.felsch@...gutronix.de> wrote:
> > > > > >
> > > > > > On 25-03-26, Matthew Wilcox wrote:
> > > > > > > On Tue, Mar 25, 2025 at 09:07:39PM +0100, Marco Felsch wrote:
> > > > > > > > Skip manipulating the refcount in case of slab pages according commit
> > > > > > > > b9c0e49abfca ("mm: decline to manipulate the refcount on a slab page").
> > > > > > >
> > > > > > > This almost certainly isn't right.  I know nothing about TEE, but that
> > > > > > > you are doing this indicates a problem.  The hack that we put into
> > > > > > > networking should not be blindly replicated.
> > > > > > >
> > > > > > > Why are you taking a reference on the pages to begin with?  Is it copy
> > > > > > > and pasted from somewhere else, or was there actual thought put into it?
> > > > > >
> > > > > > Not sure, this belongs to the TEE maintainers.
> > > > >
> > > > > I don't know. We were getting the user pages first, so I assume we
> > > > > just did the same thing when we added support for kernel pages.
> > > > >
> > > > > >
> > > > > > > If it's "prevent the caller from freeing the allocation", well, it never
> > > > > > > accomplished that with slab allocations.  So for callers that do kmalloc
> > > > > > > (eg setup_mm_hdr()  in drivers/firmware/efi/stmm/tee_stmm_efi.c), you
> > > > > > > have to rely on them not freeing the allocation while the TEE driver
> > > > > > > has it.
> > > >
> > > > It's not just about the TEE driver but rather if the TEE implementation
> > > > (a trusted OS) to whom the page is registered with. We don't want the
> > > > trusted OS to work on registered kernel pages if they gets free somehow
> > > > in the TEE client driver. Having a reference in the TEE subsystem
> > > > assured us that won't happen. But if you say slab allocations are still
> > > > prone the kernel pages getting freed even after refcount then can you
> > > > suggest how should we handle this better?
> > > >
> > > > As otherwise it can cause very hard to debug problems if trusted OS can
> > > > manipulate kernel pages that are no longer available.
> > > 
> > > We must be able to rely on the kernel callers to have the needed
> > > references before calling tee_shm_register_kernel_buf() and to keep
> > > those until after calling tee_shm_free().
> > > 
> > > 
> > > >
> > > > > > >
> > > > > > > And if that's your API contract, then there's no point in taking
> > > > > > > refcounts on other kinds of pages either; it's just unnecessary atomic
> > > > > > > instructions.  So the right patch might be something like this:
> > > > > > >
> > > > > > > +++ b/drivers/tee/tee_shm.c
> > > > > > > @@ -15,29 +15,11 @@
> > > > > > >  #include <linux/highmem.h>
> > > > > > >  #include "tee_private.h"
> > > > > >
> > > > > > I had the same diff but didn't went this way since we can't be sure that
> > > > > > iov's are always slab backed. As far as I understood IOVs. In
> > > > > > 'worst-case' scenario an iov can be backed by different page types too.
> > > > >
> > > > > We're only using kvec's. Briefly, before commit 7bdee4157591 ("tee:
> > > > > Use iov_iter to better support shared buffer registration") we checked
> > > > > with is_vmalloc_addr() || is_kmap_addr(). I like Matthew's suggestion,
> > > > > it's nice to fix problems by deleting code. :-)
> > > > >
> > > > > Sumit, you know the callers better. What do you think?
> > > >
> > > > If we don't have a sane way to refcont registered kernel pages in TEE
> > > > subsystem then yeah we have to solely rely on the client drivers to
> > > > behave properly. Nevertheless, it's still within the kernel boundaries
> > > > which we can rely upon.
> > > 
> > > Yes.
> > > 
> > > Cheers,
> > > Jens