[PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request

[Aleksa Sarai]

On 2025-01-18, Oleg Nesterov <oleg at redhat.com> 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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20250120/ffcd9372/attachment.bin>