linux-kernel - Re: [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20241211-gemsen-zuarbeiten-ae8d062ec251@brauner>
Date: Wed, 11 Dec 2024 11:23:43 +0100
From: Christian Brauner <brauner@...nel.org>
To: Florian Weimer <fweimer@...hat.com>, Aleksa Sarai <cyphar@...har.com>, 
	Ingo Molnar <mingo@...hat.com>
Cc: Peter Zijlstra <peterz@...radead.org>, 
	Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt <rostedt@...dmis.org>, 
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Valentin Schneider <vschneid@...hat.com>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Jan Kara <jack@...e.cz>, Arnd Bergmann <arnd@...db.de>, Shuah Khan <shuah@...nel.org>, 
	Kees Cook <kees@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-arch@...r.kernel.org, linux-kselftest@...r.kernel.org, libc-alpha@...rceware.org
Subject: Re: [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to_user

On Tue, Dec 10, 2024 at 07:14:07PM +0100, Florian Weimer wrote:
> * Aleksa Sarai:
> 
> > sched_getattr(2) doesn't care about trailing non-zero bytes in the
> > (ksize > usize) case, so just use copy_struct_to_user() without checking
> > ignored_trailing.
> 
> I think this is what causes glibc's misc/tst-sched_setattr test to fail
> on recent kernels.  The previous non-modifying behavior was documented
> in the manual page:
> 
>        If the caller-provided attr buffer is larger than the kernel's
>        sched_attr structure, the additional bytes in the user-space
>        structure are not touched.
> 
> I can just drop this part of the test if the kernel deems both behaviors
> valid.

I think in general both behaviors are valid but I would consider zeroing
the unknown parts of the provided buffer to be the safer option. And all
newer extensible struct system calls do that.

But if sched_getattr(2) wants to keep its old behavior it wouldn't be a
problem to just handle this case:

diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
index 0d71fcbaf1e3..46140ec449ba 100644
--- a/kernel/sched/syscalls.c
+++ b/kernel/sched/syscalls.c
@@ -1126,6 +1126,15 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
        }

        kattr.size = min(usize, sizeof(kattr));
+       /*
+        * If userspace passed a larger structure than the kernel knows
+        * we historically didn't zero the unknown bits but
+        * copy_struct_to_user() will. Retain the old behavior by
+        * limiting the copy_to_user() to the size the kernel knows
+        * about.
+        */
+       if (usize > sizeof(kattr))
+               usize = sizeof(kattr);
        return copy_struct_to_user(uattr, usize, &kattr, sizeof(kattr), NULL);
 }