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;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     luto@kernel.org, tglx@linutronix.de, keescook@chromium.org,
        gofmanp@gmail.com, christian.brauner@ubuntu.com,
        peterz@infradead.org, willy@infradead.org, shuah@kernel.org,
        linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
        linux-kselftest@vger.kernel.org, x86@kernel.org,
        kernel@collabora.com
References: <20201127193238.821364-1-krisman@collabora.com>
        <20201127193238.821364-4-krisman@collabora.com>
Date:   Wed, 30 Jun 2021 16:44:41 -0500
In-Reply-To: <20201127193238.821364-4-krisman@collabora.com> (Gabriel Krisman
        Bertazi's message of "Fri, 27 Nov 2020 14:32:34 -0500")
Message-ID: <8735szowmu.fsf@disp2133>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH v8 3/7] kernel: Implement selective syscall userspace redirection
Precedence: bulk


Why does do_syscal_user_dispatch call do_exit(SIGSEGV) and
do_exit(SIGSYS) instead of force_sig(SIGSEGV) and force_sig(SIGSYS)?

Looking at the code these cases are not expected to happen, so I would
be surprised if userspace depends on any particular behaviour on the
failure path so I think we can change this.

Is using do_exit in this way something you copied from seccomp?

The reason I am asking is that by using do_exit you deprive userspace
of the change to catch the signal handler and try and fix things.

Also by using do_exit only a single thread of a multi-thread application
is terminated which seems wrong.

I am asking because I am going through the callers of do_exit so I can
refactor things and clean things up and this use just looks wrong.

Gabriel Krisman Bertazi <krisman@collabora.com> writes:

<snip>

> +bool do_syscall_user_dispatch(struct pt_regs *regs)
> +{
> +	struct syscall_user_dispatch *sd = &current->syscall_dispatch;
> +	char state;
> +
> +	if (likely(instruction_pointer(regs) - sd->offset < sd->len))
> +		return false;
> +
> +	if (unlikely(arch_syscall_is_vdso_sigreturn(regs)))
> +		return false;
> +
> +	if (likely(sd->selector)) {
> +		/*
> +		 * access_ok() is performed once, at prctl time, when
> +		 * the selector is loaded by userspace.
> +		 */
> +		if (unlikely(__get_user(state, sd->selector)))
> +			do_exit(SIGSEGV);
                        ^^^^^^^^^^^^^^^^

I think it makes more sense if the code does:

		if (unlikely(__get_user(state, sd->selector))) {
                	force_sig(SIGSEGV);
                        return true;
                }

> +
> +		if (likely(state == PR_SYS_DISPATCH_OFF))
> +			return false;
> +
> +		if (state != PR_SYS_DISPATCH_ON)
> +			do_exit(SIGSYS);
                        ^^^^^^^^^^^^^^^
> +	}
> +
> +	sd->on_dispatch = true;
> +	syscall_rollback(current, regs);
> +	trigger_sigsys(regs);
> +
> +	return true;
> +}

Eric