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

[Bradley Morgan]

On July 18, 2026 2:19:57 PM GMT+01:00, Oleg Nesterov <oleg@xxxxxxxxxx>
wrote:
>On 07/17, Cen Zhang (Microsoft) wrote:
>>
>>  int kill_cad_pid(int sig, int priv)
>>  {
>> -	return kill_pid(cad_pid, sig, priv);
>> +	struct pid *pid;
>> +	int ret;
>> +
>> +	rcu_read_lock();
>> +	pid = get_pid(rcu_dereference(cad_pid));
>> +	rcu_read_unlock();
>> +
>> +	ret = kill_pid(pid, sig, priv);
>> +	put_pid(pid);
>
>Hmm. Why do we need get/put_pid() ? I think that
>
>	rcu_read_lock();
>	ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
>	rcu_read_unlock();
>
>	return ret;
>
>should work just fine?
>
>Oleg.
>
>

On 07/18, Oleg Nesterov wrote:
> Hmm. Why do we need get/put_pid() ? I think that
>
> 	rcu_read_lock();
> 	ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
> 	rcu_read_unlock();
>
> 	return ret;
>
> should work just fine?

youre right that the get/put is redundant. kill_pid() doesnt sleep, it just
grabs tasklist_lock, so doing it inside the read section is fine and the
extra ref buys nothing.

but i dont think either form is actually safe, and thats the bit id like
Cen to address. rcu_read_lock() only protects the pid if the freeing side
defers to a grace period. on the cad_pid path it doesnt:

proc_do_cad_pid():
	put_pid(xchg(&cad_pid, new_pid));

put_pid():
	if (refcount_dec_and_test(&pid->count)) {
		pidfs_free_pid(pid);
		kmem_cache_free(ns->pid_cachep, pid);	/* synchronous */
		put_pid_ns(ns);
	}

thats a straight kmem_cache_free, no call_rcu. only free_pid() goes through
call_rcu(delayed_put_pid), and thats dropping the hash reference, not the
reference cad_pid holds. and the pid cache isnt SLAB_TYPESAFE_BY_RCU:

	kmem_cache_create("pid", ...,
		SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT, NULL);

so:

	cpu0 kill_cad_pid		cpu1 proc_do_cad_pid
	rcu_read_lock()
	p = rcu_dereference(cad_pid)
					put_pid(old) // last ref, freed and reused
	kill_pid(p, ...)		// UAF
	rcu_read_unlock()

your form and Cens both hit this. Cens get_pid() is no safer either, its
a refcount_inc on the same p thats already freed, so its a UAF too, not a
more careful variant.

to make the rcu_read_lock() mean anything the writer has to defer. either
proc_do_cad_pid() drops the old pid via call_rcu instead of a bare
put_pid(), or the pid cache goes SLAB_TYPESAFE_BY_RCU and the reader uses a
not zero tryget. with the writer fixed your clean form is exactly right and
the ref stays unnecessary.

its reachable when cad_pid isnt init: root writes a pid, that task exits,
cad_pid ends up holding the last ref, and the next write frees it while a
signal is in flight.


Thanks!