Re: [PATCH v1 1/1] rseq: Validate read-only fields under DEBUG_RSEQ config
From: Peter Zijlstra
Date: Tue Nov 12 2024 - 09:48:03 EST
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..