Re: [PATCH v3 2/2] selftests: Add kselftest for syscall user dispatch

From: Kees Cook
Date: Mon Jul 13 2020 - 20:22:48 EST

Next message: Tony Finch: "Re: [PATCH] unifdef: Replace HTTP links with HTTPS ones"
Previous message: Inki Dae: "Re: [PATCH v7 07/36] drm: exynos: use common helper for a scatterlist contiguity check"
In reply to: Gabriel Krisman Bertazi: "[PATCH v3 2/2] selftests: Add kselftest for syscall user dispatch"
Next in thread: Gabriel Krisman Bertazi: "[PATCH v3 1/2] kernel: Implement selective syscall userspace redirection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Jul 12, 2020 at 12:45:16AM -0400, Gabriel Krisman Bertazi wrote:
> Implement functionality tests for syscall user dispatch. In order to
> make the test portable, refrain from open coding syscall dispatchers and
> calculating glibc memory ranges.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
> ---
> tools/testing/selftests/Makefile | 1 +
> .../syscall_user_dispatch/.gitignore | 1 +
> .../selftests/syscall_user_dispatch/Makefile | 5 +
> .../selftests/syscall_user_dispatch/config | 1 +
> .../syscall_user_dispatch.c | 259 ++++++++++++++++++
> 5 files changed, 267 insertions(+)
> create mode 100644 tools/testing/selftests/syscall_user_dispatch/.gitignore
> create mode 100644 tools/testing/selftests/syscall_user_dispatch/Makefile
> create mode 100644 tools/testing/selftests/syscall_user_dispatch/config
> create mode 100644 tools/testing/selftests/syscall_user_dispatch/syscall_user_dispatch.c
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 1195bd85af38..31b07dd774a6 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -47,6 +47,7 @@ TARGETS += openat2
> TARGETS += rseq
> TARGETS += rtc
> TARGETS += seccomp
> +TARGETS += syscall_user_dispatch
> TARGETS += sigaltstack
> TARGETS += size
> TARGETS += sparc64

nit: moar alphabetical! :)

> diff --git a/tools/testing/selftests/syscall_user_dispatch/.gitignore b/tools/testing/selftests/syscall_user_dispatch/.gitignore
> new file mode 100644
> index 000000000000..fadfb304c539
> --- /dev/null
> +++ b/tools/testing/selftests/syscall_user_dispatch/.gitignore
> @@ -0,0 +1 @@
> +syscall_user_dispatch

nit: this needs as the first line:

# SPDX-License-Identifier: GPL-2.0-only

> diff --git a/tools/testing/selftests/syscall_user_dispatch/Makefile b/tools/testing/selftests/syscall_user_dispatch/Makefile
> new file mode 100644
> index 000000000000..4785c98d4714
> --- /dev/null
> +++ b/tools/testing/selftests/syscall_user_dispatch/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +CFLAGS += -Wall
> +
> +TEST_GEN_PROGS := syscall_user_dispatch
> +include ../lib.mk
> diff --git a/tools/testing/selftests/syscall_user_dispatch/config b/tools/testing/selftests/syscall_user_dispatch/config
> new file mode 100644
> index 000000000000..22c4dfe167ca
> --- /dev/null
> +++ b/tools/testing/selftests/syscall_user_dispatch/config
> @@ -0,0 +1 @@
> +CONFIG_SYSCALL_USER_DISPATCH=y
> diff --git a/tools/testing/selftests/syscall_user_dispatch/syscall_user_dispatch.c b/tools/testing/selftests/syscall_user_dispatch/syscall_user_dispatch.c
> new file mode 100644
> index 000000000000..d713147863ef
> --- /dev/null
> +++ b/tools/testing/selftests/syscall_user_dispatch/syscall_user_dispatch.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020 Collabora Ltd.
> + *
> + * Test code for syscall user dispatch
> + */
> +
> +#define _GNU_SOURCE
> +#include <sys/types.h>
> +#include <sys/prctl.h>
> +#include <sys/syscall.h>
> +#include <sys/sysinfo.h>
> +#include <signal.h>
> +#include <errno.h>
> +
> +#include "../kselftest_harness.h"
> +
> +#ifndef PR_SET_SYSCALL_USER_DISPATCH

style nit: I usually use the initial ifndef to wrap all those related to
it. i.e.:

#ifndef PR_SET_SYSCALL_USER_DISPATCH
# define PR_SET_SYSCALL_USER_DISPATCH 59
# define PR_SYS_DISPATCH_OFF 0
# define PR_SYS_DISPATCH_ON 1
...
#endif

But either way is fine.

> +# define PR_SET_SYSCALL_USER_DISPATCH 59
> +#endif
> +
> +#ifndef PR_SYS_DISPATCH_OFF
> +# define PR_SYS_DISPATCH_OFF 0
> +#endif
> +
> +#ifndef PR_SYS_DISPATCH_ON
> +# define PR_SYS_DISPATCH_ON 1
> +#endif
> +
> +#ifndef SYS_USER_DISPATCH
> +# define SYS_USER_DISPATCH 2
> +#endif
> +
> +#define SYSCALL_DISPATCH_ON(x) ((x) = 1)
> +#define SYSCALL_DISPATCH_OFF(x) ((x) = 0)
> +
> +/* Test Summary:
> + *
> + * - dispatch_trigger_sigsys: Verify if PR_SET_SYSCALL_USER_DISPATCH is
> + * able to trigger SIGSYS on a syscall.
> + *
> + * - bad_selector: Test that a bad selector value triggers SIGSEGV.
> + *
> + * - bad_prctl_param: Test that the API correctly rejects invalid
> + * parameters on prctl
> + *
> + * - dispatch_and_return: Test that a syscall is selectively dispatched
> + * to userspace depending on the value of selector.
> + *
> + * - disable_dispatch: Test that the PR_SYS_DISPATCH_OFF correctly
> + * disables the dispatcher
> + *
> + * - direct_dispatch_range: Test that a syscall within the allowed range
> + * can bypass the dispatcher.
> + */
> +
> +TEST_SIGNAL(dispatch_trigger_sigsys, SIGSYS)
> +{
> + char sel = 0;
> + struct sysinfo info;
> + int ret;
> +
> + ret = sysinfo(&info);
> + ASSERT_EQ(0, ret);
> +
> + ret = prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 0, 0, &sel);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support CONFIG_SYSCALL_USER_DISPATCH");
> + }
> +
> + SYSCALL_DISPATCH_ON(sel);
> +
> + sysinfo(&info);
> +
> + EXPECT_FALSE(true) {
> + TH_LOG("Unreachable!");
> + }
> +}
> +
> +TEST_SIGNAL(bad_selector, SIGSEGV)
> +{
> + char sel = -1;
> + long ret;
> +
> + ret = prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 0, 0, &sel);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support CONFIG_SYSCALL_USER_DISPATCH");
> + }
> + EXPECT_FALSE(true) {
> + TH_LOG("Unreachable!");
> + }
> +}
> +
> +TEST(bad_prctl_param)
> +{
> + char sel = 0;
> + int op;
> +
> + /* Invalid op */
> + op = -1;
> + prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel);
> + ASSERT_EQ(EINVAL, errno);
> +
> + /* PR_SYS_DISPATCH_OFF */
> + op = PR_SYS_DISPATCH_OFF;
> +
> + /* start_addr != 0 */
> + prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0xff, 0);
> + EXPECT_EQ(EINVAL, errno);
> +
> + /* end_addr != 0 */
> + prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0);
> + EXPECT_EQ(EINVAL, errno);
> +
> + /* sel != NULL */
> + prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x0, &sel);
> + EXPECT_EQ(EINVAL, errno);

I think here would be a good place to test PR_GET_... too?

> +
> + /* Valid parameter */
> + errno = 0;
> + syscall(SYS_prctl, PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x0, 0x0);
> + EXPECT_EQ(0, errno);
> +
> + /* PR_SYS_DISPATCH_ON */
> + op = PR_SYS_DISPATCH_ON;
> +
> + /* start_addr > end_addr */
> + syscall(SYS_prctl, PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0x0, &sel);
> + EXPECT_EQ(EINVAL, errno);
> +
> + /* Invalid selector */
> + syscall(SYS_prctl, PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x1, (void *) -1);
> + ASSERT_EQ(EFAULT, errno);
> +}
> +
> +#define MAGIC_SYSCALL_1 0xff00 /* Bad Linux syscall number */

Some archs do weird things with syscalls. Maybe use __NR_syscalls + 1?
For x86, this should be fine, though.

> +
> +/*
> + * Use global selector for handle_sigsys tests, to avoid passing
> + * selector to signal handler
> + */
> +char glob_sel;
> +int nr_syscalls_emulated;
> +int si_code;
> +
> +static void handle_sigsys(int sig, siginfo_t *info, void *ucontext)
> +{
> + si_code = info->si_code;
> +
> + if (info->si_syscall == MAGIC_SYSCALL_1)
> + nr_syscalls_emulated++;
> +
> + /* In preparation for sigreturn. */
> + SYSCALL_DISPATCH_OFF(glob_sel);
> +}
> +
> +TEST(dispatch_and_return)
> +{
> + long ret;
> + struct sigaction act;
> + sigset_t mask;
> +
> + glob_sel = 0;
> + nr_syscalls_emulated = 0;
> + si_code = 0;
> +
> + memset(&act, 0, sizeof(act));
> + sigemptyset(&mask);
> +
> + act.sa_sigaction = handle_sigsys;
> + act.sa_flags = SA_SIGINFO;
> + act.sa_mask = mask;
> +
> + ret = sigaction(SIGSYS, &act, NULL);
> + ASSERT_EQ(0, ret);
> +
> + /* Make sure selector is good prior to prctl. */
> + SYSCALL_DISPATCH_OFF(glob_sel);
> +
> + ret = prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 0, 0, &glob_sel);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support CONFIG_SYSCALL_USER_DISPATCH");
> + }
> +
> + /* MAGIC_SYSCALL_1 doesn't exist. */
> + SYSCALL_DISPATCH_OFF(glob_sel);
> + ret = syscall(MAGIC_SYSCALL_1);
> + EXPECT_EQ(-1, ret) {
> + TH_LOG("Dispatch triggered unexpectedly");
> + }
> +
> + /* MAGIC_SYSCALL_1 should be emulated. */
> + nr_syscalls_emulated = 0;
> + SYSCALL_DISPATCH_ON(glob_sel);
> +
> + ret = syscall(MAGIC_SYSCALL_1);
> + EXPECT_EQ(MAGIC_SYSCALL_1, ret) {
> + TH_LOG("Failed to intercept syscall");
> + }
> + EXPECT_EQ(1, nr_syscalls_emulated) {
> + TH_LOG("Failed to emulate syscall");
> + }
> + ASSERT_EQ(SYS_USER_DISPATCH, si_code) {
> + TH_LOG("Bad si_code in SIGSYS");
> + }
> +}
> +
> +TEST(disable_dispatch)
> +{
> + int ret;
> + struct sysinfo info;
> + char sel = 0;
> +
> + ret = prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 0, 0, &sel);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support CONFIG_SYSCALL_USER_DISPATCH");
> + }
> +
> + /* MAGIC_SYSCALL_1 doesn't exist. */
> + SYSCALL_DISPATCH_OFF(glob_sel);
> +
> + ret = prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_OFF, 0, 0, 0);
> + EXPECT_EQ(0, ret) {
> + TH_LOG("Failed to unset syscall user dispatch");
> + }
> +
> + /* Shouldn't have any effect... */
> + SYSCALL_DISPATCH_ON(glob_sel);
> +
> + ret = syscall(SYS_sysinfo, &info);
> + EXPECT_EQ(0, ret) {
> + TH_LOG("Dispatch triggered unexpectedly");
> + }
> +}
> +
> +TEST(direct_dispatch_range)
> +{
> + int ret = 0;
> + struct sysinfo info;
> + char sel = 0;
> +
> + /*
> + * Instead of calculating libc addresses; allow the entire
> + * memory map and lock the selector.
> + */
> + ret = prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 0, -1L, &sel);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support CONFIG_SYSCALL_USER_DISPATCH");
> + }
> +
> + SYSCALL_DISPATCH_ON(sel);
> +
> + ret = syscall(SYS_sysinfo, &info);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Dispatch triggered unexpectedly");
> + }
> +}
> +
> +TEST_HARNESS_MAIN

Yay tests! :) Thank you! :)

--
Kees Cook

Next message: Tony Finch: "Re: [PATCH] unifdef: Replace HTTP links with HTTPS ones"
Previous message: Inki Dae: "Re: [PATCH v7 07/36] drm: exynos: use common helper for a scatterlist contiguity check"
In reply to: Gabriel Krisman Bertazi: "[PATCH v3 2/2] selftests: Add kselftest for syscall user dispatch"
Next in thread: Gabriel Krisman Bertazi: "[PATCH v3 1/2] kernel: Implement selective syscall userspace redirection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]