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

[Arnd Bergmann <arnd@arndb.de>]

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