linux-kernel - Re: [PATCH] posix-clock: Explicitly handle compat ioctls

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c7abaaf0-3bf4-4c49-b4b2-c0d9914c2dcc@t-8ch.de>
Date: Wed, 22 Jan 2025 17:48:40 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Richard Cochran <richardcochran@...il.com>
Cc: Arnd Bergmann <arnd@...db.de>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Anna-Maria Gleixner <anna-maria@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>, 
	Thomas Gleixner <tglx@...utronix.de>, John Stultz <johnstul@...ibm.com>, 
	Netdev <netdev@...r.kernel.org>, linux-kernel@...r.kernel.org, 
	Cyrill Gorcunov <gorcunov@...il.com>, stable@...r.kernel.org
Subject: Re: [PATCH] posix-clock: Explicitly handle compat ioctls

On 2025-01-22 08:23:35-0800, Richard Cochran wrote:
> On Wed, Jan 22, 2025 at 08:30:51AM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 21, 2025, at 23:41, Thomas Weißschuh wrote:
> > > Pointer arguments passed to ioctls need to pass through compat_ptr() to
> > > work correctly on s390; as explained in Documentation/driver-api/ioctl.rst.
> > > Plumb the compat_ioctl callback through 'struct posix_clock_operations'
> > > and handle the different ioctls cmds in the new ptp_compat_ioctl().
> > >
> > > Using compat_ptr_ioctl is not possible.
> > > For the commands PTP_ENABLE_PPS/PTP_ENABLE_PPS2 on s390
> > > it would corrupt the argument 0x80000000, aka BIT(31) to zero.
> > >
> > > Fixes: 0606f422b453 ("posix clocks: Introduce dynamic clocks")
> > > Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
> > > Cc: stable@...r.kernel.org
> > > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > 
> > This looks correct to me,
> 
> I'm not familiar with s390, but I can remember that the compat ioctl
> was nixed during review.

>From Documentation/driver-api/ioctl.rst:

	compat_ptr()
	------------

	On the s390 architecture, 31-bit user space has ambiguous representations
	for data pointers, with the upper bit being ignored. When running such
	a process in compat mode, the compat_ptr() helper must be used to
	clear the upper bit of a compat_uptr_t and turn it into a valid 64-bit
	pointer.  On other architectures, this macro only performs a cast to a
	``void __user *`` pointer.

	In an compat_ioctl() callback, the last argument is an unsigned long,
	which can be interpreted as either a pointer or a scalar depending on
	the command. If it is a scalar, then compat_ptr() must not be used, to
	ensure that the 64-bit kernel behaves the same way as a 32-bit kernel
	for arguments with the upper bit set.

	The compat_ptr_ioctl() helper can be used in place of a custom
	compat_ioctl file operation for drivers that only take arguments that
	are pointers to compatible data structures.

TLDR:

If the argument is a pointer, pass it through compat_ptr().
If the argument is a scalar, *do not* pass it through compat_ptr().

If all ioctls handled by a struct file_operations::unlocked_ioctl are
pointers, use .compat_ioctl = compat_ptr_ioctl.
If all ioctls handled by a struct file_operations::unlocked_ioctl are
scalars, use .compat_ioctl = .unlocked_ioctl.
If it's mixed, add a custom handler that knows the details, like this
patch does.

This all assumes the actual data structures are compatible between
compat and non-compat, which is the case here.
If they are not compatible those also need to be converted around.

>    https://lore.kernel.org/lkml/201012161716.42520.arnd@arndb.de/
> 
>    https://lore.kernel.org/lkml/alpine.LFD.2.00.1012161939340.12146@localhost6.localdomain6/
> 
> Can you explain what changed or what was missed?

>From your link:

	* I would recommend starting without a compat_ioctl operation if you can.
	Just mandate that all ioctls for posix clocks are compatible and call
	fops->ioctl from the posix_clock_compat_ioctl() function.
	If you ever need compat_ioctl handling, it can still be added later.

The fact that compat_ptr() is necessary for pointer arguments was missed.
And because there are two commands with scalar arguments,
compat_ptr_ioctl() can't be used.


Thomas