linux-kernel - Re: [PATCH v8 1/1] ptrace,syscall_user_dispatch: checkpoint/restore support for SUD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 10 Feb 2023 00:45:24 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Gregory Price <gourry.memverge@...il.com>,
        linux-kernel@...r.kernel.org
Cc:     linux-doc@...r.kernel.org, oleg@...hat.com, avagin@...il.com,
        peterz@...radead.org, luto@...nel.org, krisman@...labora.com,
        corbet@....net, shuah@...nel.org,
        Gregory Price <gregory.price@...verge.com>
Subject: Re: [PATCH v8 1/1] ptrace,syscall_user_dispatch: checkpoint/restore
 support for SUD

On Tue, Jan 31 2023 at 09:44, Gregory Price wrote:
>  
> +static inline int syscall_user_dispatch_get_config(struct task_struct *task, unsigned long size,
> +	void __user *data)

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#line-breaks

All over the place.

> index 195ae64a8c87..6d2f3b86f932 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -112,6 +112,15 @@ struct ptrace_rseq_configuration {
>  	__u32 pad;
>  };
>  
> +#define PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG 0x4210
> +#define PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG 0x4211
> +struct syscall_user_dispatch_config {

Can you please visibly separate the defines from the struct definition
by a newline? Glueing that stuff together is just horrible to read.

> +	__u64 mode;
> +	__s8 *selector;
> +	__u64 offset;
> +	__u64 len;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Please add proper documentation to this struct. It's user space ABI and
it's not the job of the man page maintainers to figure out what this
actually means.

> +int syscall_user_dispatch_set_config(struct task_struct *task, unsigned long size,
> +		void __user *data)
> +{
> +	struct syscall_user_dispatch_config config;
> +
> +	if (size != sizeof(struct syscall_user_dispatch_config))
> +		return -EINVAL;
> +
> +	if (copy_from_user(&config, data, sizeof(config)))
> +		return -EFAULT;
> +
> +	return set_syscall_user_dispatch(config.mode, config.offset, config.len,
> +			config.selector);

How is this supposed to work? This is called from the ptracer to set the
user dispatch mode on the ptracee, i.e. on @task.

But set_syscall_user_dispatch() operates on current, which is the
ptracer itself.

Clearly well tested with the non-existant selftest, which is part of
this submission.

So please fix the above issues, add a selftest and proper documentation.

I'm neither impressed by this patch nor by the reviews.

Sigh

        tglx