On Thu, Jul 16, 2020 at 08:22:09PM +0200, Thomas Gleixner wrote: > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxx/ > +/* > + * 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
