Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 9 Jul 2020 12:47:06 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     tglx@linutronix.de, linux-kernel@vger.kernel.org,
        kernel@collabora.com, Andy Lutomirski <luto@kernel.org>,
        Paul Gofman <gofmanp@gmail.com>,
        Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v2] kernel: Implement selective syscall userspace
 redirection
Message-ID: <20200709114706.GB12769@casper.infradead.org>
References: <20200709043840.4189530-1-krisman@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200709043840.4189530-1-krisman@collabora.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 09, 2020 at 12:38:40AM -0400, Gabriel Krisman Bertazi wrote:
> The proposed interface looks like this:
> 
>   prctl(PR_SET_SYSCALL_USER_DISPATCH, <op>, <dispatcher>, [selector])
> 
> Dispatcher is the address of a syscall instruction that is allowed to
> by-pass the blockage, such that in fast paths you don't need to disable
> the trap nor check the selector.  This is essential to return from
> SIGSYS to a blocked area without triggering another SIGSYS from the
> rt_sigreturn.

Should <dispatcher> be a single pointer or should the interface specify
a range from which syscalls may be made without being redirected?  eg,
one could specify the whole of libc.

prctl(PR_SET_SYSCALL_USER_DISPATCH, <op>, <start>, <inclusive-end>, [selector])

> +++ b/include/linux/syscall_user_dispatch.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _SYSCALL_USER_DISPATCH_H
> +#define _SYSCALL_USER_DISPATCH_H
> +
> +struct task_struct;
> +static void clear_tsk_thread_flag(struct task_struct *tsk, int flag);
> +
> +#ifdef CONFIG_SYSCALL_USER_DISPATCH
> +struct syscall_user_dispatch {
> +	int __user *selector;
> +	unsigned long __user dispatcher;

The __user annotation is on the pointer, not the value.  ie, it's

	unsigned long foo;
	unsigned long __user *p;

	get_user(foo, p)

> +++ b/include/uapi/asm-generic/siginfo.h
> @@ -285,6 +285,7 @@ typedef struct siginfo {
>   */
>  #define SYS_SECCOMP	1	/* seccomp triggered */
>  #define NSIGSYS		1
> +#define SYS_USER_REDIRECT 2

I'd suggest that SYS_USER_REDIRECT should be moved up by one line.

> +int set_syscall_user_dispatch(int mode, unsigned long __user dispatcher,
> +			      int __user *selector)
> +{
> +	switch (mode) {
> +	case PR_SYSCALL_DISPATCH_DISABLE:
> +		if (dispatcher || selector)
> +			return -EINVAL;
> +		break;
> +	case PR_SYSCALL_DISPATCH_ENABLE:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (selector) {
> +		if (!access_ok(selector, sizeof(int)))
> +			return -EFAULT;
> +	}

You're not enforcing the alignment requirement here.  

> +	spin_lock_irq(&current->sighand->siglock);
> +
> +	current->syscall_dispatch.selector = selector;
> +	current->syscall_dispatch.dispatcher = dispatcher;
> +
> +	/* make sure fastlock is committed before setting the flag. */

fastlock?  ;-)
I don't think you actually need this.  You're setting per-thread state on
yourself, so what's the race that you're concerned about?

> +	smp_mb__before_atomic();
> +
> +	if (mode == PR_SYSCALL_DISPATCH_ENABLE)
> +		set_tsk_thread_flag(current, TIF_SYSCALL_USER_DISPATCH);
> +	else
> +		clear_tsk_thread_flag(current, TIF_SYSCALL_USER_DISPATCH);
> +
> +	spin_unlock_irq(&current->sighand->siglock);
> +
> +	return 0;
> +}
> -- 
> 2.27.0
>