hwmon (occ): Fix integer overflow in multiplication

PATCH In review Eddie James 4 participants 7 messages Dec 6, 2017 – Dec 19, 2017 Hardware monitoring

lore

Share & tools

lore

Eddie James

author

2017-12-06 19:19 UTC #0 lore raw

PATCH 1/1 hwmon (occ): Fix integer overflow in multiplication

From: "Edward A. James" <[email protected]>

Power values were overflowing INT_MAX when being converted to microwatts, even though the storage was sufficiently large (unsigned 64 bit). Change literals to unsigned long long.

Signed-off-by Edward A. James <[email protected]>

---
 drivers/hwmon/occ/common.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

drivers/hwmon/occ/common.c +9 -8

index 3175e7b..a64f3db 100644
@@ -468,7 +468,7 @@ static ssize_t occ_show_power_1(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int rc;
-	u32 val = 0;
+	u64 val = 0;
 	struct power_sensor_1 *power;
 	struct occ *occ = dev_get_drvdata(dev);
 	struct occ_sensors *sensors = &occ->sensors;

@@ -491,11 +491,11 @@ static ssize_t occ_show_power_1(struct device *dev,
 		val = get_unaligned_be32(&power->accumulator);
 		break;
 	case 3:
-		val = get_unaligned_be16(&power->value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->value) * 1000000ULL;
 		break;
 	}
 
-	return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+	return snprintf(buf, PAGE_SIZE - 1, "%llu\n", val);
 }
 
 static ssize_t occ_show_power_2(struct device *dev,

@@ -525,7 +525,7 @@ static ssize_t occ_show_power_2(struct device *dev,
 		val = get_unaligned_be64(&power->accumulator);
 		break;
 	case 3:
-		val = get_unaligned_be16(&power->value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->value) * 1000000ULL;
 		break;
 	case 4:
 		val = power->function_id;

@@ -562,7 +562,8 @@ static ssize_t occ_show_power_a0(struct device *dev,
 		val = get_unaligned_be16(&power->system.update_time);
 		break;
 	case 2:
-		val = get_unaligned_be16(&power->system.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->system.value) *
+			1000000ULL;
 		break;
 	case 3:
 		val = get_unaligned_be32(&power->system.update_tag);

@@ -574,7 +575,7 @@ static ssize_t occ_show_power_a0(struct device *dev,
 		val = get_unaligned_be16(&power->proc.update_time);
 		break;
 	case 6:
-		val = get_unaligned_be16(&power->proc.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->proc.value) * 1000000ULL;
 		break;
 	case 7:
 		val = get_unaligned_be32(&power->proc.update_tag);

@@ -583,7 +584,7 @@ static ssize_t occ_show_power_a0(struct device *dev,
 		val = get_unaligned_be64(&power->proc.accumulator);
 		break;
 	case 9:
-		val = get_unaligned_be16(&power->vdd.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->vdd.value) * 1000000ULL;
 		break;
 	case 10:
 		val = get_unaligned_be32(&power->vdd.update_tag);

@@ -592,7 +593,7 @@ static ssize_t occ_show_power_a0(struct device *dev,
 		val = get_unaligned_be64(&power->vdd.accumulator);
 		break;
 	case 12:
-		val = get_unaligned_be16(&power->vdn.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->vdn.value) * 1000000ULL;
 		break;
 	case 13:
 		val = get_unaligned_be32(&power->vdn.update_tag);

Signature · 1 line

1.8.3.1

Joel Stanley

2017-12-08 06:16 UTC #1 lore raw

Quoted 12 lines · Eddie James

drivers/hwmon/occ/common.c +1 -1

@@ -468,7 +468,7 @@ static ssize_t occ_show_power_1(struct device *dev,
                                struct device_attribute *attr, char *buf)
 {
        int rc;
-       u32 val = 0;
+       u64 val = 0;
        struct power_sensor_1 *power;
        struct occ *occ = dev_get_drvdata(dev);
        struct occ_sensors *sensors = &occ->sensors;

drivers/hwmon/occ/common.c static ssize_t occ_show_power_1(struct device *dev, · line 494

Quoted 5 lines

drivers/hwmon/occ/common.c +1 -1

@@ -491,11 +491,11 @@ static ssize_t occ_show_power_1(struct device *dev,
                val = get_unaligned_be32(&power->accumulator);
                break;
        case 3:
-               val = get_unaligned_be16(&power->value) * 1000000;
+               val = (u64)get_unaligned_be16(&power->value) * 1000000ULL;

I don't think these casts are required, are they?

Eddie James

author

2017-12-08 22:17 UTC #2 lore raw

Quoted 11 lines · Joel Stanley

Quoted 10 lines · Eddie James

drivers/hwmon/occ/common.c +1 -1

@@ -468,7 +468,7 @@ static ssize_t occ_show_power_1(struct device *dev,
                                 struct device_attribute *attr, char *buf)
  {
         int rc;
-       u32 val = 0;
+       u64 val = 0;
         struct power_sensor_1 *power;
         struct occ *occ = dev_get_drvdata(dev);

Quoted 8 lines

drivers/hwmon/occ/common.c static ssize_t occ_show_power_1(struct device *dev, · line 494

Quoted 7 lines

drivers/hwmon/occ/common.c +1 -1

         struct occ_sensors *sensors = &occ->sensors;

@@ -491,11 +491,11 @@ static ssize_t occ_show_power_1(struct device *dev,
                 val = get_unaligned_be32(&power->accumulator);
                 break;
         case 3:
-               val = get_unaligned_be16(&power->value) * 1000000;
+               val = (u64)get_unaligned_be16(&power->value) * 1000000ULL;

I don't think these casts are required, are they?

I'm not at all sure. I didn't think adding ULL was required here, but apparently it is. I don't really trust GCC...

Quoted 1 line

Joel Stanley

2017-12-18 14:11 UTC #3 lore raw

Quoted 30 lines · Eddie James

Quoted 17 lines · Joel Stanley

Quoted 14 lines · Eddie James

drivers/hwmon/occ/common.c +1 -1

@@ -468,7 +468,7 @@ static ssize_t occ_show_power_1(struct device *dev,
                                 struct device_attribute *attr, char
 *buf)
  {
         int rc;
-       u32 val = 0;
+       u64 val = 0;
         struct power_sensor_1 *power;
         struct occ *occ = dev_get_drvdata(dev);

         struct occ_sensors *sensors = &occ->sensors;

drivers/hwmon/occ/common.c +0 -0

@@ -491,11 +491,11 @@ static ssize_t occ_show_power_1(struct device *dev,

Quoted 8 lines

drivers/hwmon/occ/common.c static ssize_t occ_show_power_1(struct device *dev,
Quoted 6 lines
drivers/hwmon/occ/common.c +1 -1
                 val = get_unaligned_be32(&power->accumulator);
                 break;
         case 3:
-               val = get_unaligned_be16(&power->value) * 1000000;
+               val = (u64)get_unaligned_be16(&power->value) *
 1000000ULL;
I don't think these casts are required, are they?

Quoted 2 lines

I'm not at all sure. I didn't think adding ULL was required here, but apparently it is. I don't really trust GCC...

Sorry, I thought you were joking here. GCC is behaving as it should. Hit me up on IRC for an explanation sometime.

Can you resend without the casts?

Cheers,

Joel

Lei YU

2017-12-19 02:27 UTC #4 lore raw

Hi Eddie,

Except for the values for power, the value used for temperature is u16 and overflows
as well.
Powercore reports that if the temperature goes over 65 degrees, the value overflows
obviously as long as it is over 65535.

Could you fix this as well?

Thanks!

--
BRs,
Lei YU

Quoted 42 lines · Joel Stanley

Quoted 30 lines · Eddie James
Quoted 17 lines · Joel Stanley
Quoted 14 lines · Eddie James
drivers/hwmon/occ/common.c +1 -1
@@ -468,7 +468,7 @@ static ssize_t occ_show_power_1(struct device *dev,
                                 struct device_attribute *attr, char
 *buf)
  {
         int rc;
-       u32 val = 0;
+       u64 val = 0;
         struct power_sensor_1 *power;
         struct occ *occ = dev_get_drvdata(dev);
         struct occ_sensors *sensors = &occ->sensors;
drivers/hwmon/occ/common.c +0 -0
@@ -491,11 +491,11 @@ static ssize_t occ_show_power_1(struct device *dev,
Quoted 8 lines
drivers/hwmon/occ/common.c static ssize_t occ_show_power_1(struct device *dev,
Quoted 6 lines
drivers/hwmon/occ/common.c +1 -1
                 val = get_unaligned_be32(&power->accumulator);
                 break;
         case 3:
-               val = get_unaligned_be16(&power->value) * 1000000;
+               val = (u64)get_unaligned_be16(&power->value) *
 1000000ULL;
I don't think these casts are required, are they?
Quoted 2 lines
I'm not at all sure. I didn't think adding ULL was required here, but apparently it is. I don't really trust GCC...
Sorry, I thought you were joking here. GCC is behaving as it should. Hit me up on IRC for an explanation sometime.
Can you resend without the casts?
Cheers,
Joel

Eddie James

author

2017-12-19 19:56 UTC #5 lore raw

Quoted 10 lines · Lei YU

Hi Eddie,
Except for the values for power, the value used for temperature is u16 and overflows
as well.
Powercore reports that if the temperature goes over 65 degrees, the value overflows
obviously as long as it is over 65535.
Could you fix this as well?

Yep, thanks for spotting that.

Eddie

Quoted 44 lines

Thanks!
--
BRs,
Lei YU
Quoted 36 lines · Joel Stanley
Quoted 25 lines · Eddie James
Quoted 14 lines · Joel Stanley
Quoted 12 lines · Eddie James
drivers/hwmon/occ/common.c +1 -1
@@ -468,7 +468,7 @@ static ssize_t occ_show_power_1(struct device *dev,
                                  struct device_attribute *attr, char
 *buf)
   {
          int rc;
-       u32 val = 0;
+       u64 val = 0;
          struct power_sensor_1 *power;
          struct occ *occ = dev_get_drvdata(dev);
          struct occ_sensors *sensors = &occ->sensors;
Quoted 8 lines
drivers/hwmon/occ/common.c static ssize_t occ_show_power_1(struct device *dev, · line 495
Quoted 7 lines
drivers/hwmon/occ/common.c +1 -1
@@ -491,11 +491,11 @@ static ssize_t occ_show_power_1(struct device *dev,
                  val = get_unaligned_be32(&power->accumulator);
                  break;
          case 3:
-               val = get_unaligned_be16(&power->value) * 1000000;
+               val = (u64)get_unaligned_be16(&power->value) *
 1000000ULL;
I don't think these casts are required, are they?
Quoted 2 lines
I'm not at all sure. I didn't think adding ULL was required here, but apparently it is. I don't really trust GCC...
Sorry, I thought you were joking here. GCC is behaving as it should. Hit me up on IRC for an explanation sometime.
Can you resend without the casts?
Cheers,
Joel

Christopher Bostic

Reviewed-by on v1 1/1

2017-12-12 14:24 UTC #6 lore raw

In reply to Eddie James

Reviewed-by Christopher Bostic <[email protected]>

Quoted 84 lines · Eddie James

From: "Edward A. James" <[email protected]>

Power values were overflowing INT_MAX when being converted to microwatts, even though the storage was sufficiently large (unsigned 64 bit). Change literals to unsigned long long.

Signed-off-by Edward A. James <[email protected]>

---
  drivers/hwmon/occ/common.c | 17 +++++++++--------
  1 file changed, 9 insertions(+), 8 deletions(-)

drivers/hwmon/occ/common.c +9 -8

index 3175e7b..a64f3db 100644
@@ -468,7 +468,7 @@ static ssize_t occ_show_power_1(struct device *dev,
  				struct device_attribute *attr, char *buf)
  {
  	int rc;
-	u32 val = 0;
+	u64 val = 0;
  	struct power_sensor_1 *power;
  	struct occ *occ = dev_get_drvdata(dev);
  	struct occ_sensors *sensors = &occ->sensors;

@@ -491,11 +491,11 @@ static ssize_t occ_show_power_1(struct device *dev,
  		val = get_unaligned_be32(&power->accumulator);
  		break;
  	case 3:
-		val = get_unaligned_be16(&power->value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->value) * 1000000ULL;
  		break;
  	}
 
-	return snprintf(buf, PAGE_SIZE - 1, "%u\n", val);
+	return snprintf(buf, PAGE_SIZE - 1, "%llu\n", val);
  }
 
  static ssize_t occ_show_power_2(struct device *dev,

@@ -525,7 +525,7 @@ static ssize_t occ_show_power_2(struct device *dev,
  		val = get_unaligned_be64(&power->accumulator);
  		break;
  	case 3:
-		val = get_unaligned_be16(&power->value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->value) * 1000000ULL;
  		break;
  	case 4:
  		val = power->function_id;

@@ -562,7 +562,8 @@ static ssize_t occ_show_power_a0(struct device *dev,
  		val = get_unaligned_be16(&power->system.update_time);
  		break;
  	case 2:
-		val = get_unaligned_be16(&power->system.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->system.value) *
+			1000000ULL;
  		break;
  	case 3:
  		val = get_unaligned_be32(&power->system.update_tag);

@@ -574,7 +575,7 @@ static ssize_t occ_show_power_a0(struct device *dev,
  		val = get_unaligned_be16(&power->proc.update_time);
  		break;
  	case 6:
-		val = get_unaligned_be16(&power->proc.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->proc.value) * 1000000ULL;
  		break;
  	case 7:
  		val = get_unaligned_be32(&power->proc.update_tag);

@@ -583,7 +584,7 @@ static ssize_t occ_show_power_a0(struct device *dev,
  		val = get_unaligned_be64(&power->proc.accumulator);
  		break;
  	case 9:
-		val = get_unaligned_be16(&power->vdd.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->vdd.value) * 1000000ULL;
  		break;
  	case 10:
  		val = get_unaligned_be32(&power->vdd.update_tag);

@@ -592,7 +593,7 @@ static ssize_t occ_show_power_a0(struct device *dev,
  		val = get_unaligned_be64(&power->vdd.accumulator);
  		break;
  	case 12:
-		val = get_unaligned_be16(&power->vdn.value) * 1000000;
+		val = (u64)get_unaligned_be16(&power->vdn.value) * 1000000ULL;
  		break;
  	case 13:
  		val = get_unaligned_be32(&power->vdn.update_tag);