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@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- drivers/ptp/ptp_chardev.c | 18 ++++++++++++++++++ drivers/ptp/ptp_clock.c | 1 + drivers/ptp/ptp_private.h | 7 +++++++ include/linux/posix-clock.h | 3 +++ kernel/time/posix-clock.c | 4 ++-- 5 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index ea96a14d72d141a4b255563b66bac8ed568b45e9..704af620c1173c12ee26b3b6c7982693e3c4c21f 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -4,6 +4,7 @@ * * Copyright (C) 2010 OMICRON electronics GmbH */ +#include <linux/compat.h> #include <linux/module.h> #include <linux/posix-clock.h> #include <linux/poll.h> @@ -507,6 +508,23 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, return err; }
+#ifdef CONFIG_COMPAT +long ptp_compat_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, + unsigned long arg) +{ + switch (cmd) { + case PTP_ENABLE_PPS: + case PTP_ENABLE_PPS2: + /* These take in scalar arg, do not convert */ + break; + default: + arg = (unsigned long)compat_ptr(arg); + } + + return ptp_ioctl(pccontext, cmd, arg); +} +#endif + __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp, poll_table *wait) { diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 77a36e7bddd54e8f45eab317e687f033f57cc5bc..dec84b81cedfd13bcf8c97be6c3c27d73cd671f6 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -180,6 +180,7 @@ static struct posix_clock_operations ptp_clock_ops = { .clock_getres = ptp_clock_getres, .clock_settime = ptp_clock_settime, .ioctl = ptp_ioctl, + .compat_ioctl = ptp_compat_ioctl, .open = ptp_open, .release = ptp_release, .poll = ptp_poll, diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 18934e28469ee6e3bf9c9e6d1a1adb82808d88e6..13999a7af47a7b67ae8dc549351fbafef2ae402f 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -133,6 +133,13 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin, long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT +long ptp_compat_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, + unsigned long arg); +#else +#define ptp_compat_ioctl NULL +#endif + int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode);
int ptp_release(struct posix_clock_context *pccontext); diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h index ef8619f489203eeb369ae580fc4d4b2439c94ae9..8bdc7aec5f7979c42752a233077620818d15acdd 100644 --- a/include/linux/posix-clock.h +++ b/include/linux/posix-clock.h @@ -54,6 +54,9 @@ struct posix_clock_operations { long (*ioctl)(struct posix_clock_context *pccontext, unsigned int cmd, unsigned long arg);
+ long (*compat_ioctl)(struct posix_clock_context *pccontext, unsigned int cmd, + unsigned long arg); + int (*open)(struct posix_clock_context *pccontext, fmode_t f_mode);
__poll_t (*poll)(struct posix_clock_context *pccontext, struct file *file, diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 1af0bb2cc45c0aab843f77eb156992de469c8fb9..63184d92ef139c3fb9254745619ebca120fecce6 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -101,8 +101,8 @@ static long posix_clock_compat_ioctl(struct file *fp, if (!clk) return -ENODEV;
- if (clk->ops.ioctl) - err = clk->ops.ioctl(pccontext, cmd, arg); + if (clk->ops.compat_ioctl) + err = clk->ops.compat_ioctl(pccontext, cmd, arg);
put_posix_clock(clk);
--- base-commit: 0bc21e701a6ffacfdde7f04f87d664d82e8a13bf change-id: 20250103-posix-clock-compat_ioctl-96fbac549146
Best regards,
On Tue, Jan 21, 2025 at 11:41:24PM +0100, 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@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
Reviewed-by: Cyrill Gorcunov gorcunov@gmail.com
Thanks, Thomas!
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@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
This looks correct to me,
Reviewed-by: Arnd Bergmann arnd@arndb.de
+#ifdef CONFIG_COMPAT +long ptp_compat_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
unsigned long arg)+{
- switch (cmd) {
- case PTP_ENABLE_PPS:
- case PTP_ENABLE_PPS2:
/* These take in scalar arg, do not convert */break;
I was confused a bit here because the PTP_ENABLE_PPS and PTP_ENABLE_PPS2 definitions use _IOW(..., int), suggesting that the argument is passed through a pointer, when the code uses the 'arg' as a integer instead. Not your fault of course but it might help to explain this in the comment.
Arnd
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@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
This looks correct to me,
I'm not familiar with s390, but I can remember that the compat ioctl was nixed during review.
https://lore.kernel.org/lkml/201012161716.42520.arnd@arndb.de/
https://lore.kernel.org/lkml/alpine.LFD.2.00.1012161939340.12146@localhost6....
Can you explain what changed or what was missed?
Thanks, Richard
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@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.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....
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
On Wed, Jan 22, 2025, at 17:23, 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@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
This looks correct to me,
I'm not familiar with s390, but I can remember that the compat ioctl was nixed during review.
https://lore.kernel.org/lkml/201012161716.42520.arnd@arndb.de/
https://lore.kernel.org/lkml/alpine.LFD.2.00.1012161939340.12146@localhost6....
Can you explain what changed or what was missed?
The original review comment was about the complex argument transformation that is needed on architectures other than s390, which thankfully still works fine.
A lot of times we can ignore the s390 problem, and there are many drivers that still do, mainly because s390 has a very limited set of device drivers it actually uses, and also because 32-bit userspace is getting very rare on that architecture.
To do things correctly on all architectures, it's usually sufficient to just use compat_ptr_ioctl(), as in:
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 1af0bb2cc45c..e64d37f221b5 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -90,26 +90,6 @@ static long posix_clock_ioctl(struct file *fp, return err; }
-#ifdef CONFIG_COMPAT -static long posix_clock_compat_ioctl(struct file *fp, - unsigned int cmd, unsigned long arg) -{ - struct posix_clock_context *pccontext = fp->private_data; - struct posix_clock *clk = get_posix_clock(fp); - int err = -ENOTTY; - - if (!clk) - return -ENODEV; - - if (clk->ops.ioctl) - err = clk->ops.ioctl(pccontext, cmd, arg); - - put_posix_clock(clk); - - return err; -} -#endif - static int posix_clock_open(struct inode *inode, struct file *fp) { int err; @@ -173,9 +153,7 @@ static const struct file_operations posix_clock_file_operations = { .unlocked_ioctl = posix_clock_ioctl, .open = posix_clock_open, .release = posix_clock_release, -#ifdef CONFIG_COMPAT - .compat_ioctl = posix_clock_compat_ioctl, -#endif + .compat_ioctl = compat_ptr_ioctl, };
int posix_clock_register(struct posix_clock *clk, struct device *dev)
but this was only added in 2018 and was not available in your original version. Unfortunately this only works if 'arg' is always a pointer or a nonnegative integer less than 2^31. If the argument can be a negative integer, it's actually broken on all architectures because the current code performs a zero-extension when it should be doing a sign-extension.
A simpler variant of the patch would move the switch/case logic into posix_clock_compat_ioctl() and avoid the extra function pointer, simply calling posix_clock_ioctl() with the modified argument.
Arnd
On 2025-01-22 18:15:13+0100, Arnd Bergmann wrote:
A simpler variant of the patch would move the switch/case logic into posix_clock_compat_ioctl() and avoid the extra function pointer, simply calling posix_clock_ioctl() with the modified argument.
That would work, but be a layering violation. Or a "compat_mode" argument to ptp_ioctl()
I'm fine with either approach.
Thomas
On Thu, Jan 23, 2025, at 17:22, Thomas Weißschuh wrote:
On 2025-01-22 18:15:13+0100, Arnd Bergmann wrote:
A simpler variant of the patch would move the switch/case logic into posix_clock_compat_ioctl() and avoid the extra function pointer, simply calling posix_clock_ioctl() with the modified argument.
That would work, but be a layering violation. Or a "compat_mode" argument to ptp_ioctl()
I'm fine with either approach.
You don't even need to add an argument, just use in_compat_syscall() to check for compat mode inside of the ioctl handler. If CONFIG_COMPAT is disabled, that gets optimized out through dead-code-elimination.
Arnd
On Tue, Jan 21, 2025 at 11:41:24PM +0100, 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@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
...
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 77a36e7bddd54e8f45eab317e687f033f57cc5bc..dec84b81cedfd13bcf8c97be6c3c27d73cd671f6 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -180,6 +180,7 @@ static struct posix_clock_operations ptp_clock_ops = { .clock_getres = ptp_clock_getres, .clock_settime = ptp_clock_settime, .ioctl = ptp_ioctl,
- .compat_ioctl = ptp_compat_ioctl, .open = ptp_open, .release = ptp_release, .poll = ptp_poll,
nit: compat_ioctl should also be added to the Kernel doc for struct posix_clock_operations
...
On Tue, Jan 21, 2025 at 11:41:24PM +0100, 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.
PTP_ENABLE_PPS is either on of off, and the code tests whether the passed argument is zero or not.
So does this compat code actually fix an issue for s390?
Thanks, Richard
linux-stable-mirror@lists.linaro.org
-
Arnd Bergmann -
Cyrill Gorcunov -
Richard Cochran -
Simon Horman -
Thomas Weißschuh