* [PATCH] sg: fix integer overflow @ 2014-01-24 16:34 Mikulas Patocka 2014-01-24 16:36 ` Douglas Gilbert 2014-01-24 16:56 ` James Bottomley 0 siblings, 2 replies; 5+ messages in thread From: Mikulas Patocka @ 2014-01-24 16:34 UTC (permalink / raw) To: Doug Gilbert; +Cc: linux-scsi, James E.J. Bottomley On alpha, USER_HZ may be higher than HZ. This results in integer overflow in MULDIV. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org --- drivers/scsi/sg.c | 2 ++ 1 file changed, 2 insertions(+) Index: linux-3.13/drivers/scsi/sg.c =================================================================== --- linux-3.13.orig/drivers/scsi/sg.c 2014-01-20 21:44:02.000000000 +0100 +++ linux-3.13/drivers/scsi/sg.c 2014-01-24 17:28:02.000000000 +0100 @@ -856,8 +856,10 @@ sg_ioctl(struct file *filp, unsigned int return result; if (val < 0) return -EIO; +#if USER_HZ < HZ if (val >= MULDIV (INT_MAX, USER_HZ, HZ)) val = MULDIV (INT_MAX, USER_HZ, HZ); +#endif sfp->timeout_user = val; sfp->timeout = MULDIV (val, HZ, USER_HZ); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sg: fix integer overflow 2014-01-24 16:34 [PATCH] sg: fix integer overflow Mikulas Patocka @ 2014-01-24 16:36 ` Douglas Gilbert 2014-01-24 16:56 ` James Bottomley 1 sibling, 0 replies; 5+ messages in thread From: Douglas Gilbert @ 2014-01-24 16:36 UTC (permalink / raw) To: Mikulas Patocka; +Cc: linux-scsi, James E.J. Bottomley On 14-01-24 11:34 AM, Mikulas Patocka wrote: > On alpha, USER_HZ may be higher than HZ. This results in integer overflow > in MULDIV. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Acked-by: Douglas Gilbert <dgilbert@interlog.com> > Cc: stable@vger.kernel.org > > --- > drivers/scsi/sg.c | 2 ++ > 1 file changed, 2 insertions(+) > > Index: linux-3.13/drivers/scsi/sg.c > =================================================================== > --- linux-3.13.orig/drivers/scsi/sg.c 2014-01-20 21:44:02.000000000 +0100 > +++ linux-3.13/drivers/scsi/sg.c 2014-01-24 17:28:02.000000000 +0100 > @@ -856,8 +856,10 @@ sg_ioctl(struct file *filp, unsigned int > return result; > if (val < 0) > return -EIO; > +#if USER_HZ < HZ > if (val >= MULDIV (INT_MAX, USER_HZ, HZ)) > val = MULDIV (INT_MAX, USER_HZ, HZ); > +#endif > sfp->timeout_user = val; > sfp->timeout = MULDIV (val, HZ, USER_HZ); > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sg: fix integer overflow 2014-01-24 16:34 [PATCH] sg: fix integer overflow Mikulas Patocka 2014-01-24 16:36 ` Douglas Gilbert @ 2014-01-24 16:56 ` James Bottomley 2014-01-24 17:04 ` Mikulas Patocka 1 sibling, 1 reply; 5+ messages in thread From: James Bottomley @ 2014-01-24 16:56 UTC (permalink / raw) To: Mikulas Patocka; +Cc: Doug Gilbert, linux-scsi On Fri, 2014-01-24 at 11:34 -0500, Mikulas Patocka wrote: > On alpha, USER_HZ may be higher than HZ. This results in integer overflow > in MULDIV. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > Cc: stable@vger.kernel.org > > --- > drivers/scsi/sg.c | 2 ++ > 1 file changed, 2 insertions(+) > > Index: linux-3.13/drivers/scsi/sg.c > =================================================================== > --- linux-3.13.orig/drivers/scsi/sg.c 2014-01-20 21:44:02.000000000 +0100 > +++ linux-3.13/drivers/scsi/sg.c 2014-01-24 17:28:02.000000000 +0100 > @@ -856,8 +856,10 @@ sg_ioctl(struct file *filp, unsigned int > return result; > if (val < 0) > return -EIO; > +#if USER_HZ < HZ > if (val >= MULDIV (INT_MAX, USER_HZ, HZ)) > val = MULDIV (INT_MAX, USER_HZ, HZ); > +#endif Is this really a problem worth fixing? Is USER_HZ ever greater than HZ? > sfp->timeout_user = val; If it can happen, this needs fixing as well: > sfp->timeout = MULDIV (val, HZ, USER_HZ); any val that would divide to less than 1 will now set the timeout to zero, which will cause the queue default timeout to be applied instead. I'd fix it in the macro rather than having code peppered with #ifs James ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sg: fix integer overflow 2014-01-24 16:56 ` James Bottomley @ 2014-01-24 17:04 ` Mikulas Patocka 2014-01-24 17:28 ` Mikulas Patocka 0 siblings, 1 reply; 5+ messages in thread From: Mikulas Patocka @ 2014-01-24 17:04 UTC (permalink / raw) To: James Bottomley; +Cc: Doug Gilbert, linux-scsi On Fri, 24 Jan 2014, James Bottomley wrote: > On Fri, 2014-01-24 at 11:34 -0500, Mikulas Patocka wrote: > > On alpha, USER_HZ may be higher than HZ. This results in integer overflow > > in MULDIV. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Cc: stable@vger.kernel.org > > > > --- > > drivers/scsi/sg.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > Index: linux-3.13/drivers/scsi/sg.c > > =================================================================== > > --- linux-3.13.orig/drivers/scsi/sg.c 2014-01-20 21:44:02.000000000 +0100 > > +++ linux-3.13/drivers/scsi/sg.c 2014-01-24 17:28:02.000000000 +0100 > > @@ -856,8 +856,10 @@ sg_ioctl(struct file *filp, unsigned int > > return result; > > if (val < 0) > > return -EIO; > > +#if USER_HZ < HZ > > if (val >= MULDIV (INT_MAX, USER_HZ, HZ)) > > val = MULDIV (INT_MAX, USER_HZ, HZ); > > +#endif > > Is this really a problem worth fixing? Is USER_HZ ever greater than HZ? Yes. The integer overflow results in unpredictable value and the timeout is limited to that value. > > sfp->timeout_user = val; > > If it can happen, this needs fixing as well: > > > sfp->timeout = MULDIV (val, HZ, USER_HZ); This line is correct. Note, that here the arguments HZ and USER_HZ are swapped. Here, you divide val by USER_HZ and multiply it by HZ, so there is no oveflow if USER_HZ > HZ. There could be overflow if USER_HZ < HZ, but the above "if" condition avoids that. > any val that would divide to less than 1 will now set the timeout to > zero, which will cause the queue default timeout to be applied instead. > > I'd fix it in the macro rather than having code peppered with #ifs You can't easily fix it in the macro: "MULDIV (INT_MAX, USER_HZ, HZ)" is undefined value if USER_HZ > HZ, it is not specified what the macro should return in this case. > James Mikulas ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sg: fix integer overflow 2014-01-24 17:04 ` Mikulas Patocka @ 2014-01-24 17:28 ` Mikulas Patocka 0 siblings, 0 replies; 5+ messages in thread From: Mikulas Patocka @ 2014-01-24 17:28 UTC (permalink / raw) To: James Bottomley; +Cc: Doug Gilbert, linux-scsi On Fri, 24 Jan 2014, Mikulas Patocka wrote: > > > On Fri, 24 Jan 2014, James Bottomley wrote: > > > On Fri, 2014-01-24 at 11:34 -0500, Mikulas Patocka wrote: > > > On alpha, USER_HZ may be higher than HZ. This results in integer overflow > > > in MULDIV. > > > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > > Cc: stable@vger.kernel.org > > > > > > --- > > > drivers/scsi/sg.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > Index: linux-3.13/drivers/scsi/sg.c > > > =================================================================== > > > --- linux-3.13.orig/drivers/scsi/sg.c 2014-01-20 21:44:02.000000000 +0100 > > > +++ linux-3.13/drivers/scsi/sg.c 2014-01-24 17:28:02.000000000 +0100 > > > @@ -856,8 +856,10 @@ sg_ioctl(struct file *filp, unsigned int > > > return result; > > > if (val < 0) > > > return -EIO; > > > +#if USER_HZ < HZ > > > if (val >= MULDIV (INT_MAX, USER_HZ, HZ)) > > > val = MULDIV (INT_MAX, USER_HZ, HZ); > > > +#endif > > > > Is this really a problem worth fixing? Is USER_HZ ever greater than HZ? > > Yes. The integer overflow results in unpredictable value and the timeout > is limited to that value. > > > > sfp->timeout_user = val; > > > > If it can happen, this needs fixing as well: > > > > > sfp->timeout = MULDIV (val, HZ, USER_HZ); > > This line is correct. Note, that here the arguments HZ and USER_HZ are > swapped. Here, you divide val by USER_HZ and multiply it by HZ, so there > is no oveflow if USER_HZ > HZ. There could be overflow if USER_HZ < HZ, > but the above "if" condition avoids that. > > > any val that would divide to less than 1 will now set the timeout to > > zero, which will cause the queue default timeout to be applied instead. > > > > I'd fix it in the macro rather than having code peppered with #ifs > > You can't easily fix it in the macro: "MULDIV (INT_MAX, USER_HZ, HZ)" is > undefined value if USER_HZ > HZ, it is not specified what the macro should > return in this case. > > > James > > Mikulas BTW. if you want to refactor the code to keep #ifs out, you use this. It's does the same thing. From: Mikulas Patocka <mpatocka@redhat.com> Subject: [PATCH] sg: fix integer overflow On alpha, USER_HZ may be higher than HZ. This results in integer overflow in MULDIV. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org --- drivers/scsi/sg.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) Index: linux-3.13/drivers/scsi/sg.c =================================================================== --- linux-3.13.orig/drivers/scsi/sg.c 2014-01-24 18:21:49.000000000 +0100 +++ linux-3.13/drivers/scsi/sg.c 2014-01-24 18:24:38.000000000 +0100 @@ -85,6 +85,12 @@ static void sg_proc_cleanup(void); */ #define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)) +#if USER_HZ < HZ +#define MAX_USER_TIMEOUT MULDIV (INT_MAX, USER_HZ, HZ)) +#else +#define MAX_USER_TIMEOUT INT_MAX +#endif + #define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ) int sg_big_buff = SG_DEF_RESERVED_SIZE; @@ -856,8 +862,8 @@ sg_ioctl(struct file *filp, unsigned int return result; if (val < 0) return -EIO; - if (val >= MULDIV (INT_MAX, USER_HZ, HZ)) - val = MULDIV (INT_MAX, USER_HZ, HZ); + if (val >= MAX_USER_TIMEOUT) + val = MAX_USER_TIMEOUT; sfp->timeout_user = val; sfp->timeout = MULDIV (val, HZ, USER_HZ); ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-24 17:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-24 16:34 [PATCH] sg: fix integer overflow Mikulas Patocka 2014-01-24 16:36 ` Douglas Gilbert 2014-01-24 16:56 ` James Bottomley 2014-01-24 17:04 ` Mikulas Patocka 2014-01-24 17:28 ` Mikulas Patocka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox