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