linux-kernel - Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250117150627.GA15109@strace.io>
Date: Fri, 17 Jan 2025 17:06:27 +0200
From: "Dmitry V. Levin" <ldv@...ace.io>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Eugene Syromyatnikov <evgsyr@...il.com>,
	Mike Frysinger <vapier@...too.org>,
	Renzo Davoli <renzo@...unibo.it>,
	Davide Berardi <berardi.dav@...il.com>,
	strace-devel@...ts.strace.io, linux-kernel@...r.kernel.org,
	linux-api@...r.kernel.org
Subject: Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request

On Fri, Jan 17, 2025 at 03:45:57PM +0100, Oleg Nesterov wrote:
> On 01/16, Dmitry V. Levin wrote:
> >
> > The idea is to use "op" to specify the operation, and "flags" to specify
> > future extensions to the operation.
> 
> OK,
> 
> > That is, the zero check implied by copy_struct_from_user() is not really
> > needed here since the compatibility is tracked by "op" and "flags":
> 
> OK, but then why this patch uses copy_struct_from_user() ?
> 
> Why can't we simply do
> 
> 	if (user_size != PTRACE_SYSCALL_INFO_SIZE_VER0)
> 		return -EINVAL;
> 
> 	if (copy_from_user(..., user_size))
> 		return EFAULT;
> 
> now, until we add the extensions ?

We should accept larger user_size from the very beginning, so that in case
the structure grows in the future, the userspace that sicks to the current
set of supported features would be still able to work with older kernels.

I think we can do the following:

        /*
         * ptrace_syscall_info.seccomp is the largest member in the union,
         * and ret_data is the last field there.
         * min_size can be less than sizeof(info) due to alignment.
         */
        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;

We cannot just use sizeof(info) because it depends on the alignment of
__u64.  Also, I don't think we need to fill with zeroes the trailing
padding bytes of the structure as we are not going to use them in any way.


-- 
ldv