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@xxxxxxxxx>
Thanks!