linux-kernel - Re: [PATCH v11 2/2] 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:   Wed, 22 Feb 2023 10:24:06 -0500
From:   Gregory Price <gregory.price@...verge.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Gregory Price <gourry.memverge@...il.com>,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        avagin@...il.com, peterz@...radead.org, luto@...nel.org,
        krisman@...labora.com, tglx@...utronix.de, corbet@....net,
        shuah@...nel.org
Subject: Re: [PATCH v11 2/2] ptrace,syscall_user_dispatch: checkpoint/restore
 support for SUD

On Wed, Feb 22, 2023 at 01:48:35PM +0100, Oleg Nesterov wrote:
> On 02/21, Gregory Price wrote:
> >
> > +struct ptrace_sud_config {
> > +	__u8  mode;
> > +	__u8  pad[7];
>               ^^^^^^
> Why?
> 

The struct isn't packed, so this is for alignment/consistency of size.
The padding gets added anyway, and differently between 32/64 bit.
Without padding, allocating this in 32-bit mode creates a structure of
size 28 (4-byte aligned), while in 64-bit mode it creates a structure of
size 32 (8-byte aligned).

ptrace_syscall_info in the same file has the same thing.

> > +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;
> > +	if (size != sizeof(struct ptrace_sud_config))
> > +		return -EINVAL;
> 
> Andrei, do we really need this check?
> 

My understanding is that it's a sanity check against the above issue.
In fact, it was what lead me to add the padding.

Without the padding, sizeof(ptrace_sud_config) will be 32b in the kernel
and 28b in userland.

> > +
> > +	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;
> 
> As the kernel test robot reports, this is not -Wpointer-to-int-cast friendly.
> Please use uintptr_t. See for example ptrace_get_rseq_configuration(). Same
> for syscall_user_dispatch_set_config().
> 

.rseq_abi_pointer = (u64)(uintptr_t)task->rseq,

aye aye. I saw the error yesterday, I need to change my compile settings.

> > +	if (copy_to_user(data, &config, sizeof(config))) {
> 
> This leaks info in (uninitialized) config.pad[]. You can probably simply make
> config.mode __u64 as well.
> 
> Minor, but sizeof(struct ptrace_sud_config) above vs this sizeof(config)) doesn't
> look consistent to me...
> 

I hadn't considered uninit data. It's technically a __u32, but it's
probably just cleaner to promote/cast here than deal with padding.

> > +static int sys_ptrace(int request, pid_t pid, void *addr, void *data)
> > +{
> > +	return syscall(SYS_ptrace, request, pid, addr, data);
> > +}
> 
> Why can't you simply use ptrace() ?
> 
> Oleg.
> 

ptrace() is the libc wrapper around the syscall.

I would assume there are issues with #include <sys/ptrace.h> and
#include <linux/ptrace.h> in the same file. Since libc doesn't have the
new definitions.

Not sure if there's any stipulations around how selftests have to be
written, i just wrote this one based on the surrounding tests and got
it to work.  I would think direct usage of the syscall is preferred,
but i'm ignorant here.

I'll make some changes and give it a few days before shipping another
patch.

~Gregory