[PATCH] pid: fix cad_pid use-after-free race in proc_do_cad_pid()

[PATCH] pid: fix cad_pid use-after-free race in proc_do_cad_pid()

[Cen Zhang (Microsoft)]

proc_do_cad_pid() reads the global cad_pid pointer and passes it to
pid_vnr() without protecting the lifetime of the referenced struct pid.
A concurrent writer can replace cad_pid and drop the final reference to
the old struct pid after the reader has loaded the pointer but before
pid_vnr() has finished dereferencing it, causing a use-after-free.

Fix this by wrapping the read side in an RCU read-side critical section
and waiting for a grace period before dropping the old cad_pid reference
on the write side. Do not use call_rcu(&old_pid->rcu, ...) here:
struct pid::rcu is already used by free_pid(), so queueing it again can
corrupt the RCU callback list.

KASAN crash stack:
  kernel/pid.c:545 pid_nr_ns()          # reads freed pid->level
  kernel/pid.c:556 pid_vnr()
  kernel/pid.c:775 proc_do_cad_pid()
  fs/proc/proc_sysctl.c proc_sys_call_handler()
  fs/read_write.c vfs_read()
  fs/read_write.c __x64_sys_pread64()

Fixes: 9ec52099e4b8 ("[PATCH] replace cad_pid by a struct pid")
Reported-by: AutonomousCodeSecurity@microsoft.com
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
 kernel/pid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index f55189a3d07d..fee62b2c0c7b 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -768,11 +768,15 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
 		size_t *lenp, loff_t *ppos)
 {
 	struct pid *new_pid;
+	struct pid *old_pid;
 	pid_t tmp_pid;
 	int r;
 	struct ctl_table tmp_table = *table;
 
+	rcu_read_lock();
 	tmp_pid = pid_vnr(cad_pid);
+	rcu_read_unlock();
+
 	tmp_table.data = &tmp_pid;
 
 	r = proc_dointvec(&tmp_table, write, buffer, lenp, ppos);
@@ -783,7 +787,9 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
 	if (!new_pid)
 		return -ESRCH;
 
-	put_pid(xchg(&cad_pid, new_pid));
+	old_pid = xchg(&cad_pid, new_pid);
+	synchronize_rcu();
+	put_pid(old_pid);
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH] pid: fix cad_pid use-after-free race in proc_do_cad_pid()

[Mateusz Guzik]

On Fri, Jul 17, 2026 at 11:01 PM Cen Zhang (Microsoft)
<blbllhy@gmail.com> wrote:
>
> proc_do_cad_pid() reads the global cad_pid pointer and passes it to
> pid_vnr() without protecting the lifetime of the referenced struct pid.
> A concurrent writer can replace cad_pid and drop the final reference to
> the old struct pid after the reader has loaded the pointer but before
> pid_vnr() has finished dereferencing it, causing a use-after-free.
>
> Fix this by wrapping the read side in an RCU read-side critical section
> and waiting for a grace period before dropping the old cad_pid reference
> on the write side. Do not use call_rcu(&old_pid->rcu, ...) here:
> struct pid::rcu is already used by free_pid(), so queueing it again can
> corrupt the RCU callback list.
>
> KASAN crash stack:
>   kernel/pid.c:545 pid_nr_ns()          # reads freed pid->level
>   kernel/pid.c:556 pid_vnr()
>   kernel/pid.c:775 proc_do_cad_pid()
>   fs/proc/proc_sysctl.c proc_sys_call_handler()
>   fs/read_write.c vfs_read()
>   fs/read_write.c __x64_sys_pread64()
>
> Fixes: 9ec52099e4b8 ("[PATCH] replace cad_pid by a struct pid")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
>  kernel/pid.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f55189a3d07d..fee62b2c0c7b 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -768,11 +768,15 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
>                 size_t *lenp, loff_t *ppos)
>  {
>         struct pid *new_pid;
> +       struct pid *old_pid;
>         pid_t tmp_pid;
>         int r;
>         struct ctl_table tmp_table = *table;
>
> +       rcu_read_lock();
>         tmp_pid = pid_vnr(cad_pid);
> +       rcu_read_unlock();
> +
>         tmp_table.data = &tmp_pid;
>
>         r = proc_dointvec(&tmp_table, write, buffer, lenp, ppos);
> @@ -783,7 +787,9 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
>         if (!new_pid)
>                 return -ESRCH;
>
> -       put_pid(xchg(&cad_pid, new_pid));
> +       old_pid = xchg(&cad_pid, new_pid);
> +       synchronize_rcu();
> +       put_pid(old_pid);
>         return 0;
>  }
>

I don't understand what this call to synchronize_rcu() is accomplishing.

sashiko points out the actual use of cad_pid in kill_cad_pid() suffers
the same liveness issue, i.e., the routine needs to enter rcu and grab
a ref on the found obj before issuing kill_pid(). I would deinline it
while patching the problem.

Re: [PATCH] pid: fix cad_pid use-after-free race in proc_do_cad_pid()

[Bradley Morgan]

Hi Cen,

Applies to master, context matches. The race is real..

[re: what the synchronize_rcu() is accomplishing]

put_pid() frees synchronously via kmem_cache_free(), and pid_cachep is
not SLAB_TYPESAFE_BY_RCU. So the writer has to wait out RCU readers
before the free, and synchronize_rcu() is that wait. Its standing in for
the call_rcu() we cant use, free_pid() owns pid->rcu. Correct,
just not like very obvious. please put a one line comment above it
saying exactly that.


> +	rcu_read_lock();
> 	tmp_pid = pid_vnr(cad_pid);
> +	rcu_read_unlock();

plain load. Use rcu_dereference() and annotate cad_pid __rcu, otherwise
sparse cant see any of this.

> -	put_pid(xchg(&cad_pid, new_pid));
> +	old_pid = xchg(&cad_pid, new_pid);
> +	synchronize_rcu();
> +	put_pid(old_pid);

ok, fine. xchg() is fully ordered, one writer per old_pid, no double put.

but this fixes one of two readers. kill_cad_pid() in sched/signal.h has
the same unprotected load and is reachable from ctrl_alt_del(), thats
hardirq context, so your grace period does nothing for it. Deinline it
into kernel/pid.c and do e.g:

	rcu_read_lock();
	pid = get_pid(rcu_dereference(cad_pid));
	rcu_read_unlock();
	ret = kill_pid(pid, sig, priv);
	put_pid(pid);

Please make it a two patch series, deinline with no functional change, then
fix both readers.

Nits:

The Fixes: tag LGTM, before 9ec52099e4b8 there was no struct to free. If
you want CC stable, add a line in
the changelog about impact: iirc it is root only (the sysctl is
0600) and only when cad_pid holds the last ref of an exited task. Subject
prefix should be
"pid:". Keep the vN changelog below the ---.

with those addressed, please add:

Reviewed-by: Bradley Morgan <include@grrlz.net>
Thanks!