Re: [PATCH v1 1/1] rseq: Validate read-only fields under DEBUG_RSEQ config

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Nov 12, 2024 at 09:27:23AM -0500, Mathieu Desnoyers wrote:

> Your approach looks indeed better than mine, I'll steal it with your
> permission. :)

Ofc.


> > > +#ifdef CONFIG_DEBUG_RSEQ
> > > +	/*
> > > +	 * Initialize the in-kernel rseq fields copy for validation of
> > > +	 * read-only fields.
> > > +	 */
> > > +	if (get_user(rseq_kernel_fields(current)->cpu_id_start, &rseq->cpu_id_start) ||
> > > +	    get_user(rseq_kernel_fields(current)->cpu_id, &rseq->cpu_id) ||
> > > +	    get_user(rseq_kernel_fields(current)->node_id, &rseq->node_id) ||
> > > +	    get_user(rseq_kernel_fields(current)->mm_cid, &rseq->mm_cid))
> > > +		return -EFAULT;
> > > +#endif
> > 
> > So I didn't change the above, but wouldn't it make more sense to do
> > rseq_reset_rseq_cpu_node_id() here, but without the validation?
> 
> Indeed we could do this (for both DEBUG_RSEQ={y,n}), but it would add extra
> useless stores to those userspace fields on rseq registration, which is
> performed on every thread creation starting from glibc 2.35. The
> rseq_set_notify_resume() invoked at the end of registration ensures that
> those fields get populated before return to userspace.
> 
> So I am not against a more robust approach, but I'm reluctant to add redundant
> work on thread creation.

Ah, indeed. Oh well..