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

[Gregory Price <gregory.price@memverge.com>]

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