[PATCH v3 1/2] pid: deinline kill_cad_pid

[PATCH v3 1/2] pid: deinline kill_cad_pid

[Cen Zhang (Microsoft)]

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.

Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Suggested-by: Bradley Morgan <include@grrlz.net>
Reviewed-by: Bradley Morgan <include@grrlz.net>
Link: https://lore.kernel.org/all/CAGudoHH0vz=1m97EDHQFLLwGJmDPmqWJ+444b7rMiD059drEwQ@mail.gmail.com/
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
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(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 584ae88b435e..d45a5476b97d 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -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)
diff --git a/kernel/pid.c b/kernel/pid.c
index f55189a3d07d..234ebee29375 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -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)
 {
-- 
2.53.0

[PATCH v3 2/2] pid: fix cad_pid use-after-free race

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

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

Fixes: 9ec52099e4b8 ("[PATCH] replace cad_pid by a struct pid")
Reported-by: AutonomousCodeSecurity@microsoft.com
Closes: https://lore.kernel.org/all/20260717210143.4734-1-blbllhy@gmail.com/
Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Bradley Morgan <include@grrlz.net>
Cc: stable@vger.kernel.org
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
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(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 373bcc0598d1..31ce72b1233c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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
diff --git a/init/main.c b/init/main.c
index e363232b428b..19a10d0c2760 100644
--- a/init/main.c
+++ b/init/main.c
@@ -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);
 
diff --git a/kernel/pid.c b/kernel/pid.c
index 234ebee29375..fb7c0d18efa9 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -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 *buffe
 	if (!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;
 }
 
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 695c33e75efd..fc191a48c0e9 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -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)
-- 
2.53.0

Re: [PATCH v3 1/2] pid: deinline kill_cad_pid

[Oleg Nesterov]

On 07/19, Cen Zhang (Microsoft) wrote:
>
> 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.

...

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

Re: [PATCH v3 1/2] pid: deinline kill_cad_pid

[Cen Zhang (Microsoft)]

Ah, I missed checking sashiko. Thanks Oleg, please go ahead.

Re: [PATCH v3 1/2] pid: deinline kill_cad_pid

[Pavel Tikhomirov]

On 7/19/26 17:58, Cen Zhang (Microsoft) wrote:
> 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.
> 
> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Suggested-by: Bradley Morgan <include@grrlz.net>
> Reviewed-by: Bradley Morgan <include@grrlz.net>
> Link: https://lore.kernel.org/all/CAGudoHH0vz=1m97EDHQFLLwGJmDPmqWJ+444b7rMiD059drEwQ@mail.gmail.com/
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
> 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(-)
> 
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index 584ae88b435e..d45a5476b97d 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -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.

>  
>  /* These can be the second arg to send_sig_info/send_group_sig_info.  */
>  #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f55189a3d07d..234ebee29375 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -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)
>  {

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.

Re: [PATCH v3 2/2] pid: fix cad_pid use-after-free race

[Pavel Tikhomirov]

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

On 7/19/26 17:58, Cen Zhang (Microsoft) 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.
> 
> 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()
> 
> Fixes: 9ec52099e4b8 ("[PATCH] replace cad_pid by a struct pid")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Closes: https://lore.kernel.org/all/20260717210143.4734-1-blbllhy@gmail.com/
> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: Bradley Morgan <include@grrlz.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
> 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(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 373bcc0598d1..31ce72b1233c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -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
> diff --git a/init/main.c b/init/main.c
> index e363232b428b..19a10d0c2760 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -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);
>  
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 234ebee29375..fb7c0d18efa9 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -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 *buffe
>  	if (!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;
>  }
>  
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 695c33e75efd..fc191a48c0e9 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -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)

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.

Re: [PATCH v3 1/2] pid: deinline kill_cad_pid

[Eric W. Biederman]

"Cen Zhang (Microsoft)" <blbllhy@gmail.com> writes:

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

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

> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Suggested-by: Bradley Morgan <include@grrlz.net>
> Reviewed-by: Bradley Morgan <include@grrlz.net>
> Link: https://lore.kernel.org/all/CAGudoHH0vz=1m97EDHQFLLwGJmDPmqWJ+444b7rMiD059drEwQ@mail.gmail.com/
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
> 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(-)
>
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index 584ae88b435e..d45a5476b97d 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -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)
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f55189a3d07d..234ebee29375 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -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)
>  {

Re: [PATCH v3 1/2] pid: deinline kill_cad_pid

[Oleg Nesterov]

On 07/20, Eric W. Biederman wrote:
>
> "Cen Zhang (Microsoft)" <blbllhy@gmail.com> writes:
>
> > 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.
>
> 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.

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.

Re: [PATCH v3 1/2] pid: deinline kill_cad_pid

[Oleg Nesterov]

On 07/20, Oleg Nesterov wrote:
>
> On 07/20, Eric W. Biederman wrote:
> >
> > "Cen Zhang (Microsoft)" <blbllhy@gmail.com> writes:
> >
> > > 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.
> >
> > 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.
>
> 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.

Forgot to mention... Either way 1/2 can remove EXPORT_SYMBOL(cad_pid)
but it has to add EXPORT_SYMBOL(kill_cad_pid).

Oleg.