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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > > Fixes: 9a450484089d ("lp: support 64-bit time_t user space")
> > > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> > > ---
> > >  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