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.
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
