On 2025-01-18, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> On 01/17, Dmitry V. Levin wrote:
> >
>
> (reordered)
>
> > struct ptrace_syscall_info has members of type __u64, and it currently
> > ends with "__u32 ret_data". So depending on the alignment, the structure
> > either has extra 4 trailing padding bytes, or it doesn't.
>
> Ah, I didn't realize that the last member is __u32, so I completely
> misunderstood your "it depends on the alignment of __u64" note.
>
> > For example, on x86_64 sizeof(struct ptrace_syscall_info) is currently 88,
> > while on x86 it is 84.
>
> Not good, but too late to complain...
>
> OK, I see your point now and I won't argue with approach you outlined in your
> previous email
>
> size_t min_size = offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
> size_t copy_size = min(sizeof(info), user_size);
>
> if (copy_size < min_size)
> return -EINVAL;
>
> if (copy_from_user(&info, datavp, copy_size))
> return -EFAULT;
>
> -------------------------------------------------------------------------------
> Thats said... Can't resist,
>
> > An absolutely artificial example: let's say we're adding an optional
> > 64-bit field "artificial" to ptrace_syscall_info.seccomp, this means
> > sizeof(ptrace_syscall_info) grows by 8 bytes. When userspace wants
> > to set this optional field, it sets a bit in ptrace_syscall_info.flags,
> > this tells the kernel to look into this new "artificial" field.
> > When userspace is not interested in setting new optional fields,
> > it just keeps ptrace_syscall_info.flags == 0. Remember, however, that
> > by adding the new optional field sizeof(ptrace_syscall_info) grew by 8 bytes.
> >
> > What we need is to make sure that an older kernel that has no idea of this
> > new field would still accept the bigger size, so that userspace would be
> > able to continue doing its
> > ptrace(PTRACE_SET_SYSCALL_INFO, pid, sizeof(info), &info)
> > despite of potential growth of sizeof(info) until it actually starts using
> > new optional fields.
>
> This is clear, but personally I don't really like this pattern... Consider
>
> void set_syscall_info(int unlikely_condition)
> {
> struct ptrace_syscall_info info;
>
> fill_info(&info);
> if (unlikely_condition) {
> info.flags = USE_ARTIFICIAL;
> info.artificial = 1;
> }
>
> assert(ptrace(PTRACE_SET_SYSCALL_INFO, sizeof(info), &info) == 0);
> }
>
> Now this application (running on the older kernel) can fail or not, depending
> on "unlikely_condition". To me it would be better to always fail in this case.
>
> That is why I tried to suggest to use "user_size" as a version number.
> Currently we have PTRACE_SYSCALL_INFO_SIZE_VER0, when we add the new
> "artificial" member we will have PTRACE_SYSCALL_INFO_SIZE_VER1. Granted,
> this way set_syscall_info() can't use sizeof(info), it should do
user_size *is* a version number, it's just that copy_struct_from_user()
allows programs built with newer headers to run on older kernels (if
they don't use the new features). The alternative is that programs that
build with a newer set of kernel headers will implicitly have a larger
ptrace_syscall_info struct, which will cause them to start failing after
the binary is rebuilt.
*Strictly speaking* this wouldn't be a kernel regression (because it's a
new binary, the old binary would still work), but the risk of these
kinds of APIs being incredibly fragile is the reason why I went with the
check_zeroed_user() approach in copy_struct_from_user().
(I haven't looked at the details of this patchset, this is just a
general comment about copy_struct_from_user() and why this feature is
useful to userspace programs. Not all APIs need the extensibility of
copy_struct_from_user().)
> ptrace(PTRACE_SET_SYSCALL_INFO, PTRACE_SYSCALL_INFO_SIZE_VER1, info);
>
> and the kernel needs more checks, but this is what I had in mind when I said
> that the 1st version can just require "user_size == PTRACE_SYSCALL_INFO_SIZE_VER0".
>
> But I won't insist, I do not pretend I understand the user-space needs.
>
> Thanks!
>
> Oleg.
>
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
Attachment:
signature.asc
Description: PGP signature
