[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 Superseded, archived
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);
Joel Stanley Dec. 18, 2017, 2:11 p.m. | #4
On Sat, Dec 9, 2017 at 8:47 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>
>
> 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...

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 Dec. 19, 2017, 2:27 a.m. | #5
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

On Mon, Dec 18, 2017 at 10:11 PM, Joel Stanley <joel@jms.id.au> wrote:
> On Sat, Dec 9, 2017 at 8:47 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>>
>>
>> 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...
>
> 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 Dec. 19, 2017, 7:56 p.m. | #6
On 12/18/2017 08:27 PM, Lei YU wrote:
> 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

>
> Thanks!
>
> --
> BRs,
> Lei YU
>
> On Mon, Dec 18, 2017 at 10:11 PM, Joel Stanley <joel@jms.id.au> wrote:
>> On Sat, Dec 9, 2017 at 8:47 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>>>
>>> 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...
>> 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

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