[PATCH 1/2] scsi: sg: Avoid overflow when USER_HZ > HZ

[PATCH 1/2] scsi: sg: Avoid overflow when USER_HZ > HZ

[Paul Burton]

Calculating the maximum timeout that a user can set via the
SG_SET_TIMEOUT ioctl involves multiplying INT_MAX by USER_HZ/HZ. If
USER_HZ is larger than HZ then this results in an overflow when
performed as a 32 bit integer calculation, resulting in compiler
warnings such as the following:

  drivers/scsi/sg.c: In function 'sg_ioctl':
  drivers/scsi/sg.c:91:67: warning: integer overflow in expression [-Woverflow]
   #define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL))
                                                                     ^
  drivers/scsi/sg.c:887:14: note: in expansion of macro 'MULDIV'
     if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
                ^
  drivers/scsi/sg.c:91:67: warning: integer overflow in expression [-Woverflow]
   #define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL))
                                                                     ^
  drivers/scsi/sg.c:888:13: note: in expansion of macro 'MULDIV'
         val = MULDIV (INT_MAX, USER_HZ, HZ);
               ^

Avoid this overflow by performing the (constant) arithmetic on 64 bit
integers, which ensures that overflow from multiplying the 32 bit values
cannot occur. When converting the result back to a 32 bit integer use
min_t to ensure that we don't simply truncate a value beyond INT_MAX to
a 32 bit integer, but instead use INT_MAX where the result was larger
than it. As the values are all compile time constant the 64 bit
arithmetic should have no runtime cost.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---
 drivers/scsi/sg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index ae7d9bd..bb5ec2d 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -884,8 +884,9 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 			return result;
 		if (val < 0)
 			return -EIO;
-		if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
-		    val = MULDIV (INT_MAX, USER_HZ, HZ);
+		if (val >= MULDIV((s64)INT_MAX, USER_HZ, HZ))
+			val = min_t(s64, MULDIV((s64)INT_MAX, USER_HZ, HZ),
+				    INT_MAX);
 		sfp->timeout_user = val;
 		sfp->timeout = MULDIV (val, HZ, USER_HZ);
 
-- 
2.9.3

[PATCH 2/2] scsi: sg: Use mult_frac, drop MULDIV macro

[Paul Burton]

The MULDIV macro is essentially a duplicate of the more standard
mult_frac macro. Replace use of MULDIV with mult_frac & drop the
duplication.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---
 drivers/scsi/sg.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index bb5ec2d..070332e 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -79,18 +79,7 @@ static void sg_proc_cleanup(void);
  */
 #define SG_MAX_CDB_SIZE 252
 
-/*
- * Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
- * Then when using 32 bit integers x * m may overflow during the calculation.
- * Replacing muldiv(x) by muldiv(x)=((x % d) * m) / d + int(x / d) * m
- * calculates the same, but prevents the overflow when both m and d
- * are "small" numbers (like HZ and USER_HZ).
- * Of course an overflow is inavoidable if the result of muldiv doesn't fit
- * in 32 bits.
- */
-#define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL))
-
-#define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
+#define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
 
 int sg_big_buff = SG_DEF_RESERVED_SIZE;
 /* N.B. This variable is readable and writeable via
@@ -884,11 +873,11 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 			return result;
 		if (val < 0)
 			return -EIO;
-		if (val >= MULDIV((s64)INT_MAX, USER_HZ, HZ))
-			val = min_t(s64, MULDIV((s64)INT_MAX, USER_HZ, HZ),
+		if (val >= mult_frac((s64)INT_MAX, USER_HZ, HZ))
+			val = min_t(s64, mult_frac((s64)INT_MAX, USER_HZ, HZ),
 				    INT_MAX);
 		sfp->timeout_user = val;
-		sfp->timeout = MULDIV (val, HZ, USER_HZ);
+		sfp->timeout = mult_frac(val, HZ, USER_HZ);
 
 		return 0;
 	case SG_GET_TIMEOUT:	/* N.B. User receives timeout as return value */
-- 
2.9.3

Re: [PATCH 1/2] scsi: sg: Avoid overflow when USER_HZ > HZ

[Martin K. Petersen]

>>>>> "Paul" == Paul Burton <paul.burton@imgtec.com> writes:

Paul> Calculating the maximum timeout that a user can set via the
Paul> SG_SET_TIMEOUT ioctl involves multiplying INT_MAX by
Paul> USER_HZ/HZ. If USER_HZ is larger than HZ then this results in an
Paul> overflow when performed as a 32 bit integer calculation, resulting
Paul> in compiler warnings such as the following:

Doug: Please review.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/2] scsi: sg: Avoid overflow when USER_HZ > HZ

[Douglas Gilbert]

On 2016-08-19 06:43 PM, Paul Burton wrote:
> Calculating the maximum timeout that a user can set via the
> SG_SET_TIMEOUT ioctl involves multiplying INT_MAX by USER_HZ/HZ. If
> USER_HZ is larger than HZ then this results in an overflow when
> performed as a 32 bit integer calculation, resulting in compiler
> warnings such as the following:
>
>   drivers/scsi/sg.c: In function 'sg_ioctl':
>   drivers/scsi/sg.c:91:67: warning: integer overflow in expression [-Woverflow]
>    #define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL))
>                                                                      ^
>   drivers/scsi/sg.c:887:14: note: in expansion of macro 'MULDIV'
>      if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
>                 ^
>   drivers/scsi/sg.c:91:67: warning: integer overflow in expression [-Woverflow]
>    #define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL))
>                                                                      ^
>   drivers/scsi/sg.c:888:13: note: in expansion of macro 'MULDIV'
>          val = MULDIV (INT_MAX, USER_HZ, HZ);
>                ^
>
> Avoid this overflow by performing the (constant) arithmetic on 64 bit
> integers, which ensures that overflow from multiplying the 32 bit values
> cannot occur. When converting the result back to a 32 bit integer use
> min_t to ensure that we don't simply truncate a value beyond INT_MAX to
> a 32 bit integer, but instead use INT_MAX where the result was larger
> than it. As the values are all compile time constant the 64 bit
> arithmetic should have no runtime cost.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>

That macro has been changed several times, hopefully this is the final
time.

Acked by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>  drivers/scsi/sg.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index ae7d9bd..bb5ec2d 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -884,8 +884,9 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
>  			return result;
>  		if (val < 0)
>  			return -EIO;
> -		if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
> -		    val = MULDIV (INT_MAX, USER_HZ, HZ);
> +		if (val >= MULDIV((s64)INT_MAX, USER_HZ, HZ))
> +			val = min_t(s64, MULDIV((s64)INT_MAX, USER_HZ, HZ),
> +				    INT_MAX);
>  		sfp->timeout_user = val;
>  		sfp->timeout = MULDIV (val, HZ, USER_HZ);
>
>

Re: [PATCH 2/2] scsi: sg: Use mult_frac, drop MULDIV macro

[Douglas Gilbert]

On 2016-08-19 06:43 PM, Paul Burton wrote:
> The MULDIV macro is essentially a duplicate of the more standard
> mult_frac macro. Replace use of MULDIV with mult_frac & drop the
> duplication.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>

I spoke too soon, as this patch changes the last one :-)
As far as I can determine mult_frac() is a cleaner version of
MULDIV(); so

Acked by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>  drivers/scsi/sg.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index bb5ec2d..070332e 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -79,18 +79,7 @@ static void sg_proc_cleanup(void);
>   */
>  #define SG_MAX_CDB_SIZE 252
>
> -/*
> - * Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
> - * Then when using 32 bit integers x * m may overflow during the calculation.
> - * Replacing muldiv(x) by muldiv(x)=((x % d) * m) / d + int(x / d) * m
> - * calculates the same, but prevents the overflow when both m and d
> - * are "small" numbers (like HZ and USER_HZ).
> - * Of course an overflow is inavoidable if the result of muldiv doesn't fit
> - * in 32 bits.
> - */
> -#define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL))
> -
> -#define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
> +#define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
>
>  int sg_big_buff = SG_DEF_RESERVED_SIZE;
>  /* N.B. This variable is readable and writeable via
> @@ -884,11 +873,11 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
>  			return result;
>  		if (val < 0)
>  			return -EIO;
> -		if (val >= MULDIV((s64)INT_MAX, USER_HZ, HZ))
> -			val = min_t(s64, MULDIV((s64)INT_MAX, USER_HZ, HZ),
> +		if (val >= mult_frac((s64)INT_MAX, USER_HZ, HZ))
> +			val = min_t(s64, mult_frac((s64)INT_MAX, USER_HZ, HZ),
>  				    INT_MAX);
>  		sfp->timeout_user = val;
> -		sfp->timeout = MULDIV (val, HZ, USER_HZ);
> +		sfp->timeout = mult_frac(val, HZ, USER_HZ);
>
>  		return 0;
>  	case SG_GET_TIMEOUT:	/* N.B. User receives timeout as return value */
>

Re: [PATCH 1/2] scsi: sg: Avoid overflow when USER_HZ > HZ

[Martin K. Petersen]

>>>>> "Paul" == Paul Burton <paul.burton@imgtec.com> writes:

Paul> Calculating the maximum timeout that a user can set via the
Paul> SG_SET_TIMEOUT ioctl involves multiplying INT_MAX by
Paul> USER_HZ/HZ. If USER_HZ is larger than HZ then this results in an
Paul> overflow when performed as a 32 bit integer calculation, resulting
Paul> in compiler warnings such as the following:

Applied to 4.9/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] scsi: sg: Use mult_frac, drop MULDIV macro

[Martin K. Petersen]

>>>>> "Paul" == Paul Burton <paul.burton@imgtec.com> writes:

Paul> The MULDIV macro is essentially a duplicate of the more standard
Paul> mult_frac macro. Replace use of MULDIV with mult_frac & drop the
Paul> duplication.

Applied to 4.9/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering