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
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.
