pid: deinline kill_cad_pid
Subsystems (2)
PATCH v3 1/2 pid: deinline kill_cad_pid
Move kill_cad_pid() out of the header without changing behavior. This prepares for taking a reference to cad_pid under RCU in the following fix.
---
v3: Add Reviewed-by from Bradley Morgan.
v2: New preparatory patch to deinline kill_cad_pid().
include/linux/sched/signal.h | 5 +---- kernel/pid.c | 5 +++++ 2 files changed, 6 insertions(+), 4 deletions(-)
include/linux/sched/signal.h +1 -4
@@ -562,10 +562,7 @@ static inline sigset_t *sigmask_to_save(void)return res; } -static inline int kill_cad_pid (int sig,int priv) -{ -return kill_pid (cad_pid, sig, priv); -} +int kill_cad_pid (int sig,int priv);/* These can be the second arg to send_sig_info/send_group_sig_info. */ #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
kernel/pid.c +5 -0
@@ -557,6 +557,11 @@ pid_t pid_vnr(struct pid *pid) }EXPORT_SYMBOL_GPL (pid_vnr); +int kill_cad_pid (int sig,int priv) +{ +return kill_pid (cad_pid, sig, priv); +} +pid_t __task_pid_nr_ns (struct task_struct *task,enum pid_type type,struct pid_namespace *ns) {
Signature · 1 line
2.53.0
PATCH v3 2/2 pid: fix cad_pid use-after-free race
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.
The sysctl is mode 0600, but access is checked against the owning user namespace, so an unprivileged user can reach it via userns, pidns, and a proc mount.
Fix this by treating cad_pid as an RCU-protected pointer at both read sites and by waiting for a grace period before dropping the old reference on the write side.
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()
---
v3:
- Keep kill_cad_pid() inside the RCU read-side critical section instead of taking a pid reference, as suggested by Oleg.
v2:
- Split out kill_cad_pid() deinline into a preparatory patch.
- Annotate cad_pid as __rcu and use rcu_dereference().
- Protect kill_cad_pid() by taking a pid reference under RCU.
- Add a comment explaining why synchronize_rcu() is used instead of
call_rcu().
include/linux/sched.h | 2 +- init/main.c | 2 +- kernel/pid.c | 22 +++++++++++++++++++--- kernel/reboot.c | 2 +- 4 files changed, 22 insertions(+), 6 deletions(-)
include/linux/sched.h +1 -1
@@ -1767,7 +1767,7 @@ static inline bool is_lazy_mmu_mode_active(void) }#endif -extern struct pid *cad_pid; +extern struct pid __rcu *cad_pid;/* * Per process flags
init/main.c +1 -1
@@ -1636,7 +1636,7 @@ static noinline void __init kernel_init_freeable(void)*/ set_mems_allowed (node_states[N_MEMORY ]); - cad_pid =get_pid (task_pid (current)); +rcu_assign_pointer (cad_pid,get_pid (task_pid (current)));smp_prepare_cpus (setup_max_cpus);
kernel/pid.c +19 -3
@@ -559,7 +559,13 @@ EXPORT_SYMBOL_GPL(pid_vnr);int kill_cad_pid (int sig,int priv) { -return kill_pid (cad_pid, sig, priv); +int ret; + +rcu_read_lock (); + ret =kill_pid (rcu_dereference (cad_pid), sig, priv); +rcu_read_unlock (); + +return ret; } pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
@@ -773,11 +779,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; - tmp_pid =pid_vnr (cad_pid); +rcu_read_lock (); + tmp_pid =pid_vnr (rcu_dereference (cad_pid)); +rcu_read_unlock (); + tmp_table.data = &tmp_pid; r =proc_dointvec (&tmp_table, write, buffer, lenp, ppos);
@@ -788,7 +798,13 @@ 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 =unrcu_pointer (xchg (&cad_pid,RCU_INITIALIZER (new_pid))); +/* +* Wait for cad_pid readers before put_pid(). We cannot use +* call_rcu() here because free_pid() already owns pid->rcu. +*/ +synchronize_rcu (); +put_pid (old_pid);return 0 ; }
kernel/reboot.c +1 -1
@@ -24,7 +24,7 @@*/ static int C_A_D =1 ; -struct pid *cad_pid; +struct pid __rcu *cad_pid;EXPORT_SYMBOL (cad_pid);#if defined(CONFIG_ARM )
Signature · 1 line
2.53.0
Re: [PATCH v3 2/2] pid: fix cad_pid use-after-free race
Quoted 138 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.The sysctl is mode 0600, but access is checked against the owning user namespace, so an unprivileged user can reach it via userns, pidns, and a proc mount.
Fix this by treating
cad_pidas an RCU-protected pointer at both read sites and by waiting for a grace period before dropping the old reference on the write side.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]Suggested-by Mateusz Guzik <[email protected]>Suggested-by Oleg Nesterov <[email protected]>Reviewed-by Bradley Morgan <[email protected]>Signed-off-by Cen Zhang (Microsoft) <[email protected]>---
v3:
- Keepkill_cad_pid()inside the RCU read-side critical section instead of taking a pid reference, as suggested by Oleg.v2:
- Split outkill_cad_pid()deinline into a preparatory patch.
- Annotatecad_pidas__rcuand usercu_dereference().
- Protectkill_cad_pid()by taking a pid reference under RCU.
- Add a comment explaining whysynchronize_rcu()is used instead ofcall_rcu().include/linux/sched.h | 2 +- init/main.c | 2 +- kernel/pid.c | 22 +++++++++++++++++++--- kernel/reboot.c | 2 +- 4 files changed, 22 insertions(+), 6 deletions(-)include/linux/sched.h +1
-1@@ -1767,7 +1767,7 @@ static inline bool is_lazy_mmu_mode_active(void) }#endif -extern struct pid *cad_pid; +extern struct pid __rcu *cad_pid;/* * Per process flags init/main.c +1
-1@@ -1636,7 +1636,7 @@ static noinline void __init kernel_init_freeable(void)*/ set_mems_allowed (node_states[N_MEMORY ]); - cad_pid =get_pid (task_pid (current)); +rcu_assign_pointer (cad_pid,get_pid (task_pid (current)));smp_prepare_cpus (setup_max_cpus);kernel/pid.c +19
-3@@ -559,7 +559,13 @@ EXPORT_SYMBOL_GPL(pid_vnr);int kill_cad_pid (int sig,int priv) { -return kill_pid (cad_pid, sig, priv); +int ret; + +rcu_read_lock (); + ret =kill_pid (rcu_dereference (cad_pid), sig, priv); +rcu_read_unlock (); + +return ret; } pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,@@ -773,11 +779,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; - tmp_pid =pid_vnr (cad_pid); +rcu_read_lock (); + tmp_pid =pid_vnr (rcu_dereference (cad_pid)); +rcu_read_unlock (); + tmp_table.data = &tmp_pid; r =proc_dointvec (&tmp_table, write, buffer, lenp, ppos);@@ -788,7 +798,13 @@ 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 =unrcu_pointer (xchg (&cad_pid,RCU_INITIALIZER (new_pid))); +/* +* Wait for cad_pid readers before put_pid(). We cannot use +* call_rcu() here because free_pid() already owns pid->rcu. +*/ +synchronize_rcu (); +put_pid (old_pid);return 0 ; }kernel/reboot.c +1
-1@@ -24,7 +24,7 @@*/ static int C_A_D =1 ; -struct pid *cad_pid; +struct pid __rcu *cad_pid;EXPORT_SYMBOL (cad_pid);#if defined(CONFIG_ARM )
Signature · 2 lines
Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo.
Quoted 4 lines · Cen Zhang (Microsoft)
Move
kill_cad_pid()out of the header without changing behavior. This prepares for taking a reference tocad_pidunder RCU in the following fix.
...
kernel/pid.c pid_t pid_vnr(struct pid *pid) · line 563
Quoted 4 lines
kernel/pid.c +4
-0+int kill_cad_pid (int sig,int priv) +{ +return kill_pid (cad_pid, sig, priv); +}
Well... this version doesn't address another comment from sashiko.
Without EXPORT_SYMBOL() this can break the modules which use kill_cad_pid().
Say, drivers/acpi/tiny-power-button.c
I am starting to think that we should apply my patch first, it is more straightforward. Your 2/2 can be trivially rebased on top of it.
Oleg.
Ah, I missed checking sashiko. Thanks Oleg, please go ahead.
Quoted 22 lines · Cen Zhang (Microsoft)
Move
kill_cad_pid()out of the header without changing behavior. This prepares for taking a reference tocad_pidunder RCU in the following fix.Suggested-by Mateusz Guzik <[email protected]>Suggested-by Bradley Morgan <[email protected]>Reviewed-by Bradley Morgan <[email protected]>Link https://lore.kernel.org/all/CAGudoHH0vz=1m97EDHQFLLwGJmDPmqWJ+444b7rMiD059drEwQ@mail.gmail.com/ open hereSigned-off-by Cen Zhang (Microsoft) <[email protected]>---
v3: Add Reviewed-by from Bradley Morgan.
v2: New preparatory patch to deinlinekill_cad_pid().include/linux/sched/signal.h | 5 +---- kernel/pid.c | 5 +++++ 2 files changed, 6 insertions(+), 4 deletions(-)include/linux/sched/signal.h +0
-0
include/linux/sched/signal.h static inline sigset_t *sigmask_to_save(void) · line 565
Quoted 8 lines
include/linux/sched/signal.h +1
-4@@ -562,10 +562,7 @@ static inline sigset_t *sigmask_to_save(void)return res; } -static inline int kill_cad_pid (int sig,int priv) -{ -return kill_pid (cad_pid, sig, priv); -} +int kill_cad_pid (int sig,int priv);
Maybe we should explicitly include signal.h in kernel/reboot.c to have kill_cad_pid defined there? This already was there before this patch, and it compiled somehow, so it's definitely included implicitly somehow.
Quoted 19 lines
include/linux/sched/signal.h +0
-0/* These can be the second arg to send_sig_info/send_group_sig_info. */ #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)kernel/pid.c +5
-0@@ -557,6 +557,11 @@ pid_t pid_vnr(struct pid *pid) }EXPORT_SYMBOL_GPL (pid_vnr); +int kill_cad_pid (int sig,int priv) +{ +return kill_pid (cad_pid, sig, priv); +} +pid_t __task_pid_nr_ns (struct task_struct *task,enum pid_type type,struct pid_namespace *ns) {
Signature · 2 lines
Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo.
Quoted 3 lines · Cen Zhang (Microsoft)
Move
kill_cad_pid()out of the header without changing behavior. This prepares for taking a reference tocad_pidunder RCU in the following fix.
nit: If you are going to uninline kill_cad_pid should be placed in signal.c not in pid.c
That is where everything else that sends signals lives.
Eric
Quoted 45 lines
Suggested-by Mateusz Guzik <[email protected]>Suggested-by Bradley Morgan <[email protected]>Reviewed-by Bradley Morgan <[email protected]>Link https://lore.kernel.org/all/CAGudoHH0vz=1m97EDHQFLLwGJmDPmqWJ+444b7rMiD059drEwQ@mail.gmail.com/ open hereSigned-off-by Cen Zhang (Microsoft) <[email protected]>---
v3: Add Reviewed-by from Bradley Morgan.
v2: New preparatory patch to deinlinekill_cad_pid().include/linux/sched/signal.h | 5 +---- kernel/pid.c | 5 +++++ 2 files changed, 6 insertions(+), 4 deletions(-)include/linux/sched/signal.h +1
-4@@ -562,10 +562,7 @@ static inline sigset_t *sigmask_to_save(void)return res; } -static inline int kill_cad_pid (int sig,int priv) -{ -return kill_pid (cad_pid, sig, priv); -} +int kill_cad_pid (int sig,int priv);/* These can be the second arg to send_sig_info/send_group_sig_info. */ #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)kernel/pid.c +5
-0@@ -557,6 +557,11 @@ pid_t pid_vnr(struct pid *pid) }EXPORT_SYMBOL_GPL (pid_vnr); +int kill_cad_pid (int sig,int priv) +{ +return kill_pid (cad_pid, sig, priv); +} +pid_t __task_pid_nr_ns (struct task_struct *task,enum pid_type type,struct pid_namespace *ns) {
Quoted 11 lines · Eric W. Biederman
Quoted 3 lines · Cen Zhang (Microsoft)
Move
kill_cad_pid()out of the header without changing behavior. This prepares for taking a reference tocad_pidunder RCU in the following fix.nit: If you are going to uninline
kill_cad_pidshould be placed insignal.cnot inpid.cThat is where everything else that sends signals lives.
Agreed, but...
I had a private discussion with Alexey. And he pointed out that if we move kill_cad_pid() to reboot.c we are almost ready to unexport cad_pid and make it static.
The only problem is kernel_init_freeable(). but I guess we can shift the cad_pid initialization to reboot_ksysfs_init().
Anyway, this needs another patch. So I agree with signal.c in 1/2.
Oleg.
Quoted 16 lines · Oleg Nesterov
Quoted 11 lines · Eric W. Biederman
Quoted 3 lines · Cen Zhang (Microsoft)
Move
kill_cad_pid()out of the header without changing behavior. This prepares for taking a reference tocad_pidunder RCU in the following fix.nit: If you are going to uninline
kill_cad_pidshould be placed insignal.cnot inpid.cThat is where everything else that sends signals lives.
Agreed, but...
Quoted 8 lines
I had a private discussion with Alexey. And he pointed out that if we move
kill_cad_pid()toreboot.cwe are almost ready to unexportcad_pidand make it static.The only problem is
kernel_init_freeable(). but I guess we can shift thecad_pidinitialization toreboot_ksysfs_init().Anyway, this needs another patch. So I agree with
signal.cin 1/2.
Forgot to mention... Either way 1/2 can remove EXPORT_SYMBOL(cad_pid) but it has to add EXPORT_SYMBOL(kill_cad_pid).
Oleg.
Thanks everyone for the review.
Oleg's sysctl placement change is now queued in sysctl-next and present in linux-next. I'll send a v4 shortly. It is rebased on current sysctl-next code and addresses the remaining review comments.
Thanks,
Cen