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

[Oleg Nesterov]

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.