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
