[PATCH] sg: fix integer overflow

[PATCH] sg: fix integer overflow

[Mikulas Patocka]

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);
 

Re: [PATCH] sg: fix integer overflow

[Douglas Gilbert]

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);
>
>

Re: [PATCH] sg: fix integer overflow

[James Bottomley]

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

Re: [PATCH] sg: fix integer overflow

[Mikulas Patocka]

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

Re: [PATCH] sg: fix integer overflow

[Mikulas Patocka]

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);