pid: fix cad_pid use-after-free race in proc_do_cad_pid()
PATCH 1/1 pid: fix cad_pid use-after-free race in proc_do_cad_pid()
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()
--- kernel/pid.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
kernel/pid.c +7 -1
@@ -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 *buffeif (!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 ; }
Signature · 1 line
2.53.0
Quoted 49 lines · Cen Zhang (Microsoft)
proc_do_cad_pid()reads the globalcad_pidpointer and passes it topid_vnr()without protecting the lifetime of the referenced struct pid. A concurrent writer can replacecad_pidand drop the final reference to the old struct pid after the reader has loaded the pointer but beforepid_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_pidreference on the write side. Do not usecall_rcu(&old_pid->rcu, ...)here:
structpid::rcuis already used byfree_pid(), so queueing it again can corrupt the RCU callback list.KASAN crash stack:
kernel/pid.c:545pid_nr_ns()# reads freedpid->level
kernel/pid.c:556pid_vnr()
kernel/pid.c:775proc_do_cad_pid()
fs/proc/proc_sysctl.cproc_sys_call_handler()
fs/read_write.cvfs_read()
fs/read_write.c__x64_sys_pread64()Fixes 9ec52099e4b8 ("[PATCH] replace cad_pid by a struct pid")Reported-by [email protected]Signed-off-by Cen Zhang (Microsoft) <[email protected]>--- kernel/pid.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)kernel/pid.c +4
-0@@ -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);
kernel/pid.c static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe · line 795
Quoted 10 lines
kernel/pid.c +3
-1@@ -783,7 +787,9 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffeif (!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.
Bradley Morgan
Reviewed-byHi 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.
kernel/pid.c static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe · line 778
Quoted 3 lines
kernel/pid.c +1
-0+rcu_read_lock ();tmp_pid = pid_vnr(cad_pid);kernel/pid.c +1
-0+rcu_read_unlock ();
plain load. Use rcu_dereference() and annotate cad_pid __rcu, otherwise sparse cant see any of this.
kernel/pid.c static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe · line 792
Quoted 4 lines
kernel/pid.c +3
-1-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:
Thanks!
Thanks both for the review.
One impact clarification: this is not init-namespace-root-only. The sysctl is mode 0600, but access is checked against the owning user namespace, so an unprivileged user can reach it via userns + pidns + a proc mount.
The demonstrated impact is a KASAN UAF. On non-debug builds the proc_do_cad_pid() read path likely only returns a stale pid value unless the freed pid object is reused in a very tight window.
I'll send a v2 series as suggested: patch 1 deinlines kill_cad_pid() without functional changes, and patch 2 contains the updated fix.