Re: [BUG] hard-to-hit mm_struct UAF due to insufficiently careful vma_refcount_put() wrt SLAB_TYPESAFE_BY_RCU

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Jul 24, 2025 at 04:50:49PM +0200, Jann Horn wrote:
> On Thu, Jul 24, 2025 at 7:13 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> > On Wed, Jul 23, 2025 at 09:43:35PM +0200, Jann Horn wrote:
> > > Sorry, while typing up this mail I realized I didn't have this stuff
> > > particularly straight in my head myself when writing my previous mails
> > > about this...
> > >
> > > On Wed, Jul 23, 2025 at 8:45 PM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > > On Wed, Jul 23, 2025 at 08:30:30PM +0200, Jann Horn wrote:
> > > > > On Wed, Jul 23, 2025 at 8:14 PM Lorenzo Stoakes
> > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > > On Wed, Jul 23, 2025 at 06:26:53PM +0200, Jann Horn wrote:
> > > > > > > There's a racy UAF in `vma_refcount_put()` when called on the
> > > > > > > `lock_vma_under_rcu()` path because `SLAB_TYPESAFE_BY_RCU` is used
> > > > > > > without sufficient protection against concurrent object reuse:
> > > > > > >
> > > > > > > lock_vma_under_rcu() looks up a VMA locklessly with mas_walk() under
> > > > > > > rcu_read_lock(). At that point, the VMA may be concurrently freed, and
> > > > > > > it can be recycled by another process. vma_start_read() then
> > > > > > > increments the vma->vm_refcnt (if it is in an acceptable range), and
> > > > > > > if this succeeds, vma_start_read() can return a reycled VMA. (As a
> > > > > > > sidenote, this goes against what the surrounding comments above
> > > > > > > vma_start_read() and in lock_vma_under_rcu() say - it would probably
> > > > > > > be cleaner to perform the vma->vm_mm check inside vma_start_read().)
> > > > > > >
> > > > > > > In this scenario where the VMA has been recycled, lock_vma_under_rcu()
> > > > > > > will then detect the mismatching ->vm_mm pointer and drop the VMA
> > > > > > > through vma_end_read(), which calls vma_refcount_put().
> > > > > >
> > > > > > So in _correctly_ identifying the recycling, we then hit a problem. Fun!
> > > > > >
> > > > > > > vma_refcount_put() does this:
> > > > > > >
> > > > > > > ```
> > > > > > > static inline void vma_refcount_put(struct vm_area_struct *vma)
> > > > > > > {
> > > > > > >         /* Use a copy of vm_mm in case vma is freed after we drop vm_refcnt */
> > > > > > >         struct mm_struct *mm = vma->vm_mm;
> > > > > >
> > > > > > Are we at a point where we _should_ be looking at a VMA with vma->vm_mm ==
> > > > > > current->mm here?
> > > > >
> > > > > Well, you _hope_ to be looking at a VMA with vma->vm_mm==current->mm,
> > > > > but if you lose a race it is intentional that you can end up with
> > > > > another MM's VMA here.
> >
> > Right I get the SLAB_TYPESAFE_BY_RCU thing, what I'm saying overall is 'can we
> > detect that we lost the race by knowing what mm this should be'...
> >
> > >
> > > (I forgot: The mm passed to lock_vma_under_rcu() is potentially
> > > different from current->mm if we're coming from uffd_mfill_lock(),
> > > which would be intentional and desired, but that's not relevant here.
> > > Sorry for making things more confusing.)
> >
> > ...and because of this, no we can't. I hate how uffd is implemented.
>
> I mean, we are in a context where we're looking up a VMA under a
> specific MM, we know which MM the VMA should be from. And we have a
> bailout that checks for this. It's just that by the time we can check
> if the MM matches the expected one, we've already grabbed the VMA.

OK.

>
> > > > Right so, we have:
> > > >
> > > > 'mm we meant to get' (which apparently can't be assumed to be current->mm)
> > > > 'mm we actually got' (which may or may not be freed at any time)
> > > >
> > > > The _meant to get_ one might have eternal waiters. Or might not even need
> > > > to be woken up.
> > > >
> > > > I don't see why keeping the 'actually got' one around really helps us? Am I
> > > > missing something?
> > >
> > > We basically have taken a read lock on a VMA that is part of the
> > > "actually got" MM, and so we may have caused writers from that MM to
> > > block and sleep, and since we did that we have to wake them back up
> > > and say "sorry, locked the wrong object, please continue".
> >
> > OK I think this is the crux of it then, and what I've been missing here -
> > we have taken a read lock _by mistake_ in effect on the recycled mm, which
> > may end up to be a spurious one that we need to immediately drop, but
> > because of this we might have waiters that could wait forever.
> >
> > OK I get it. But to safely reference the mm here we need to be assured it
> > stays around because in case of this not being true, we have nothing to
> > prevent that mm going away right?
>
> Yes - as Suren explained, as long as we hold a reference to the VMA,
> the MM also stays around, but to access the MM after dropping the VMA
> we need to somehow grab a reference on the MM first.

OK.