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;
+}