linux-kernel - Re: [PATCH] mm/mmap_lock: Reset maple state on lock_vma_under

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJuCfpESKECudgqvm8CQ_whi761hWRPAhurR5efRVC4Hp2r8Qw@mail.gmail.com>
Date: Tue, 11 Nov 2025 16:10:42 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Shakeel Butt <shakeel.butt@...ux.dev>, 
	Jann Horn <jannh@...gle.com>, stable@...r.kernel.org, 
	syzbot+131f9eb2b5807573275c@...kaller.appspotmail.com
Subject: Re: [PATCH] mm/mmap_lock: Reset maple state on lock_vma_under_rcu() retry

On Tue, Nov 11, 2025 at 2:18 PM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> On 11/11/25 22:56, Liam R. Howlett wrote:
> > The retry in lock_vma_under_rcu() drops the rcu read lock before
> > reacquiring the lock and trying again.  This may cause a use-after-free
> > if the maple node the maple state was using was freed.

Ah, good catch. I didn't realize the state is RCU protected.

> >
> > The maple state is protected by the rcu read lock.  When the lock is
> > dropped, the state cannot be reused as it tracks pointers to objects
> > that may be freed during the time where the lock was not held.
> >
> > Any time the rcu read lock is dropped, the maple state must be
> > invalidated.  Resetting the address and state to MA_START is the safest
> > course of action, which will result in the next operation starting from
> > the top of the tree.
> >
> > Prior to commit 0b16f8bed19c ("mm: change vma_start_read() to drop RCU
> > lock on failure"), the rcu read lock was dropped and NULL was returned,
> > so the retry would not have happened.  However, now that the read lock
> > is dropped regardless of the return, we may use a freed maple tree node
> > cached in the maple state on retry.

Hmm. The above paragraph does not sound right to me, unless I
completely misunderstood it. Before 0b16f8bed19c we would keep RCU
lock up until the end of lock_vma_under_rcu(), so retries could still
happen but we were not dropping the RCU lock while doing that. After
0b16f8bed19c we drop RCU lock if vma_start_read() fails, so retrying
after such failure becomes unsafe. So, if you agree with me assessment
then I suggest changing it to:

Prior to commit 0b16f8bed19c ("mm: change vma_start_read() to drop RCU
lock on failure"), the retry after vma_start_read() failure was
happening under the same RCU lock. However, now that the read lock is
dropped on failure, we may use a freed maple tree node cached in the
maple state on retry.

> >
> > Cc: Suren Baghdasaryan <surenb@...gle.com>
> > Cc: stable@...r.kernel.org
> > Fixes: 0b16f8bed19c ("mm: change vma_start_read() to drop RCU lock on failure")
>
> The commit is 6.18-rc1 so we don't need Cc: stable, but it's a mm-hotfixes
> material that must go to Linus before 6.18.
>
> > Reported-by: syzbot+131f9eb2b5807573275c@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=131f9eb2b5807573275c
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
>
> Acked-by: Vlastimil Babka <vbabka@...e.cz>

With the changelog text sorted out.

Reviewed-by: Suren Baghdasaryan <surenb@...gle.com>

Thanks!

>
> > ---
> >  mm/mmap_lock.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > index 39f341caf32c0..f2532af6208c0 100644
> > --- a/mm/mmap_lock.c
> > +++ b/mm/mmap_lock.c
> > @@ -257,6 +257,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >               if (PTR_ERR(vma) == -EAGAIN) {
> >                       count_vm_vma_lock_event(VMA_LOCK_MISS);
> >                       /* The area was replaced with another one */
> > +                     mas_set(&mas, address);
> >                       goto retry;
> >               }
> >
>