[linux,dev-4.10] hwmon (occ): Fix integer overflow in multiplication

Message ID 1512587959-27264-1-git-send-email-eajames@linux.vnet.ibm.com
State New
Headers show
Series
  • [linux,dev-4.10] hwmon (occ): Fix integer overflow in multiplication
Related show

Commit Message

Eddie James Dec. 6, 2017, 7:19 p.m.
From: "Edward A. James" <eajames@us.ibm.com>

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 <eajames@us.ibm.com>
---
 drivers/hwmon/occ/common.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Joel Stanley Dec. 8, 2017, 6:16 a.m. | #1
On Thu, Dec 7, 2017 at 5:49 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -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;

I don't think these casts are required, are they?
Eddie James Dec. 8, 2017, 10:17 p.m. | #2
On 12/08/2017 12:16 AM, Joel Stanley wrote:
> On Thu, Dec 7, 2017 at 5:49 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> --- a/drivers/hwmon/occ/common.c
>> +++ b/drivers/hwmon/occ/common.c
>> @@ -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;
> 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...

>
Christopher Bostic Dec. 12, 2017, 2:24 p.m. | #3
Reviewed-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>


On 12/6/17 1:19 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> 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 <eajames@us.ibm.com>
> ---
>   drivers/hwmon/occ/common.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 3175e7b..a64f3db 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -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);

Patch

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 3175e7b..a64f3db 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -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);