posix-clock: Explicitly handle compat ioctls

Commit Message

Thomas Weißschuh Jan. 21, 2025, 10:41 p.m. UTC

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: [e-mail]
Signed-off-by: Thomas Weißschuh <[e-mail]>
---
 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(-)


---
base-commit: 0bc21e701a6ffacfdde7f04f87d664d82e8a13bf
change-id: 20250103-posix-clock-compat_ioctl-96fbac549146

Best regards,

Comments

Cyrill Gorcunov Jan. 22, 2025, 6:59 a.m. UTC | #1

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: [e-mail]
> Signed-off-by: Thomas Weißschuh <[e-mail]>
Reviewed-by: Cyrill Gorcunov <[e-mail]>

Thanks, Thomas!

Arnd Bergmann Jan. 22, 2025, 7:30 a.m. UTC | #2

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: [e-mail]
> Signed-off-by: Thomas Weißschuh <[e-mail]>

This looks correct to me,

Reviewed-by: Arnd Bergmann <[e-mail]>

> +#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

Simon Horman Jan. 22, 2025, 9:52 a.m. UTC | #3

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: [e-mail]
> Signed-off-by: Thomas Weißschuh <[e-mail]>

...

> 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

...

Richard Cochran Jan. 22, 2025, 4:23 p.m. UTC | #4

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: [e-mail]
> > Signed-off-by: Thomas Weißschuh <[e-mail]>
> 
> 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/[e-mail]/

   https://lore.kernel.org/lkml/[e-mail]6/

Can you explain what changed or what was missed?

Thanks,
Richard

Richard Cochran Jan. 22, 2025, 4:39 p.m. UTC | #5

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

Thomas Weißschuh Jan. 22, 2025, 4:48 p.m. UTC | #6

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: [e-mail]
> > > Signed-off-by: Thomas Weißschuh <[e-mail]>
> > 
> > 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/[e-mail]/
> 
>    https://lore.kernel.org/lkml/[e-mail]6/
> 
> 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

Arnd Bergmann Jan. 22, 2025, 5:15 p.m. UTC | #7

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: [e-mail]
>> > Signed-off-by: Thomas Weißschuh <[e-mail]>
>> 
>> 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/[e-mail]/
>
>    
> https://lore.kernel.org/lkml/[e-mail]6/
>
> 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

Patch

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);