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