sched/cache: Fix use-after-free of the mm replaced by exec

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

---
  fs/exec.c             |  1 +
  include/linux/sched.h |  4 ++++
  kernel/events/core.c  |  2 ++
  kernel/sched/fair.c   | 16 ++++++++++++++++
  4 files changed, 23 insertions(+)

fs/exec.c +1 -0

index 745f6eb5279e6..6194c38807980 100644
@@ -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);
  }
 

include/linux/sched.h +4 -0

index 8b3d47a325cca..6ae31bffe049e 100644
@@ -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

kernel/events/core.c +0 -0

index a6c8e38a31104..2f29cbccf03f1 100644

Hi Hyunwoo,

Quoted 39 lines · Hyunwoo Kim

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()

Quoted 9 lines

     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.

Quoted 36 lines

---
  fs/exec.c             |  1 +
  include/linux/sched.h |  4 ++++
  kernel/events/core.c  |  2 ++
  kernel/sched/fair.c   | 16 ++++++++++++++++
  4 files changed, 23 insertions(+)

fs/exec.c +1 -0

index 745f6eb5279e6..6194c38807980 100644
@@ -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);
  }

include/linux/sched.h +4 -0

index 8b3d47a325cca..6ae31bffe049e 100644
@@ -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

kernel/events/core.c +0 -1

index a6c8e38a31104..2f29cbccf03f1 100644
--- a/kernel/events/core.c

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache, · line 5430

Quoted 6 lines

+++ b/kernel/events/core.c

kernel/events/core.c +2 -0

@@ -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?

Quoted 12 lines

kernel/events/core.c +11 -0

+/* 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.

kernel/events/core.c

Quoted 2 lines

kernel/events/core.c +2 -0

+	 */
+	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?

Quoted 90 lines · Yu C

Hi Hyunwoo,

Quoted 39 lines · Hyunwoo Kim

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()

Quoted 9 lines

     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.

Quoted 36 lines

---
  fs/exec.c             |  1 +
  include/linux/sched.h |  4 ++++
  kernel/events/core.c  |  2 ++
  kernel/sched/fair.c   | 16 ++++++++++++++++
  4 files changed, 23 insertions(+)

fs/exec.c +1 -0

index 745f6eb5279e6..6194c38807980 100644
@@ -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);
  }

include/linux/sched.h +4 -0

index 8b3d47a325cca..6ae31bffe049e 100644
@@ -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

kernel/events/core.c +0 -1

index a6c8e38a31104..2f29cbccf03f1 100644
--- a/kernel/events/core.c

Quoted 8 lines

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache, · line 5430

Quoted 6 lines

+++ b/kernel/events/core.c

kernel/events/core.c +2 -0

@@ -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.

Quoted 20 lines

Quoted 4 lines

kernel/events/core.c +3 -0

+/* exec() has switched to the new mm and is about to drop the old one. */
+void sched_cache_exec_done(void)
+{

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache,

Quoted 10 lines

kernel/events/core.c +10 -0

+	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?

Quoted 100 lines · Hyunwoo Kim

Quoted 90 lines · Yu C

Hi Hyunwoo,

Quoted 39 lines · Hyunwoo Kim

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()

Quoted 9 lines

     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.

Quoted 36 lines

---
  fs/exec.c             |  1 +
  include/linux/sched.h |  4 ++++
  kernel/events/core.c  |  2 ++
  kernel/sched/fair.c   | 16 ++++++++++++++++
  4 files changed, 23 insertions(+)

fs/exec.c +1 -0

index 745f6eb5279e6..6194c38807980 100644
@@ -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);
  }

include/linux/sched.h +4 -0

index 8b3d47a325cca..6ae31bffe049e 100644
@@ -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

kernel/events/core.c +0 -1

index a6c8e38a31104..2f29cbccf03f1 100644
--- a/kernel/events/core.c

Quoted 8 lines

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache, · line 5430

Quoted 6 lines

+++ b/kernel/events/core.c

kernel/events/core.c +2 -0

@@ -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?

Quoted 22 lines

Duh.. that one is unrelated. My mistake.

Quoted 20 lines

Quoted 4 lines

kernel/events/core.c +3 -0

+/* exec() has switched to the new mm and is about to drop the old one. */
+void sched_cache_exec_done(void)
+{

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache,

Quoted 10 lines

kernel/events/core.c +10 -0

+	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.

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/[email protected]/

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.

Quoted 124 lines · Tim Chen

Quoted 122 lines · Hyunwoo Kim

Quoted 90 lines · Yu C

Hi Hyunwoo,

Quoted 39 lines · Hyunwoo Kim

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()

Quoted 9 lines

     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.

Quoted 36 lines

---
  fs/exec.c             |  1 +
  include/linux/sched.h |  4 ++++
  kernel/events/core.c  |  2 ++
  kernel/sched/fair.c   | 16 ++++++++++++++++
  4 files changed, 23 insertions(+)

fs/exec.c +1 -0

index 745f6eb5279e6..6194c38807980 100644
@@ -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);
  }

include/linux/sched.h +4 -0

index 8b3d47a325cca..6ae31bffe049e 100644
@@ -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

kernel/events/core.c +0 -1

index a6c8e38a31104..2f29cbccf03f1 100644
--- a/kernel/events/core.c

Quoted 8 lines

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache, · line 5430

Quoted 6 lines

+++ b/kernel/events/core.c

kernel/events/core.c +2 -0

@@ -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.

Quoted 20 lines

Quoted 4 lines

kernel/events/core.c +3 -0

+/* exec() has switched to the new mm and is about to drop the old one. */
+void sched_cache_exec_done(void)
+{

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache,

Quoted 10 lines

kernel/events/core.c +10 -0

+	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?

Quoted 1 line

Thanks to Kyunwoo to show this potential use-after-free problem.

It is a real, triggerable issue.

Quoted 19 lines

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/[email protected]/

Quoted 3 lines

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.

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

Best regards,
Hyunwoo Kim

Quoted 5 lines

Quoted 3 lines

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.

Quoted 125 lines · Hyunwoo Kim

Quoted 124 lines · Tim Chen

Quoted 122 lines · Hyunwoo Kim

Quoted 90 lines · Yu C

Hi Hyunwoo,

Quoted 39 lines · Hyunwoo Kim

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()

Quoted 9 lines

     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.

Quoted 36 lines

---
  fs/exec.c             |  1 +
  include/linux/sched.h |  4 ++++
  kernel/events/core.c  |  2 ++
  kernel/sched/fair.c   | 16 ++++++++++++++++
  4 files changed, 23 insertions(+)

fs/exec.c +1 -0

index 745f6eb5279e6..6194c38807980 100644
@@ -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);
  }

include/linux/sched.h +4 -0

index 8b3d47a325cca..6ae31bffe049e 100644
@@ -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

kernel/events/core.c +0 -1

index a6c8e38a31104..2f29cbccf03f1 100644
--- a/kernel/events/core.c

Quoted 8 lines

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache, · line 5430

Quoted 6 lines

+++ b/kernel/events/core.c

kernel/events/core.c +2 -0

@@ -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.

Quoted 20 lines

Quoted 4 lines

kernel/events/core.c +3 -0

+/* exec() has switched to the new mm and is about to drop the old one. */
+void sched_cache_exec_done(void)
+{

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache,

Quoted 10 lines

kernel/events/core.c +10 -0

+	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?

Quoted 4 lines

Quoted 1 line

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/[email protected]/
fixes this issue.

Quoted 19 lines

Quoted 19 lines

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/[email protected]/

Quoted 5 lines

Quoted 3 lines

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?

Quoted 3 lines

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.

Quoted 168 lines · Tim Chen

Quoted 125 lines · Hyunwoo Kim

Quoted 124 lines · Tim Chen

Quoted 122 lines · Hyunwoo Kim

Quoted 90 lines · Yu C

Hi Hyunwoo,

Quoted 39 lines · Hyunwoo Kim

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()

Quoted 9 lines

     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.

Quoted 36 lines

---
  fs/exec.c             |  1 +
  include/linux/sched.h |  4 ++++
  kernel/events/core.c  |  2 ++
  kernel/sched/fair.c   | 16 ++++++++++++++++
  4 files changed, 23 insertions(+)

fs/exec.c +1 -0

index 745f6eb5279e6..6194c38807980 100644
@@ -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);
  }

include/linux/sched.h +4 -0

index 8b3d47a325cca..6ae31bffe049e 100644
@@ -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

kernel/events/core.c +0 -1

index a6c8e38a31104..2f29cbccf03f1 100644
--- a/kernel/events/core.c

Quoted 8 lines

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache, · line 5430

Quoted 6 lines

+++ b/kernel/events/core.c

kernel/events/core.c +2 -0

@@ -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.

Quoted 20 lines

Quoted 4 lines

kernel/events/core.c +3 -0

+/* exec() has switched to the new mm and is about to drop the old one. */
+void sched_cache_exec_done(void)
+{

kernel/events/core.c attach_task_ctx_data(struct task_struct *task, struct kmem_cache *ctx_cache,

Quoted 10 lines

kernel/events/core.c +10 -0

+	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?

Quoted 4 lines

Quoted 1 line

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/[email protected]/
fixes this issue.

Quoted 19 lines

Quoted 19 lines

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/[email protected]/

Quoted 5 lines

Quoted 3 lines

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?

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

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.

I confirmed that patches 1 and 2 of your series prevent this UAF.

Best regards,
Hyunwoo Kim