lkml.org 
[lkml]   [2020]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch V3 01/13] entry: Provide generic syscall entry functionality
    On Thu, Jul 16, 2020 at 08:22:09PM +0200, Thomas Gleixner wrote:
    > From: Thomas Gleixner <tglx@linutronix.de>
    >
    > On syscall entry certain work needs to be done:
    >
    > - Establish state (lockdep, context tracking, tracing)
    > - Conditional work (ptrace, seccomp, audit...)
    >
    > This code is needlessly duplicated and different in all
    > architectures.
    >
    > Provide a generic version based on the x86 implementation which has all the
    > RCU and instrumentation bits right.

    Ahh! You're reading my mind! I was just thinking about this while
    reviewing the proposed syscall redirection series[1], and pondering the
    lack of x86 TIF flags, and that nearly everything in the series (and for
    seccomp and other things) didn't need to be arch-specific. And now that
    series absolutely needs to be rebased and it'll magically work for every
    arch that switches to the generic entry code. :)

    Notes below...

    [1] https://lore.kernel.org/lkml/20200716193141.4068476-2-krisman@collabora.com/

    > +/*
    > + * Define dummy _TIF work flags if not defined by the architecture or for
    > + * disabled functionality.
    > + */

    When I was thinking about this last week I was pondering having a split
    between the arch-agnositc TIF flags and the arch-specific TIF flags, and
    that each arch could have a single "there is agnostic work to be done"
    TIF in their thread_info, and the agnostic flags could live in
    task_struct or something. Anyway, I'll keep reading...

    > +/**
    > + * syscall_enter_from_user_mode - Check and handle work before invoking
    > + * a syscall
    > + * @regs: Pointer to currents pt_regs
    > + * @syscall: The syscall number
    > + *
    > + * Invoked from architecture specific syscall entry code with interrupts
    > + * disabled. The calling code has to be non-instrumentable. When the
    > + * function returns all state is correct and the subsequent functions can be
    > + * instrumented.
    > + *
    > + * Returns: The original or a modified syscall number
    > + *
    > + * If the returned syscall number is -1 then the syscall should be
    > + * skipped. In this case the caller may invoke syscall_set_error() or
    > + * syscall_set_return_value() first. If neither of those are called and -1
    > + * is returned, then the syscall will fail with ENOSYS.

    There's been some recent confusion over "has the syscall changed,
    or did seccomp request it be skipped?" that was explored in arm64[2]
    (though I see Will and Keno in CC already). There might need to be a
    clearer way to distinguish between "wild userspace issued a -1 syscall"
    and "seccomp or ptrace asked for the syscall to be skipped". The
    difference is mostly about when ENOSYS gets set, with respect to calls
    to syscall_set_return_value(), but if the syscall gets changed, the arch
    may need to recheck the value and consider ENOSYS, etc. IIUC, what Will
    ended up with[3] was having syscall_trace_enter() return the syscall return
    value instead of the new syscall.

    [2] https://lore.kernel.org/lkml/20200704125027.GB21185@willie-the-truck/
    [3] https://lore.kernel.org/lkml/20200703083914.GA18516@willie-the-truck/

    > +static long syscall_trace_enter(struct pt_regs *regs, long syscall,
    > + unsigned long ti_work)
    > +{
    > + long ret = 0;
    > +
    > + /* Handle ptrace */
    > + if (ti_work & (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_EMU)) {
    > + ret = arch_syscall_enter_tracehook(regs);
    > + if (ret || (ti_work & _TIF_SYSCALL_EMU))
    > + return -1L;
    > + }
    > +
    > + /* Do seccomp after ptrace, to catch any tracer changes. */
    > + if (ti_work & _TIF_SECCOMP) {
    > + ret = arch_syscall_enter_seccomp(regs);
    > + if (ret == -1L)
    > + return ret;
    > + }
    > +
    > + if (unlikely(ti_work & _TIF_SYSCALL_TRACEPOINT))
    > + trace_sys_enter(regs, syscall);
    > +
    > + arch_syscall_enter_audit(regs);
    > +
    > + return ret ? : syscall;
    > +}

    Modulo the notes about -1 vs syscall number above, this looks correct to
    me for ptrace and seccomp.

    --
    Kees Cook

    \
     
     \ /
      Last update: 2020-07-16 22:53    [W:3.857 / U:0.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site