Re: [PATCH 1/2] hwmon: (adt7475) Potential error pointer dereferences

[Guenter Roeck]

On Tue, Aug 14, 2018 at 12:12:36PM +0300, Dan Carpenter wrote:
> The adt7475_update_device() function returns error pointers.  The
> problem is that in show_pwmfreq() we dereference it before the check.
> And then in pwm_use_point2_pwm_at_crit_show() there isn't a check at
> all.  I don't know if it's required, but it silences a static checker
> warning and it's doesn't hurt anything to check.
> 
> Signed-off-by: Dan Carpenter 
> Reviewed-by: Tokunori Ikegami 

Applied.

Thanks,
Guenter

RE: [PATCH 1/2] hwmon: (adt7475) Potential error pointer dereferences

[IKEGAMI Tokunori]

Hi Dan-san,

Thank you so much for the fixes.
But very sorry for the problem caused by my changes.

I have reviewed them and those look good.

Reviewed-by: Tokunori Ikegami 

Regards,
Ikegami

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Tuesday, August 14, 2018 6:13 PM
> To: Jean Delvare; IKEGAMI Tokunori
> Cc: Guenter Roeck; linux-hwmon@vger.kernel.org;
> kernel-janit...@vger.kernel.org
> Subject: [PATCH 1/2] hwmon: (adt7475) Potential error pointer
> dereferences
> 
> The adt7475_update_device() function returns error pointers.  The
> problem is that in show_pwmfreq() we dereference it before the check.
> And then in pwm_use_point2_pwm_at_crit_show() there isn't a check at
> all.  I don't know if it's required, but it silences a static checker
> warning and it's doesn't hurt anything to check.
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index 90837f7c7d0f..16045149f3db 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -962,13 +962,14 @@ static ssize_t show_pwmfreq(struct device *dev,
> struct device_attribute *attr,
>  {
>   struct adt7475_data *data = adt7475_update_device(dev);
>   struct sensor_device_attribute_2 *sattr =
> to_sensor_dev_attr_2(attr);
> - int i = clamp_val(data->range[sattr->index] & 0xf, 0,
> -   ARRAY_SIZE(pwmfreq_table) - 1);
> + int idx;
> 
>   if (IS_ERR(data))
>   return PTR_ERR(data);
> + idx = clamp_val(data->range[sattr->index] & 0xf, 0,
> + ARRAY_SIZE(pwmfreq_table) - 1);
> 
> - return sprintf(buf, "%d\n", pwmfreq_table[i]);
> + return sprintf(buf, "%d\n", pwmfreq_table[idx]);
>  }
> 
>  static ssize_t set_pwmfreq(struct device *dev, struct device_attribute
> *attr,
> @@ -1004,6 +1005,10 @@ static ssize_t
> pwm_use_point2_pwm_at_crit_show(struct device *dev,
>   char *buf)
>  {
>   struct adt7475_data *data = adt7475_update_device(dev);
> +
> + if (IS_ERR(data))
> + return PTR_ERR(data);
> +
>   return sprintf(buf, "%d\n", !!(data->config4 &
> CONFIG4_MAXDUTY));
>  }
> 

[PATCH 1/2] hwmon: (adt7475) Potential error pointer dereferences

[Dan Carpenter]

The adt7475_update_device() function returns error pointers.  The
problem is that in show_pwmfreq() we dereference it before the check.
And then in pwm_use_point2_pwm_at_crit_show() there isn't a check at
all.  I don't know if it's required, but it silences a static checker
warning and it's doesn't hurt anything to check.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 90837f7c7d0f..16045149f3db 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -962,13 +962,14 @@ static ssize_t show_pwmfreq(struct device *dev, struct 
device_attribute *attr,
 {
struct adt7475_data *data = adt7475_update_device(dev);
struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
-   int i = clamp_val(data->range[sattr->index] & 0xf, 0,
- ARRAY_SIZE(pwmfreq_table) - 1);
+   int idx;
 
if (IS_ERR(data))
return PTR_ERR(data);
+   idx = clamp_val(data->range[sattr->index] & 0xf, 0,
+   ARRAY_SIZE(pwmfreq_table) - 1);
 
-   return sprintf(buf, "%d\n", pwmfreq_table[i]);
+   return sprintf(buf, "%d\n", pwmfreq_table[idx]);
 }
 
 static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
@@ -1004,6 +1005,10 @@ static ssize_t pwm_use_point2_pwm_at_crit_show(struct 
device *dev,
char *buf)
 {
struct adt7475_data *data = adt7475_update_device(dev);
+
+   if (IS_ERR(data))
+   return PTR_ERR(data);
+
return sprintf(buf, "%d\n", !!(data->config4 & CONFIG4_MAXDUTY));
 }