[PATCH 1/8] y2038: timex: remove incorrect time_t truncation

[PATCH 1/8] y2038: timex: remove incorrect time_t truncation

[Arnd Bergmann]

A cast to 'time_t' was accidentally left in place during the
conversion of __do_adjtimex() to 64-bit timestamps, so the
resulting value is incorrectly truncated.

Remove the cast so the 64-bit time gets propagated correctly.

Cc: stable@vger.kernel.org
Fixes: ead25417f82e ("timex: use __kernel_timex internally")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 kernel/time/ntp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 65eb796610dc..069ca78fb0bf 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -771,7 +771,7 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
 	/* fill PPS status fields */
 	pps_fill_timex(txc);
 
-	txc->time.tv_sec = (time_t)ts->tv_sec;
+	txc->time.tv_sec = ts->tv_sec;
 	txc->time.tv_usec = ts->tv_nsec;
 	if (!(time_status & STA_NANO))
 		txc->time.tv_usec = ts->tv_nsec / NSEC_PER_USEC;
-- 
2.20.0

[PATCH 6/8] lp: fix sparc64 LPSETTIMEOUT ioctl

[Arnd Bergmann]

The layout of struct timeval is different on sparc64 from
anything else, and the patch I did long ago failed to take
this into account.

Change it now to handle sparc64 user space correctly again.

Quite likely nobody cares about parallel ports on sparc64,
but there is no reason not to fix it.

Cc: stable@vger.kernel.org
Fixes: 9a450484089d ("lp: support 64-bit time_t user space")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/lp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index 7c9269e3477a..bd95aba1f9fe 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg)
 	if (copy_from_user(karg, arg, sizeof(karg)))
 		return -EFAULT;
 
+	/* sparc64 suseconds_t is 32-bit only */
+	if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
+		karg[1] >>= 32;
+
 	return lp_set_timeout(minor, karg[0], karg[1]);
 }
 
-- 
2.20.0

[PATCH 7/8] ppdev: fix PPGETTIME/PPSETTIME ioctls

[Arnd Bergmann]

Going through the uses of timeval in the user space API,
I noticed two bugs in ppdev that were introduced in the y2038
conversion:

* The range check was accidentally moved from ppsettime to
  ppgettime

* On sparc64, the microseconds are in the other half of the
  64-bit word.

Fix both, and mark the fix for stable backports.

Cc: stable@vger.kernel.org
Fixes: 3b9ab374a1e6 ("ppdev: convert to y2038 safe")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/ppdev.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index c86f18aa8985..34bb88fe0b0a 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -619,20 +619,27 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(time32, argp, sizeof(time32)))
 			return -EFAULT;
 
+		if ((time32[0] < 0) || (time32[1] < 0))
+			return -EINVAL;
+
 		return pp_set_timeout(pp->pdev, time32[0], time32[1]);
 
 	case PPSETTIME64:
 		if (copy_from_user(time64, argp, sizeof(time64)))
 			return -EFAULT;
 
+		if ((time64[0] < 0) || (time64[1] < 0))
+			return -EINVAL;
+
+		if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
+			time64[1] >>= 32;
+
 		return pp_set_timeout(pp->pdev, time64[0], time64[1]);
 
 	case PPGETTIME32:
 		jiffies_to_timespec64(pp->pdev->timeout, &ts);
 		time32[0] = ts.tv_sec;
 		time32[1] = ts.tv_nsec / NSEC_PER_USEC;
-		if ((time32[0] < 0) || (time32[1] < 0))
-			return -EINVAL;
 
 		if (copy_to_user(argp, time32, sizeof(time32)))
 			return -EFAULT;
@@ -643,8 +650,9 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		jiffies_to_timespec64(pp->pdev->timeout, &ts);
 		time64[0] = ts.tv_sec;
 		time64[1] = ts.tv_nsec / NSEC_PER_USEC;
-		if ((time64[0] < 0) || (time64[1] < 0))
-			return -EINVAL;
+
+		if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
+			time64[1] <<= 32;
 
 		if (copy_to_user(argp, time64, sizeof(time64)))
 			return -EFAULT;
-- 
2.20.0

[PATCH 8/8] Input: input_event: fix struct padding on sparc64

[Arnd Bergmann]

Going through all uses of timeval, I noticed that we screwed up
input_event in the previous attempts to fix it:

The time fields now match between kernel and user space, but
all following fields are in the wrong place.

Add the required padding that is implied by the glibc timeval
definition to fix the layout, and add explicit initialization
to avoid leaking kernel stack data.

Cc: sparclinux@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: stable@vger.kernel.org
Fixes: 141e5dcaa735 ("Input: input_event - fix the CONFIG_SPARC64 mixup")
Fixes: 2e746942ebac ("Input: input_event - provide override for sparc64")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/input/evdev.c       | 3 +++
 drivers/input/misc/uinput.c | 3 +++
 include/uapi/linux/input.h  | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d7dd6fcf2db0..24a90793caf0 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -228,6 +228,9 @@ static void __pass_event(struct evdev_client *client,
 						event->input_event_sec;
 		client->buffer[client->tail].input_event_usec =
 						event->input_event_usec;
+#ifdef CONFIG_SPARC64
+		client->buffer[client->tail].__pad = 0;
+#endif
 		client->buffer[client->tail].type = EV_SYN;
 		client->buffer[client->tail].code = SYN_DROPPED;
 		client->buffer[client->tail].value = 0;
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 84051f20b18a..1d8c09e9fd47 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -80,6 +80,9 @@ static int uinput_dev_event(struct input_dev *dev,
 	ktime_get_ts64(&ts);
 	udev->buff[udev->head].input_event_sec = ts.tv_sec;
 	udev->buff[udev->head].input_event_usec = ts.tv_nsec / NSEC_PER_USEC;
+#ifdef CONFIG_SPARC64
+	udev->buff[udev->head].__pad = 0;
+#endif
 	udev->head = (udev->head + 1) % UINPUT_BUFFER_SIZE;
 
 	wake_up_interruptible(&udev->waitq);
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index f056b2a00d5c..9a61c28ed3ae 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -34,6 +34,7 @@ struct input_event {
 	__kernel_ulong_t __sec;
 #if defined(__sparc__) && defined(__arch64__)
 	unsigned int __usec;
+	unsigned int __pad;
 #else
 	__kernel_ulong_t __usec;
 #endif
-- 
2.20.0

Re: [PATCH 1/8] y2038: timex: remove incorrect time_t truncation

[Deepa Dinamani]

Thanks for fixing the bug.

Acked-by: Deepa Dinamani <deepa.kernel@gmail.com>

Re: [PATCH 8/8] Input: input_event: fix struct padding on sparc64

[Dmitry Torokhov]

Hi Arnd,

On Fri, Nov 08, 2019 at 09:34:31PM +0100, Arnd Bergmann wrote:
> Going through all uses of timeval, I noticed that we screwed up
> input_event in the previous attempts to fix it:
> 
> The time fields now match between kernel and user space, but
> all following fields are in the wrong place.
> 
> Add the required padding that is implied by the glibc timeval
> definition to fix the layout, and add explicit initialization
> to avoid leaking kernel stack data.
> 
> Cc: sparclinux@vger.kernel.org
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: stable@vger.kernel.org
> Fixes: 141e5dcaa735 ("Input: input_event - fix the CONFIG_SPARC64 mixup")
> Fixes: 2e746942ebac ("Input: input_event - provide override for sparc64")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/input/evdev.c       | 3 +++
>  drivers/input/misc/uinput.c | 3 +++
>  include/uapi/linux/input.h  | 1 +
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index d7dd6fcf2db0..24a90793caf0 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -228,6 +228,9 @@ static void __pass_event(struct evdev_client *client,
>  						event->input_event_sec;
>  		client->buffer[client->tail].input_event_usec =
>  						event->input_event_usec;
> +#ifdef CONFIG_SPARC64
> +		client->buffer[client->tail].__pad = 0;
> +#endif
>  		client->buffer[client->tail].type = EV_SYN;
>  		client->buffer[client->tail].code = SYN_DROPPED;
>  		client->buffer[client->tail].value = 0;

I do not like ifdefs here, do you think we could write:

		client->buffer[client->tail] = (struct input_event) {
			.input_event_sec = event->input_event_sec,
			.input_event_usec = event->input_event_usec,
			.type = EV_SYN,
			.code = SYN_DROPPED,
		};

to ensure all padded fields are initialized? This is not hot path as we
do not expect queue to overfill too often.

Thanks.

-- 
Dmitry

Re: [PATCH 8/8] Input: input_event: fix struct padding on sparc64

[Arnd Bergmann]

On Mon, Nov 11, 2019 at 7:28 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> I do not like ifdefs here, do you think we could write:
>
>                 client->buffer[client->tail] = (struct input_event) {
>                         .input_event_sec = event->input_event_sec,
>                         .input_event_usec = event->input_event_usec,
>                         .type = EV_SYN,
>                         .code = SYN_DROPPED,
>                 };
>
> to ensure all padded fields are initialized? This is not hot path as we
> do not expect queue to overfill too often.

Good idea, changed both instances now. Thanks for taking a look!

      Arnd

[tip: timers/urgent] ntp/y2038: Remove incorrect time_t truncation

[tip-bot2 for Arnd Bergmann]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     2f5841349df281ecf8f81cc82d869b8476f0db0b
Gitweb:        https://git.kernel.org/tip/2f5841349df281ecf8f81cc82d869b8476f0db0b
Author:        Arnd Bergmann <arnd@arndb.de>
AuthorDate:    Fri, 08 Nov 2019 21:34:24 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 12 Nov 2019 08:13:44 +01:00

ntp/y2038: Remove incorrect time_t truncation

A cast to 'time_t' was accidentally left in place during the
conversion of __do_adjtimex() to 64-bit timestamps, so the
resulting value is incorrectly truncated.

Remove the cast so the 64-bit time gets propagated correctly.

Fixes: ead25417f82e ("timex: use __kernel_timex internally")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20191108203435.112759-2-arnd@arndb.de

---
 kernel/time/ntp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 65eb796..069ca78 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -771,7 +771,7 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
 	/* fill PPS status fields */
 	pps_fill_timex(txc);
 
-	txc->time.tv_sec = (time_t)ts->tv_sec;
+	txc->time.tv_sec = ts->tv_sec;
 	txc->time.tv_usec = ts->tv_nsec;
 	if (!(time_status & STA_NANO))
 		txc->time.tv_usec = ts->tv_nsec / NSEC_PER_USEC;

Re: [Y2038] [PATCH 6/8] lp: fix sparc64 LPSETTIMEOUT ioctl

[Ben Hutchings]

On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
> The layout of struct timeval is different on sparc64 from
> anything else, and the patch I did long ago failed to take
> this into account.
> 
> Change it now to handle sparc64 user space correctly again.
> 
> Quite likely nobody cares about parallel ports on sparc64,
> but there is no reason not to fix it.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9a450484089d ("lp: support 64-bit time_t user space")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/char/lp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 7c9269e3477a..bd95aba1f9fe 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg)
>  	if (copy_from_user(karg, arg, sizeof(karg)))
>  		return -EFAULT;
>  
> +	/* sparc64 suseconds_t is 32-bit only */
> +	if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> +		karg[1] >>= 32;
> +
>  	return lp_set_timeout(minor, karg[0], karg[1]);
>  }
>

It seems like it would make way more sense to use __kernel_old_timeval.
Then you don't have to explicitly handle the sparc64 oddity.

As it is, this still over-reads from user-space which might result in a
spurious -EFAULT.

Ben.

-- 
Ben Hutchings, Software Developer                         Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom

Re: [Y2038] [PATCH 7/8] ppdev: fix PPGETTIME/PPSETTIME ioctls

[Ben Hutchings]

On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
> Going through the uses of timeval in the user space API,
> I noticed two bugs in ppdev that were introduced in the y2038
> conversion:
> 
> * The range check was accidentally moved from ppsettime to
>   ppgettime
> 
> * On sparc64, the microseconds are in the other half of the
>   64-bit word.
> 
> Fix both, and mark the fix for stable backports.

Like the patch for lpdev, this also doesn't completely fix sparc64.

Ben.

> Cc: stable@vger.kernel.org
> Fixes: 3b9ab374a1e6 ("ppdev: convert to y2038 safe")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/char/ppdev.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index c86f18aa8985..34bb88fe0b0a 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -619,20 +619,27 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(time32, argp, sizeof(time32)))
>  			return -EFAULT;
>  
> +		if ((time32[0] < 0) || (time32[1] < 0))
> +			return -EINVAL;
> +
>  		return pp_set_timeout(pp->pdev, time32[0], time32[1]);
>  
>  	case PPSETTIME64:
>  		if (copy_from_user(time64, argp, sizeof(time64)))
>  			return -EFAULT;
>  
> +		if ((time64[0] < 0) || (time64[1] < 0))
> +			return -EINVAL;
> +
> +		if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> +			time64[1] >>= 32;
> +
>  		return pp_set_timeout(pp->pdev, time64[0], time64[1]);
>  
>  	case PPGETTIME32:
>  		jiffies_to_timespec64(pp->pdev->timeout, &ts);
>  		time32[0] = ts.tv_sec;
>  		time32[1] = ts.tv_nsec / NSEC_PER_USEC;
> -		if ((time32[0] < 0) || (time32[1] < 0))
> -			return -EINVAL;
>  
>  		if (copy_to_user(argp, time32, sizeof(time32)))
>  			return -EFAULT;
> @@ -643,8 +650,9 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		jiffies_to_timespec64(pp->pdev->timeout, &ts);
>  		time64[0] = ts.tv_sec;
>  		time64[1] = ts.tv_nsec / NSEC_PER_USEC;
> -		if ((time64[0] < 0) || (time64[1] < 0))
> -			return -EINVAL;
> +
> +		if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> +			time64[1] <<= 32;
>  
>  		if (copy_to_user(argp, time64, sizeof(time64)))
>  			return -EFAULT;
-- 
Ben Hutchings, Software Developer                         Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom

Re: [Y2038] [PATCH 6/8] lp: fix sparc64 LPSETTIMEOUT ioctl

[Arnd Bergmann]

On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
>
> On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
> > The layout of struct timeval is different on sparc64 from
> > anything else, and the patch I did long ago failed to take
> > this into account.
> >
> > Change it now to handle sparc64 user space correctly again.
> >
> > Quite likely nobody cares about parallel ports on sparc64,
> > but there is no reason not to fix it.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 9a450484089d ("lp: support 64-bit time_t user space")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/char/lp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> > index 7c9269e3477a..bd95aba1f9fe 100644
> > --- a/drivers/char/lp.c
> > +++ b/drivers/char/lp.c
> > @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg)
> >       if (copy_from_user(karg, arg, sizeof(karg)))
> >               return -EFAULT;
> >
> > +     /* sparc64 suseconds_t is 32-bit only */
> > +     if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> > +             karg[1] >>= 32;
> > +
> >       return lp_set_timeout(minor, karg[0], karg[1]);
> >  }
> >
>
> It seems like it would make way more sense to use __kernel_old_timeval.

Right, that would work. I tried to keep the patch small here, changing
it to __kernel_old_timeval would require make it all more complicated
since it would still need to check some conditional to tell the difference
between sparc32 and sparc64.

I think this patch (relative to the version I posted) would work the same:

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index bd95aba1f9fe..86994421ee97 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -713,13 +713,19 @@ static int lp_set_timeout64(unsigned int minor,
void __user *arg)
        if (copy_from_user(karg, arg, sizeof(karg)))
                return -EFAULT;

-       /* sparc64 suseconds_t is 32-bit only */
-       if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
-               karg[1] >>= 32;
-
        return lp_set_timeout(minor, karg[0], karg[1]);
 }

+static int lp_set_timeout(unsigned int minor, void __user *arg)
+{
+       __kernel_old_timeval tv;
+
+       if (copy_from_user(tv, arg, sizeof(karg)))
+               return -EFAULT;
+
+       return lp_set_timeout(minor, tv->tv_sec, tv->tv_usec);
+}
+
 static long lp_ioctl(struct file *file, unsigned int cmd,
                        unsigned long arg)
 {
@@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
        mutex_lock(&lp_mutex);
        switch (cmd) {
        case LPSETTIMEOUT_OLD:
-               if (BITS_PER_LONG == 32) {
-                       ret = lp_set_timeout32(minor, (void __user *)arg);
-                       break;
-               }
-               /* fall through - for 64-bit */
+               ret = lp_set_timeout(minor, (void __user *)arg);
+               break;
        case LPSETTIMEOUT_NEW:
                ret = lp_set_timeout64(minor, (void __user *)arg);
                break;

Do you like that better? One difference here is the handling of
LPSETTIMEOUT_NEW on sparc64, which would continue to use
the 64/64 layout rather than the 64/32/pad layout, but that should
be ok, since sparc64 user space using ppdev (if any exists)
would use LPSETTIMEOUT_OLD, not LPSETTIMEOUT_NEW.

> Then you don't have to explicitly handle the sparc64 oddity.
>
> As it is, this still over-reads from user-space which might result in a
> spurious -EFAULT.

I think you got this wrong: sparc64 like most architectures naturally
aligns 64-bit members, so 'struct timeval' still uses 16 bytes including
the four padding bytes at the end, it just has the nanoseconds in
a different position from all other big-endian architectures.

      Arnd

Re: [Y2038] [PATCH 6/8] lp: fix sparc64 LPSETTIMEOUT ioctl

[Ben Hutchings]

On Wed, 2019-11-20 at 20:46 +0100, Arnd Bergmann wrote:
> On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings
> <ben.hutchings@codethink.co.uk> wrote:
> > On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
> > > The layout of struct timeval is different on sparc64 from
> > > anything else, and the patch I did long ago failed to take
> > > this into account.
> > > 
> > > Change it now to handle sparc64 user space correctly again.
> > > 
> > > Quite likely nobody cares about parallel ports on sparc64,
> > > but there is no reason not to fix it.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: 9a450484089d ("lp: support 64-bit time_t user space")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > >  drivers/char/lp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> > > index 7c9269e3477a..bd95aba1f9fe 100644
> > > --- a/drivers/char/lp.c
> > > +++ b/drivers/char/lp.c
> > > @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg)
> > >       if (copy_from_user(karg, arg, sizeof(karg)))
> > >               return -EFAULT;
> > > 
> > > +     /* sparc64 suseconds_t is 32-bit only */
> > > +     if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> > > +             karg[1] >>= 32;
> > > +
> > >       return lp_set_timeout(minor, karg[0], karg[1]);
> > >  }
> > > 
> > 
> > It seems like it would make way more sense to use __kernel_old_timeval.
> 
> Right, that would work. I tried to keep the patch small here, changing
> it to __kernel_old_timeval would require make it all more complicated
> since it would still need to check some conditional to tell the difference
> between sparc32 and sparc64.

Right.

> I think this patch (relative to the version I posted) would work the same:
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index bd95aba1f9fe..86994421ee97 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -713,13 +713,19 @@ static int lp_set_timeout64(unsigned int minor,
> void __user *arg)
>         if (copy_from_user(karg, arg, sizeof(karg)))
>                 return -EFAULT;
> 
> -       /* sparc64 suseconds_t is 32-bit only */
> -       if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> -               karg[1] >>= 32;
> -
>         return lp_set_timeout(minor, karg[0], karg[1]);
>  }
> 
> +static int lp_set_timeout(unsigned int minor, void __user *arg)

That function name is already used!  Maybe this should be
lp_set_timeout_old()?

> +{
> +       __kernel_old_timeval tv;
> +
> +       if (copy_from_user(tv, arg, sizeof(karg)))
> +               return -EFAULT;
> +
> +       return lp_set_timeout(minor, tv->tv_sec, tv->tv_usec);
> +}
> +
>  static long lp_ioctl(struct file *file, unsigned int cmd,
>                         unsigned long arg)
>  {
> @@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
>         mutex_lock(&lp_mutex);
>         switch (cmd) {
>         case LPSETTIMEOUT_OLD:
> -               if (BITS_PER_LONG == 32) {
> -                       ret = lp_set_timeout32(minor, (void __user *)arg);
> -                       break;
> -               }
> -               /* fall through - for 64-bit */
> +               ret = lp_set_timeout(minor, (void __user *)arg);
> +               break;
>         case LPSETTIMEOUT_NEW:
>                 ret = lp_set_timeout64(minor, (void __user *)arg);
>                 break;
> 
> Do you like that better?

Yes.  Aside from the duplicate function name, it looks correct and
cleaner than the current version.

> One difference here is the handling of
> LPSETTIMEOUT_NEW on sparc64, which would continue to use
> the 64/64 layout rather than the 64/32/pad layout, but that should
> be ok, since sparc64 user space using ppdev (if any exists)
> would use LPSETTIMEOUT_OLD, not LPSETTIMEOUT_NEW.

Right, that's a little weird but appears to be consistent with "new"
socket timestamps.

> > Then you don't have to explicitly handle the sparc64 oddity.
> > 
> > As it is, this still over-reads from user-space which might result in a
> > spurious -EFAULT.
> 
> I think you got this wrong: sparc64 like most architectures naturally
> aligns 64-bit members, so 'struct timeval' still uses 16 bytes including
> the four padding bytes at the end, it just has the nanoseconds in
> a different position from all other big-endian architectures.

Oh of course, yes.

Ben.

-- 
Ben Hutchings, Software Developer                         Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom

Re: [Y2038] [PATCH 6/8] lp: fix sparc64 LPSETTIMEOUT ioctl

[Arnd Bergmann]

On Wed, Nov 20, 2019 at 11:10 PM Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> On Wed, 2019-11-20 at 20:46 +0100, Arnd Bergmann wrote:
> > On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings <ben.hutchings@codethink.co.uk> wrote:
> > -
> >         return lp_set_timeout(minor, karg[0], karg[1]);
> >  }
> >
> > +static int lp_set_timeout(unsigned int minor, void __user *arg)
>
> That function name is already used!  Maybe this should be
> lp_set_timeout_old()?

Yes, that's what I used after actually compile-testing and running
into a couple of issues with my draft.

> > @@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
> >         mutex_lock(&lp_mutex);
> >         switch (cmd) {
> >         case LPSETTIMEOUT_OLD:
> > -               if (BITS_PER_LONG == 32) {
> > -                       ret = lp_set_timeout32(minor, (void __user *)arg);
> > -                       break;
> > -               }
> > -               /* fall through - for 64-bit */
> > +               ret = lp_set_timeout(minor, (void __user *)arg);
> > +               break;
> >         case LPSETTIMEOUT_NEW:
> >                 ret = lp_set_timeout64(minor, (void __user *)arg);
> >                 break;
> >
> > Do you like that better?
>
> Yes.  Aside from the duplicate function name, it looks correct and
> cleaner than the current version.

As Greg has already merged the original patch, and that version works
just as well, I'd probably just leave what I did at first. One benefit is
that in case we decide to kill off sparc64 support before drivers/char/lp.c,
the special case can be removed more easily.

I don't think either of them is going any time soon, but working on y2038
patches has made me think ahead longer term ;-)

If you still think we should change it I can send the below patch (now
actually build-tested) with your Ack.

       Arnd
---
commit 93efbb1768a5071a0a98bf4627f0104075cf83a6 (HEAD -> y2038)
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Thu Nov 21 14:45:14 2019 +0100

    lp: clean up set_timeout handling

    As Ben Hutchings noticed, we can avoid the special case for sparc64
    by dealing with '__kernel_old_timeval' arguments separately from
    the fixed-length 32-bit and 64-bit arguments.

    Note that the behavior for LPSETTIMEOUT_NEW changes on sparc64 to
    expect the same argument as other architectures, but this is ok
    because sparc64 users would pass LPSETTIMEOUT_OLD anyway.

    Suggested-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index bd95aba1f9fe..cc17d5a387c5 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -696,14 +696,14 @@ static int lp_set_timeout(unsigned int minor,
s64 tv_sec, long tv_usec)
        return 0;
 }

-static int lp_set_timeout32(unsigned int minor, void __user *arg)
+static int lp_set_timeout_old(unsigned int minor, void __user *arg)
 {
-       s32 karg[2];
+       struct __kernel_old_timeval tv;

-       if (copy_from_user(karg, arg, sizeof(karg)))
+       if (copy_from_user(&tv, arg, sizeof(tv)))
                return -EFAULT;

-       return lp_set_timeout(minor, karg[0], karg[1]);
+       return lp_set_timeout(minor, tv.tv_sec, tv.tv_usec);
 }

 static int lp_set_timeout64(unsigned int minor, void __user *arg)
@@ -713,10 +713,6 @@ static int lp_set_timeout64(unsigned int minor,
void __user *arg)
        if (copy_from_user(karg, arg, sizeof(karg)))
                return -EFAULT;

-       /* sparc64 suseconds_t is 32-bit only */
-       if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
-               karg[1] >>= 32;
-
        return lp_set_timeout(minor, karg[0], karg[1]);
 }

@@ -730,11 +726,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
        mutex_lock(&lp_mutex);
        switch (cmd) {
        case LPSETTIMEOUT_OLD:
-               if (BITS_PER_LONG == 32) {
-                       ret = lp_set_timeout32(minor, (void __user *)arg);
-                       break;
-               }
-               /* fall through - for 64-bit */
+               ret = lp_set_timeout_old(minor, (void __user *)arg);
+               break;
        case LPSETTIMEOUT_NEW:
                ret = lp_set_timeout64(minor, (void __user *)arg);
                break;
@@ -748,6 +741,16 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
 }

 #ifdef CONFIG_COMPAT
+static int lp_set_timeout32(unsigned int minor, void __user *arg)
+{
+       s32 karg[2];
+
+       if (copy_from_user(karg, arg, sizeof(karg)))
+               return -EFAULT;
+
+       return lp_set_timeout(minor, karg[0], karg[1]);
+}
+
 static long lp_compat_ioctl(struct file *file, unsigned int cmd,
                        unsigned long arg)
 {

Re: [Y2038] [PATCH 7/8] ppdev: fix PPGETTIME/PPSETTIME ioctls

[Arnd Bergmann]

On Wed, Nov 20, 2019 at 8:29 PM Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
>
> On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
> > Going through the uses of timeval in the user space API,
> > I noticed two bugs in ppdev that were introduced in the y2038
> > conversion:
> >
> > * The range check was accidentally moved from ppsettime to
> >   ppgettime
> >
> > * On sparc64, the microseconds are in the other half of the
> >   64-bit word.
> >
> > Fix both, and mark the fix for stable backports.
>
> Like the patch for lpdev, this also doesn't completely fix sparc64.

I think the same applies as in the other patch:

- it actually works correctly because of the alignment
- it's already merged in linux-next
- if you wish, I can add another cleanup patch on top, but I'd
  prefer to just leave it as it is.

      Arnd

Re: [Y2038] [PATCH 6/8] lp: fix sparc64 LPSETTIMEOUT ioctl

[Ben Hutchings]

On Thu, 2019-11-21 at 15:04 +0100, Arnd Bergmann wrote:
[...]
> As Greg has already merged the original patch, and that version works
> just as well, I'd probably just leave what I did at first. One benefit is
> that in case we decide to kill off sparc64 support before drivers/char/lp.c,
> the special case can be removed more easily.
> 
> I don't think either of them is going any time soon, but working on y2038
> patches has made me think ahead longer term ;-)
> 
> If you still think we should change it I can send the below patch (now
> actually build-tested) with your Ack.
[...]

I would like it, but since you convinced me the current version works
correctly it's obvious lower priority than the other changes you have.

Ben.

-- 
Ben Hutchings, Software Developer                         Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom