| Message ID | 20191108203435.112759-7-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 45a2d64696b11913bcf1087b041740edbade3e21 |
| Headers | show |
| Series | [1/8] y2038: timex: remove incorrect time_t truncation | expand |
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
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
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
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) {
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
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]); }
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(+) -- 2.20.0