lkml.org 
[lkml]   [2018]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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;
    > +}

    \
     
     \ /
      Last update: 2018-03-28 14:31    [W:4.094 / U:0.368 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site