All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
@ 2013-12-05 10:58 ` Dan Carpenter
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-12-05 10:58 UTC (permalink / raw)
  To: kernel-janitors

It's not enough to just test if "rpm" is zero, the "rpm * div" operation
could overflow and that could also lead to a divide by zero.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/hwmon/vt8231.c b/drivers/hwmon/vt8231.c
index 0e7017841f7d..923c18034a5f 100644
--- a/drivers/hwmon/vt8231.c
+++ b/drivers/hwmon/vt8231.c
@@ -145,7 +145,7 @@ static const u8 regtempmin[] = { 0x3a, 0x3e, 0x2c, 0x2e, 0x30, 0x32 };
  */
 static inline u8 FAN_TO_REG(long rpm, int div)
 {
-	if (rpm = 0)
+	if (rpm * div = 0)
 		return 0;
 	return clamp_val(1310720 / (rpm * div), 1, 255);
 }
diff --git a/drivers/hwmon/lm78.c b/drivers/hwmon/lm78.c
index 6cf6bff79003..fc4578195674 100644
--- a/drivers/hwmon/lm78.c
+++ b/drivers/hwmon/lm78.c
@@ -92,7 +92,7 @@ static inline u8 IN_TO_REG(unsigned long val)
 
 static inline u8 FAN_TO_REG(long rpm, int div)
 {
-	if (rpm <= 0)
+	if (rpm <= 0 || rpm * div = 0)
 		return 255;
 	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
 }
diff --git a/drivers/hwmon/sis5595.c b/drivers/hwmon/sis5595.c
index 1404e6319deb..811620fe63b4 100644
--- a/drivers/hwmon/sis5595.c
+++ b/drivers/hwmon/sis5595.c
@@ -139,7 +139,7 @@ static inline u8 IN_TO_REG(unsigned long val)
 
 static inline u8 FAN_TO_REG(long rpm, int div)
 {
-	if (rpm <= 0)
+	if (rpm <= 0 || rpm * div = 0)
 		return 255;
 	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
 }

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [lm-sensors] [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
@ 2013-12-05 10:58 ` Dan Carpenter
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-12-05 10:58 UTC (permalink / raw)
  To: kernel-janitors

It's not enough to just test if "rpm" is zero, the "rpm * div" operation
could overflow and that could also lead to a divide by zero.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/hwmon/vt8231.c b/drivers/hwmon/vt8231.c
index 0e7017841f7d..923c18034a5f 100644
--- a/drivers/hwmon/vt8231.c
+++ b/drivers/hwmon/vt8231.c
@@ -145,7 +145,7 @@ static const u8 regtempmin[] = { 0x3a, 0x3e, 0x2c, 0x2e, 0x30, 0x32 };
  */
 static inline u8 FAN_TO_REG(long rpm, int div)
 {
-	if (rpm = 0)
+	if (rpm * div = 0)
 		return 0;
 	return clamp_val(1310720 / (rpm * div), 1, 255);
 }
diff --git a/drivers/hwmon/lm78.c b/drivers/hwmon/lm78.c
index 6cf6bff79003..fc4578195674 100644
--- a/drivers/hwmon/lm78.c
+++ b/drivers/hwmon/lm78.c
@@ -92,7 +92,7 @@ static inline u8 IN_TO_REG(unsigned long val)
 
 static inline u8 FAN_TO_REG(long rpm, int div)
 {
-	if (rpm <= 0)
+	if (rpm <= 0 || rpm * div = 0)
 		return 255;
 	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
 }
diff --git a/drivers/hwmon/sis5595.c b/drivers/hwmon/sis5595.c
index 1404e6319deb..811620fe63b4 100644
--- a/drivers/hwmon/sis5595.c
+++ b/drivers/hwmon/sis5595.c
@@ -139,7 +139,7 @@ static inline u8 IN_TO_REG(unsigned long val)
 
 static inline u8 FAN_TO_REG(long rpm, int div)
 {
-	if (rpm <= 0)
+	if (rpm <= 0 || rpm * div = 0)
 		return 255;
 	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
 }

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
@ 2013-12-05 11:07 roger
  0 siblings, 0 replies; 11+ messages in thread
From: roger @ 2013-12-05 11:07 UTC (permalink / raw)
  To: kernel-janitors

On , Dan Carpenter wrote:
> It's not enough to just test if "rpm" is zero, the "rpm * div" 
> operation
> could overflow and that could also lead to a divide by zero.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/hwmon/vt8231.c b/drivers/hwmon/vt8231.c
> index 0e7017841f7d..923c18034a5f 100644
> --- a/drivers/hwmon/vt8231.c
> +++ b/drivers/hwmon/vt8231.c
> @@ -145,7 +145,7 @@ static const u8 regtempmin[] = { 0x3a, 0x3e, 0x2c,
> 0x2e, 0x30, 0x32 };
>   */
>  static inline u8 FAN_TO_REG(long rpm, int div)
>  {
> -	if (rpm = 0)
> +	if (rpm * div = 0)
>  		return 0;
>  	return clamp_val(1310720 / (rpm * div), 1, 255);
>  }

Seems a reasonable improvement to me, so ACK'd.

> diff --git a/drivers/hwmon/lm78.c b/drivers/hwmon/lm78.c
> index 6cf6bff79003..fc4578195674 100644
SNIP
>  		return 255;
>  	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
>  }


- Roger

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
  2013-12-05 10:58 ` [lm-sensors] " Dan Carpenter
@ 2013-12-05 12:06   ` Jean Delvare
  -1 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2013-12-05 12:06 UTC (permalink / raw)
  To: kernel-janitors

Hi Dan,

On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> could overflow and that could also lead to a divide by zero.

If you believe an overflow can happen (and indeed it can) then this
isn't the way to handle it. Avoiding a divide by zero is certainly nice
but properly handling the other overflow cases too would be better.

In practice, this means for the vt8231 driver:

	if (rpm = 0 || rpm > 1310720)
		return 0;

and for the lm78 and sis5595 drivers:

	if (rpm <= 0)
		return 255;
	if (rpm > 1350000)
		return 0;

That way you're certain to never overflow (the maximum value for div is
8), and insanely large values are handled properly instead of resulting
in random register values.

Thanks,
Jean

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/hwmon/vt8231.c b/drivers/hwmon/vt8231.c
> index 0e7017841f7d..923c18034a5f 100644
> --- a/drivers/hwmon/vt8231.c
> +++ b/drivers/hwmon/vt8231.c
> @@ -145,7 +145,7 @@ static const u8 regtempmin[] = { 0x3a, 0x3e, 0x2c, 0x2e, 0x30, 0x32 };
>   */
>  static inline u8 FAN_TO_REG(long rpm, int div)
>  {
> -	if (rpm = 0)
> +	if (rpm * div = 0)
>  		return 0;
>  	return clamp_val(1310720 / (rpm * div), 1, 255);
>  }
> diff --git a/drivers/hwmon/lm78.c b/drivers/hwmon/lm78.c
> index 6cf6bff79003..fc4578195674 100644
> --- a/drivers/hwmon/lm78.c
> +++ b/drivers/hwmon/lm78.c
> @@ -92,7 +92,7 @@ static inline u8 IN_TO_REG(unsigned long val)
>  
>  static inline u8 FAN_TO_REG(long rpm, int div)
>  {
> -	if (rpm <= 0)
> +	if (rpm <= 0 || rpm * div = 0)
>  		return 255;
>  	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
>  }
> diff --git a/drivers/hwmon/sis5595.c b/drivers/hwmon/sis5595.c
> index 1404e6319deb..811620fe63b4 100644
> --- a/drivers/hwmon/sis5595.c
> +++ b/drivers/hwmon/sis5595.c
> @@ -139,7 +139,7 @@ static inline u8 IN_TO_REG(unsigned long val)
>  
>  static inline u8 FAN_TO_REG(long rpm, int div)
>  {
> -	if (rpm <= 0)
> +	if (rpm <= 0 || rpm * div = 0)
>  		return 255;
>  	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
>  }

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [lm-sensors] [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
@ 2013-12-05 12:06   ` Jean Delvare
  0 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2013-12-05 12:06 UTC (permalink / raw)
  To: kernel-janitors

Hi Dan,

On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> could overflow and that could also lead to a divide by zero.

If you believe an overflow can happen (and indeed it can) then this
isn't the way to handle it. Avoiding a divide by zero is certainly nice
but properly handling the other overflow cases too would be better.

In practice, this means for the vt8231 driver:

	if (rpm = 0 || rpm > 1310720)
		return 0;

and for the lm78 and sis5595 drivers:

	if (rpm <= 0)
		return 255;
	if (rpm > 1350000)
		return 0;

That way you're certain to never overflow (the maximum value for div is
8), and insanely large values are handled properly instead of resulting
in random register values.

Thanks,
Jean

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/hwmon/vt8231.c b/drivers/hwmon/vt8231.c
> index 0e7017841f7d..923c18034a5f 100644
> --- a/drivers/hwmon/vt8231.c
> +++ b/drivers/hwmon/vt8231.c
> @@ -145,7 +145,7 @@ static const u8 regtempmin[] = { 0x3a, 0x3e, 0x2c, 0x2e, 0x30, 0x32 };
>   */
>  static inline u8 FAN_TO_REG(long rpm, int div)
>  {
> -	if (rpm = 0)
> +	if (rpm * div = 0)
>  		return 0;
>  	return clamp_val(1310720 / (rpm * div), 1, 255);
>  }
> diff --git a/drivers/hwmon/lm78.c b/drivers/hwmon/lm78.c
> index 6cf6bff79003..fc4578195674 100644
> --- a/drivers/hwmon/lm78.c
> +++ b/drivers/hwmon/lm78.c
> @@ -92,7 +92,7 @@ static inline u8 IN_TO_REG(unsigned long val)
>  
>  static inline u8 FAN_TO_REG(long rpm, int div)
>  {
> -	if (rpm <= 0)
> +	if (rpm <= 0 || rpm * div = 0)
>  		return 255;
>  	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
>  }
> diff --git a/drivers/hwmon/sis5595.c b/drivers/hwmon/sis5595.c
> index 1404e6319deb..811620fe63b4 100644
> --- a/drivers/hwmon/sis5595.c
> +++ b/drivers/hwmon/sis5595.c
> @@ -139,7 +139,7 @@ static inline u8 IN_TO_REG(unsigned long val)
>  
>  static inline u8 FAN_TO_REG(long rpm, int div)
>  {
> -	if (rpm <= 0)
> +	if (rpm <= 0 || rpm * div = 0)
>  		return 255;
>  	return clamp_val((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
>  }

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
  2013-12-05 10:58 ` [lm-sensors] " Dan Carpenter
@ 2013-12-05 12:59   ` Dan Carpenter
  -1 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-12-05 12:59 UTC (permalink / raw)
  To: kernel-janitors

On Thu, Dec 05, 2013 at 01:06:13PM +0100, Jean Delvare wrote:
> Hi Dan,
> 
> On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> > It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> > could overflow and that could also lead to a divide by zero.
> 
> If you believe an overflow can happen (and indeed it can) then this
> isn't the way to handle it. Avoiding a divide by zero is certainly nice
> but properly handling the other overflow cases too would be better.
> 
> In practice, this means for the vt8231 driver:
> 
> 	if (rpm = 0 || rpm > 1310720)
> 		return 0;
> 
> and for the lm78 and sis5595 drivers:
> 
> 	if (rpm <= 0)
> 		return 255;
> 	if (rpm > 1350000)
> 		return 0;
> 
> That way you're certain to never overflow (the maximum value for div is
> 8), and insanely large values are handled properly instead of resulting
> in random register values.

Yes.  I will resend.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [lm-sensors] [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
@ 2013-12-05 12:59   ` Dan Carpenter
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-12-05 12:59 UTC (permalink / raw)
  To: kernel-janitors

On Thu, Dec 05, 2013 at 01:06:13PM +0100, Jean Delvare wrote:
> Hi Dan,
> 
> On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> > It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> > could overflow and that could also lead to a divide by zero.
> 
> If you believe an overflow can happen (and indeed it can) then this
> isn't the way to handle it. Avoiding a divide by zero is certainly nice
> but properly handling the other overflow cases too would be better.
> 
> In practice, this means for the vt8231 driver:
> 
> 	if (rpm = 0 || rpm > 1310720)
> 		return 0;
> 
> and for the lm78 and sis5595 drivers:
> 
> 	if (rpm <= 0)
> 		return 255;
> 	if (rpm > 1350000)
> 		return 0;
> 
> That way you're certain to never overflow (the maximum value for div is
> 8), and insanely large values are handled properly instead of resulting
> in random register values.

Yes.  I will resend.

regards,
dan carpenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
  2013-12-05 10:58 ` [lm-sensors] " Dan Carpenter
@ 2013-12-05 13:13   ` Dan Carpenter
  -1 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-12-05 13:13 UTC (permalink / raw)
  To: kernel-janitors

On Thu, Dec 05, 2013 at 01:06:13PM +0100, Jean Delvare wrote:
> Hi Dan,
> 
> On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> > It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> > could overflow and that could also lead to a divide by zero.
> 
> If you believe an overflow can happen (and indeed it can) then this
> isn't the way to handle it. Avoiding a divide by zero is certainly nice
> but properly handling the other overflow cases too would be better.
> 
> In practice, this means for the vt8231 driver:
> 
> 	if (rpm = 0 || rpm > 1310720)
> 		return 0;
> 
> and for the lm78 and sis5595 drivers:
> 
> 	if (rpm <= 0)
> 		return 255;
> 	if (rpm > 1350000)
> 		return 0;

For these two are you sure the return value shouldn't be 1?

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [lm-sensors] [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
@ 2013-12-05 13:13   ` Dan Carpenter
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-12-05 13:13 UTC (permalink / raw)
  To: kernel-janitors

On Thu, Dec 05, 2013 at 01:06:13PM +0100, Jean Delvare wrote:
> Hi Dan,
> 
> On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> > It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> > could overflow and that could also lead to a divide by zero.
> 
> If you believe an overflow can happen (and indeed it can) then this
> isn't the way to handle it. Avoiding a divide by zero is certainly nice
> but properly handling the other overflow cases too would be better.
> 
> In practice, this means for the vt8231 driver:
> 
> 	if (rpm = 0 || rpm > 1310720)
> 		return 0;
> 
> and for the lm78 and sis5595 drivers:
> 
> 	if (rpm <= 0)
> 		return 255;
> 	if (rpm > 1350000)
> 		return 0;

For these two are you sure the return value shouldn't be 1?

regards,
dan carpenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
  2013-12-05 10:58 ` [lm-sensors] " Dan Carpenter
@ 2013-12-05 14:29   ` Jean Delvare
  -1 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2013-12-05 14:29 UTC (permalink / raw)
  To: kernel-janitors

On Thu, 5 Dec 2013 16:13:32 +0300, Dan Carpenter wrote:
> On Thu, Dec 05, 2013 at 01:06:13PM +0100, Jean Delvare wrote:
> > Hi Dan,
> > 
> > On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> > > It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> > > could overflow and that could also lead to a divide by zero.
> > 
> > If you believe an overflow can happen (and indeed it can) then this
> > isn't the way to handle it. Avoiding a divide by zero is certainly nice
> > but properly handling the other overflow cases too would be better.
> > 
> > In practice, this means for the vt8231 driver:
> > 
> > 	if (rpm = 0 || rpm > 1310720)
> > 		return 0;
> > 
> > and for the lm78 and sis5595 drivers:
> > 
> > 	if (rpm <= 0)
> > 		return 255;
> > 	if (rpm > 1350000)
> > 		return 0;
> 
> For these two are you sure the return value shouldn't be 1?

Hmm, yes, you're right. That won't make a big difference in practice
but returning 1 would be better.

Thanks,
-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [lm-sensors] [patch] hwmon: prevent some divide by zeros in FAN_TO_REG()
@ 2013-12-05 14:29   ` Jean Delvare
  0 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2013-12-05 14:29 UTC (permalink / raw)
  To: kernel-janitors

On Thu, 5 Dec 2013 16:13:32 +0300, Dan Carpenter wrote:
> On Thu, Dec 05, 2013 at 01:06:13PM +0100, Jean Delvare wrote:
> > Hi Dan,
> > 
> > On Thu, 5 Dec 2013 13:58:45 +0300, Dan Carpenter wrote:
> > > It's not enough to just test if "rpm" is zero, the "rpm * div" operation
> > > could overflow and that could also lead to a divide by zero.
> > 
> > If you believe an overflow can happen (and indeed it can) then this
> > isn't the way to handle it. Avoiding a divide by zero is certainly nice
> > but properly handling the other overflow cases too would be better.
> > 
> > In practice, this means for the vt8231 driver:
> > 
> > 	if (rpm = 0 || rpm > 1310720)
> > 		return 0;
> > 
> > and for the lm78 and sis5595 drivers:
> > 
> > 	if (rpm <= 0)
> > 		return 255;
> > 	if (rpm > 1350000)
> > 		return 0;
> 
> For these two are you sure the return value shouldn't be 1?

Hmm, yes, you're right. That won't make a big difference in practice
but returning 1 would be better.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-12-05 14:29 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-05 10:58 [patch] hwmon: prevent some divide by zeros in FAN_TO_REG() Dan Carpenter
2013-12-05 10:58 ` [lm-sensors] " Dan Carpenter
2013-12-05 12:06 ` Jean Delvare
2013-12-05 12:06   ` [lm-sensors] " Jean Delvare
2013-12-05 12:59 ` Dan Carpenter
2013-12-05 12:59   ` [lm-sensors] " Dan Carpenter
2013-12-05 13:13 ` Dan Carpenter
2013-12-05 13:13   ` [lm-sensors] " Dan Carpenter
2013-12-05 14:29 ` Jean Delvare
2013-12-05 14:29   ` [lm-sensors] " Jean Delvare
  -- strict thread matches above, loose matches on Subject: below --
2013-12-05 11:07 roger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.