Date:   Wed, 28 Mar 2018 14:29:46 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     "Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Andy Lutomirski <luto@...capital.net>,
        Dave Watson <davejwatson@...com>, linux-kernel@...r.kernel.org,
        linux-api@...r.kernel.org, Paul Turner <pjt@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Russell King <linux@....linux.org.uk>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>, Andrew Hunter <ahh@...gle.com>,
        Andi Kleen <andi@...stfloor.org>, Chris Lameter <cl@...ux.com>,
        Ben Maurer <bmaurer@...com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Josh Triplett <josh@...htriplett.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences
 system call (v12)

On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
> +static int rseq_update_cpu_id(struct task_struct *t)
> +{
> +	uint32_t cpu_id = raw_smp_processor_id();

u32

> +
> +	if (__put_user(cpu_id, &t->rseq->cpu_id_start))
> +		return -EFAULT;
> +	if (__put_user(cpu_id, &t->rseq->cpu_id))
> +		return -EFAULT;
> +	trace_rseq_update(t);
> +	return 0;
> +}
> +
> +static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> +{
> +	uint32_t cpu_id_start = 0, cpu_id = RSEQ_CPU_ID_UNINITIALIZED;

u32

> +
> +	/*
> +	 * Reset cpu_id_start to its initial state (0).
> +	 */
> +	if (__put_user(cpu_id_start, &t->rseq->cpu_id_start))
> +		return -EFAULT;
> +	/*
> +	 * Reset cpu_id to RSEQ_CPU_ID_UNINITIALIZED, so any user coming
> +	 * in after unregistration can figure out that rseq needs to be
> +	 * registered again.
> +	 */
> +	if (__put_user(cpu_id, &t->rseq->cpu_id))
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int rseq_get_rseq_cs(struct task_struct *t,
> +			    unsigned long *start_ip,
> +			    unsigned long *post_commit_offset,
> +			    unsigned long *abort_ip,
> +			    uint32_t *cs_flags)
> +{
> +	struct rseq_cs __user *urseq_cs;
> +	struct rseq_cs rseq_cs;
> +	unsigned long ptr;
> +	u32 __user *usig;
> +	u32 sig;
> +	int ret;
> +
> +	ret = __get_user(ptr, &t->rseq->rseq_cs);
> +	if (ret)
> +		return ret;
> +	if (!ptr)
> +		return 0;
> +	urseq_cs = (struct rseq_cs __user *)ptr;
> +	if (copy_from_user(&rseq_cs, urseq_cs, sizeof(rseq_cs)))
> +		return -EFAULT;
> +	if (rseq_cs.version > 0)
> +		return -EINVAL;
> +
> +	/* Ensure that abort_ip is not in the critical section. */
> +	if (rseq_cs.abort_ip - rseq_cs.start_ip < rseq_cs.post_commit_offset)
> +		return -EINVAL;

The kernel will not crash if userspace messes that up right? So why do
we care to check?

> +
> +	*cs_flags = rseq_cs.flags;
> +	*start_ip = rseq_cs.start_ip;
> +	*post_commit_offset = rseq_cs.post_commit_offset;
> +	*abort_ip = rseq_cs.abort_ip;

Then this becomes a straight struct assignment.

> +
> +	usig = (u32 __user *)(rseq_cs.abort_ip - sizeof(u32));
> +	ret = get_user(sig, usig);
> +	if (ret)
> +		return ret;
> +

> +	if (current->rseq_sig != sig) {
> +		printk_ratelimited(KERN_WARNING
> +			"Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
> +			sig, current->rseq_sig, current->pid, usig);
> +		return -EPERM;
> +	}

Is there any text that explains the thread model and possible attack
that this signature prevents? I failed to find any, which raises the
question, why is it there..

> +	return 0;
> +}
> +
> +static int rseq_need_restart(struct task_struct *t, uint32_t cs_flags)

u32

> +{
> +	uint32_t flags, event_mask;

u32

> +	int ret;
> +
> +	/* Get thread flags. */
> +	ret = __get_user(flags, &t->rseq->flags);
> +	if (ret)
> +		return ret;
> +
> +	/* Take critical section flags into account. */
> +	flags |= cs_flags;
> +
> +	/*
> +	 * Restart on signal can only be inhibited when restart on
> +	 * preempt and restart on migrate are inhibited too. Otherwise,
> +	 * a preempted signal handler could fail to restart the prior
> +	 * execution context on sigreturn.
> +	 */
> +	if (unlikely(flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL)) {
> +		if ((flags & (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
> +		    | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)) !=
> +		    (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
> +		     | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT))
> +			return -EINVAL;

Please put operators at the end of the previous line, not at the start
of the new line when you have to break statements.

Also, that's unreadable.

#define RSEQ_CS_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT |	\
		       RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL  |	\
		       RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)

	if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
	             (flags & RSEQ_CS_FLAGS) != RSEQ_CS_FLAGS))
		return -EINVAL;


> +	}
> +
> +	/*
> +	 * Load and clear event mask atomically with respect to
> +	 * scheduler preemption.
> +	 */
> +	preempt_disable();
> +	event_mask = t->rseq_event_mask;
> +	t->rseq_event_mask = 0;
> +	preempt_enable();
> +
> +	event_mask &= ~flags;
> +	if (event_mask)
> +		return 1;
> +	return 0;

	return !!(event_mask & ~flags);

> +}
> +
> +static int clear_rseq_cs(struct task_struct *t)
> +{
> +	unsigned long ptr = 0;
> +
> +	/*
> +	 * The rseq_cs field is set to NULL on preemption or signal
> +	 * delivery on top of rseq assembly block, as well as on top
> +	 * of code outside of the rseq assembly block. This performs
> +	 * a lazy clear of the rseq_cs field.
> +	 *
> +	 * Set rseq_cs to NULL with single-copy atomicity.
> +	 */
> +	return __put_user(ptr, &t->rseq->rseq_cs);

	__put_user(0UL, &t->rseq->rseq_cs); ?

> +}
> +
> +static int rseq_ip_fixup(struct pt_regs *regs)
> +{
> +	unsigned long ip = instruction_pointer(regs), start_ip = 0,
> +		post_commit_offset = 0, abort_ip = 0;

valid C, but yuck. Just have two 'unsigned long' lines.

Also, why the =0, the below call to rseq_get_rseq_cs() will either
initialize of fail.

> +	struct task_struct *t = current;
> +	uint32_t cs_flags = 0;

u32

> +	bool in_rseq_cs = false;
> +	int ret;
> +
> +	ret = rseq_get_rseq_cs(t, &start_ip, &post_commit_offset, &abort_ip,
> +			&cs_flags);

	ret = rseq_get_rseq_cs(t, &start_ip, &post_commit_offset,
			       &abort_ip, &cs_flags);


> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Handle potentially not being within a critical section.
> +	 * Unsigned comparison will be true when
> +	 * ip >= start_ip, and when ip < start_ip + post_commit_offset.
> +	 */
> +	if (ip - start_ip < post_commit_offset)
> +		in_rseq_cs = true;
> +
> +	/*
> +	 * If not nested over a rseq critical section, restart is
> +	 * useless. Clear the rseq_cs pointer and return.
> +	 */
> +	if (!in_rseq_cs)
> +		return clear_rseq_cs(t);


That all seems needlessly complicated; isn't:

	if (ip - start_ip >= post_commit_offset)
		return clear_rseq_cs();

equivalent? Nothing seems to use that variable after this.

> +	ret = rseq_need_restart(t, cs_flags);
> +	if (ret <= 0)
> +		return ret;
> +	ret = clear_rseq_cs(t);
> +	if (ret)
> +		return ret;
> +	trace_rseq_ip_fixup(ip, start_ip, post_commit_offset, abort_ip);
> +	instruction_pointer_set(regs, (unsigned long)abort_ip);
> +	return 0;
> +}

Powered by blists - more mailing lists