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

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.