Re: [PATCH] sched/cache: Fix use-after-free of the mm replaced by exec

[Tim Chen]

On Tue, 2026-09-01 at 06:44 +0900, Hyunwoo Kim wrote:
> On Mon, Aug 31, 2026 at 01:25:06PM -0700, Tim Chen wrote:
> > On Tue, 2026-09-01 at 04:28 +0900, Hyunwoo Kim wrote:
> > > On Mon, Aug 31, 2026 at 12:22:45PM +0800, Chen, Yu C wrote:
> > > > Hi Hyunwoo,
> > > > 
> > > > On 8/30/2026 3:30 PM, Hyunwoo Kim wrote:
> > > > > When the waker cannot use the wakelist, ttwu_queue() takes the target rq
> > > > > lock and goes down into update_curr(). If the target rq belongs to another
> > > > > CPU, the task handed to account_mm_sched() is the one running on that CPU,
> > > > > not the task being woken.
> > > > > 
> > > > > account_mm_sched() reads p->mm and updates mm->sc_stat. Nothing keeps that
> > > > > mm alive. The rq lock and rq->cpu_epoch_lock it holds have nothing to do
> > > > > with the lifetime of the mm.
> > > > > 
> > > > > If that task happens to be in execve(), exec_mmap() points tsk->mm and
> > > > > tsk->active_mm at the new mm, and exec_mm_put_old() from setup_new_exec()
> > > > > drops the old one. free_bprm() does the same when exec fails. On the way
> > > > > from mmput() down to __mmdrop(), mm_destroy_sched() calls free_percpu() on
> > > > > sc_stat.pcpu_sched and free_mm() returns the mm_struct.
> > > > > 
> > > > > Whoever already read the old pointer keeps using it. It adds to runtime in
> > > > > the freed per-cpu area and reads sc_stat in the freed mm_struct. Depending
> > > > > on the condition it also writes sc_stat.cpu. That is a use-after-free.
> > > > > 
> > > > > Commit 9f23469401b0 ("sched/cache: Fix potential NULL mm pointer access")
> > > > > changed the remaining p->mm dereference to the local variable, and said the
> > > > > active_mm reference keeps the structure allocated. That holds for the other
> > > > > paths that detach an mm, since they take an mmgrab_lazy_tlb() reference.
> > > > > exec reassigns active_mm to the new mm as well, so that reference is gone.
> > > > > What is left is the mm_users reference in bprm->old_mm, and dropping it is
> > > > > the free.
> > > > > 
> > > > >               CPU0                                CPU1
> > > > > 
> > > > >                                        write(pipe)
> > > > >                                        try_to_wake_up()
> > > > >                                          ttwu_queue()      // takes rq0 lock
> > > > >                                            enqueue_task_fair()
> > > > >                                              update_curr()
> > > > >                                                update_se()
> > > > >                                                  account_mm_sched()
> > > > >                                                    mm = rq0->curr->mm
> > > > >                                                          // old mm
> > > > >    execve()
> > > > >      exec_mmap()                       // tsk->mm = new mm
> > > > >      setup_new_exec()
> > > > >        exec_mm_put_old()
> > > > >          mmput() -> ... -> __mmdrop()
> > > > >            mm_destroy_sched()          // free_percpu()
> > > > >            free_mm()
> > > > >                                                    read mm->sc_stat.epoch
> > > > >                                                          // use-after-free
> > > > > 
> > > > 
> > > > Ah, thanks for catching this.
> > > > 
> > > > > ---
> > > > >   fs/exec.c             |  1 +
> > > > >   include/linux/sched.h |  4 ++++
> > > > >   kernel/events/core.c  |  2 ++
> > > > >   kernel/sched/fair.c   | 16 ++++++++++++++++
> > > > >   4 files changed, 23 insertions(+)
> > > > > 
> > > > > diff --git a/fs/exec.c b/fs/exec.c
> > > > > index 745f6eb5279e6..6194c38807980 100644
> > > > > --- a/fs/exec.c
> > > > > +++ b/fs/exec.c
> > > > > @@ -916,6 +916,7 @@ static void exec_mm_put_old(struct mm_struct *old_mm)
> > > > >   {
> > > > >   	setmax_mm_hiwater_rss(&current->signal->maxrss, old_mm);
> > > > >   	mm_update_next_owner(old_mm);
> > > > > +	sched_cache_exec_done();
> > > > >   	mmput(old_mm);
> > > > >   }
> > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > > index 8b3d47a325cca..6ae31bffe049e 100644
> > > > > --- a/include/linux/sched.h
> > > > > +++ b/include/linux/sched.h
> > > > > @@ -2415,10 +2415,14 @@ struct sched_cache_stat {
> > > > >   	int cpu;
> > > > >   } ____cacheline_aligned_in_smp;
> > > > > +void sched_cache_exec_done(void);
> > > > > +
> > > > >   #else
> > > > >   struct sched_cache_stat { };
> > > > > +static inline void sched_cache_exec_done(void) { }
> > > > > +
> > > > >   #endif
> > > > >   #ifndef MODULE
> > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > > > index a6c8e38a31104..2f29cbccf03f1 100644
> > > > > --- a/kernel/events/core.c
> > > > > +++ b/kernel/events/core.c
> > > > > @@ -5427,6 +5427,8 @@ attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache,
> > > > >   	if (!cd)
> > > > >   		return -ENOMEM;
> > > > > +	/* @old, loaded by the try_cmpxchg() below, is only stable under RCU. */
> > > > > +	guard(rcu)();
> > > > 
> > > > Is this change related to this UAF issue?
> > > 
> > > Duh.. that one is unrelated. My mistake.
> > > 
> > > > 
> > > > > +/* exec() has switched to the new mm and is about to drop the old one. */
> > > > > +void sched_cache_exec_done(void)
> > > > > +{
> > > > > +	struct rq_flags rf;
> > > > > +	struct rq *rq;
> > > > > +
> > > > > +	/*
> > > > > +	 * account_mm_sched() dereferences rq->curr->mm under this rq's lock,
> > > > > +	 * so a remote CPU can still be using the old mm. The lock cycle waits
> > > > > +	 * for it, and the store to tsk->mm cannot be reordered past the
> > > > > +	 * release, so later acquirers see the new mm.
> > > > > +	 */
> > > > > +	rq = this_rq_lock_irq(&rf);
> > > > 
> > > > A smart fix, learnt! It behaves like a synchronize_rcu() to protect against
> > > > the read in account_mm_sched(). Small open: since the context of invoking
> > > > account_mm_sched() is preemption-disabled, I wonder if we can simply use
> > > > synchronize_rcu() directly instead of this_rq_lock_irq() - just to avoid
> > > > contention for rq-lock in heavy system?
> > 
> > Thanks to Kyunwoo to show this potential use-after-free problem.
> 
> It is a real, triggerable issue.
> 

Appreciate if you can check whether the first 2 patches in 
https://lore.kernel.org/lkml/cover.1787955777.git.tim.c.chen@xxxxxxxxxxxxxxx/
fixes this issue.

> > 
> > While synchronize_rcu() avoids rq lock, it could introduce a long delay
> > of a full grace period that will be undesirable.
> > 
> > 
> > I think a better fix is to decouple sc_stat from mm and make it its own structure.
> > Then we can do proper refcounting and rcu management on sc_stat itself.
> > We will let task points to sc_stat with proper ref count and change the access
> > as task->sc_stat directly nstead of via task->mm->sc_stat.
> > 
> > Then we will know sc_stat exists in account_mm_sched() execution
> > as long as a task points to it and do not have to worry that sc_stat is
> > released due to the mm life cycle.
> > 
> > In the first 2 patches of the prctl control series for cache aware scheduling
> > we posted, we introduced the change I described above and should 
> > fix this issue without additional rq locking or synchronize_rcu() overhead.
> >  https://lore.kernel.org/lkml/cover.1787955777.git.tim.c.chen@xxxxxxxxxxxxxxx/
> > 
> > We should probably prioritize to merge the first couple of patches from
> > that series now in light of this issue.  It will make maintenance and future
> > modification of the cache aware scheduling code easier.
> 
> That is a fairly large change. 

However adding rcu synchronization will slow down the launch of a new process
too much.  The rq lock solution has less latency in my opinion.  But I
think decoupling sc_stat is the best solution without adding latency in the
exec path.

Wonder what is Peter's opinion?

> 
> If you add a Reported-by: tag for this UAF to the patch, I am fine with 
> handling it that way.

I'll be happy to do that. If you let us know if those two patches from prctl series fixes
the bug you found in KASAN log, I can also add a Tested-by tag in addition to Reported-by.

Thanks.

Tim

> 
> 
> Best regards,
> Hyunwoo Kim