Re: [PATCH v11 2/2] ptrace,syscall_user_dispatch: checkpoint/restore support for SUD

From: Andrei Vagin
Date: Fri Feb 24 2023 - 03:18:16 EST

On Tue, Feb 21, 2023 at 12:18 PM Gregory Price
<gourry.memverge@xxxxxxxxx> wrote:

...

> diff --git a/kernel/entry/syscall_user_dispatch.c b/kernel/entry/syscall_user_dispatch.c
> index 22396b234854..08e8b377557f 100644
> --- a/kernel/entry/syscall_user_dispatch.c
> +++ b/kernel/entry/syscall_user_dispatch.c
> @@ -4,6 +4,7 @@
> */
> #include <linux/sched.h>
> #include <linux/prctl.h>
> +#include <linux/ptrace.h>
> #include <linux/syscall_user_dispatch.h>
> #include <linux/uaccess.h>
> #include <linux/signal.h>
> @@ -113,3 +114,42 @@ int set_syscall_user_dispatch(unsigned long mode, unsigned long offset,
> {
> return task_set_syscall_user_dispatch(current, mode, offset, len, selector);
> }
> +
> +int syscall_user_dispatch_get_config(struct task_struct *task, unsigned long size,
> + void __user *data)
> +{
> + struct syscall_user_dispatch *sd = &task->syscall_dispatch;
> + struct ptrace_sud_config config;

WARNING: Missing a blank line after declarations

You need to verify all patches with ./scripts/checkpatch.pl. Here are
a few other warnings.

> + if (size != sizeof(struct ptrace_sud_config))
> + return -EINVAL;
> +

config has to be fully initialized otherwise it leaks data from a kernel stack.

> + if (test_task_syscall_work(task, SYSCALL_USER_DISPATCH))
> + config.mode = PR_SYS_DISPATCH_ON;
> + else
> + config.mode = PR_SYS_DISPATCH_OFF;
> +
> + config.offset = sd->offset;
> + config.len = sd->len;
> + config.selector = (__u64)sd->selector;
> +
> + if (copy_to_user(data, &config, sizeof(config))) {
> + return -EFAULT;
> + }
> + return 0;
> +}
> +
> +int syscall_user_dispatch_set_config(struct task_struct *task, unsigned long size,
> + void __user *data)
> +{
> + int rc;
> + struct ptrace_sud_config cfg;
> + if (size != sizeof(struct ptrace_sud_config))
> + return -EINVAL;
> +
> + if (copy_from_user(&cfg, data, sizeof(cfg))) {
> + return -EFAULT;
> + }
> + rc = task_set_syscall_user_dispatch(task, cfg.mode, cfg.offset,
> + cfg.len, (void __user*)cfg.selector);
> + return rc;
> +}
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 54482193e1ed..d99376532b56 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -32,6 +32,7 @@
> #include <linux/compat.h>
> #include <linux/sched/signal.h>
> #include <linux/minmax.h>
> +#include <linux/syscall_user_dispatch.h>
>
> #include <asm/syscall.h> /* for syscall_get_* */
>
> @@ -1259,6 +1260,14 @@ int ptrace_request(struct task_struct *child, long request,
> break;
> #endif
>
> + case PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG:
> + ret = syscall_user_dispatch_set_config(child, addr, datavp);
> + break;
> +
> + case PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG:
> + ret = syscall_user_dispatch_get_config(child, addr, datavp);
> + break;
> +
> default:
> break;
> }
> diff --git a/tools/testing/selftests/ptrace/.gitignore b/tools/testing/selftests/ptrace/.gitignore
> index 792318aaa30c..b7dde152e75a 100644
> --- a/tools/testing/selftests/ptrace/.gitignore
> +++ b/tools/testing/selftests/ptrace/.gitignore
> @@ -1,4 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0-only
> get_syscall_info
> +get_set_sud
> peeksiginfo
> vmaccess
> diff --git a/tools/testing/selftests/ptrace/Makefile b/tools/testing/selftests/ptrace/Makefile
> index 2f1f532c39db..33a36b73bcb9 100644
> --- a/tools/testing/selftests/ptrace/Makefile
> +++ b/tools/testing/selftests/ptrace/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> CFLAGS += -std=c99 -pthread -iquote../../../../include/uapi -Wall
>
> -TEST_GEN_PROGS := get_syscall_info peeksiginfo vmaccess
> +TEST_GEN_PROGS := get_syscall_info peeksiginfo vmaccess get_set_sud
>
> include ../lib.mk
> diff --git a/tools/testing/selftests/ptrace/get_set_sud.c b/tools/testing/selftests/ptrace/get_set_sud.c

I think the test has to be in a separate patch.

> new file mode 100644
> index 000000000000..c4e7b87cab03
> --- /dev/null
> +++ b/tools/testing/selftests/ptrace/get_set_sud.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include "../kselftest_harness.h"
> +#include <stdio.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <sys/wait.h>
> +#include <sys/syscall.h>
> +#include <sys/prctl.h>
> +
> +#include "linux/ptrace.h"
> +
> +static int sys_ptrace(int request, pid_t pid, void *addr, void *data)
> +{
> + return syscall(SYS_ptrace, request, pid, addr, data);
> +}
> +
> +TEST(get_set_sud)
> +{
> + struct ptrace_sud_config config;
> + pid_t child;
> + int ret = 0;
> + int status;
> +
> + child = fork();
> + ASSERT_GE(child, 0);
> + if (child == 0) {
> + ASSERT_EQ(0, sys_ptrace(PTRACE_TRACEME, 0, 0, 0)) {
> + TH_LOG("PTRACE_TRACEME: %m");
> + }
> + kill(getpid(), SIGSTOP);
> + _exit(1);
> + }
> +
> + waitpid(child, &status, 0);
> +
> + memset(&config, 0xff, sizeof(config));
> + config.mode = PR_SYS_DISPATCH_ON;
> +
> + ret = sys_ptrace(PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG, child,
> + (void*)sizeof(config), &config);
> + if (ret < 0) {
> + ASSERT_EQ(errno, EIO);

When do we expect to get EIO here?

> + goto leave;
> + }
> +
> + ASSERT_EQ(ret, 0);
> + ASSERT_EQ(config.mode, PR_SYS_DISPATCH_OFF);
> + ASSERT_EQ(config.selector, 0);
> + ASSERT_EQ(config.offset, 0);
> + ASSERT_EQ(config.len, 0);
> +
> + config.mode = PR_SYS_DISPATCH_ON;
> + config.selector = 0;
> + config.offset = 0x400000;
> + config.len = 0x1000;
> +
> + ret = sys_ptrace(PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG, child,
> + (void*)sizeof(config), &config);
> +
> + ASSERT_EQ(ret, 0);
> +
> + memset(&config, 1, sizeof(config));
> + ret = sys_ptrace(PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG, child,
> + (void*)sizeof(config), &config);
> +
> + ASSERT_EQ(ret, 0);
> + ASSERT_EQ(config.mode, PR_SYS_DISPATCH_ON);
> + ASSERT_EQ(config.selector, 0);
> + ASSERT_EQ(config.offset, 0x400000);
> + ASSERT_EQ(config.len, 0x1000);
> +
> +leave:
> + kill(child, SIGKILL);
> +}
> +
> +TEST_HARNESS_MAIN
> --
> 2.39.1
>