2020-08-20 13:23:13

by Tom Rix

Subject: [PATCH] hwmon: applesmc: check status earlier.

From: Tom Rix <[email protected]>

clang static analysis reports this representative problem

applesmc.c:758:10: warning: 1st function call argument is an
uninitialized value
left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

buffer is filled by the earlier call

ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, ...

This problem is reported because a goto skips the status check.
Other similar problems use data from applesmc_read_key before checking
the status. So move the checks to before the use.

Signed-off-by: Tom Rix <[email protected]>
---
drivers/hwmon/applesmc.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 316618409315..a18887990f4a 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -753,15 +753,18 @@ static ssize_t applesmc_light_show(struct device *dev,
}

ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, buffer, data_length);
+ if (ret)
+ goto out;
/* newer macbooks report a single 10-bit bigendian value */
if (data_length == 10) {
left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
goto out;
}
left = buffer[2];
+
+ ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
if (ret)
goto out;
- ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
right = buffer[2];

out:
@@ -810,12 +813,11 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
to_index(attr));

ret = applesmc_read_key(newkey, buffer, 2);
- speed = ((buffer[0] << 8 | buffer[1]) >> 2);
-
if (ret)
return ret;
- else
- return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
+
+ speed = ((buffer[0] << 8 | buffer[1]) >> 2);
+ return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
}

static ssize_t applesmc_store_fan_speed(struct device *dev,
@@ -851,12 +853,11 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
u8 buffer[2];

ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
- manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
-
if (ret)
return ret;
- else
- return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
+
+ manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
+ return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
}

static ssize_t applesmc_store_fan_manual(struct device *dev,
@@ -872,10 +873,11 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
return -EINVAL;

ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
- val = (buffer[0] << 8 | buffer[1]);
if (ret)
goto out;

+ val = (buffer[0] << 8 | buffer[1]);
+
if (input)
val = val | (0x01 << to_index(attr));
else
@@ -951,13 +953,12 @@ static ssize_t applesmc_key_count_show(struct device *dev,
u32 count;

ret = applesmc_read_key(KEY_COUNT_KEY, buffer, 4);
- count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
- ((u32)buffer[2]<<8) + buffer[3];
-
if (ret)
return ret;
- else
- return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
+
+ count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
+ ((u32)buffer[2]<<8) + buffer[3];
+ return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
}

static ssize_t applesmc_key_at_index_read_show(struct device *dev,
--
2.18.1

2020-08-20 22:00:27

by Henrik Rydberg

Subject: Re: [PATCH] hwmon: applesmc: check status earlier.

Hi Tom,

> From: Tom Rix <[email protected]>
>
> clang static analysis reports this representative problem
>
> applesmc.c:758:10: warning: 1st function call argument is an
> uninitialized value
> left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> buffer is filled by the earlier call
>
> ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, ...
>
> This problem is reported because a goto skips the status check.
> Other similar problems use data from applesmc_read_key before checking
> the status. So move the checks to before the use.
>
> Signed-off-by: Tom Rix <[email protected]>
> ---
> drivers/hwmon/applesmc.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 316618409315..a18887990f4a 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -753,15 +753,18 @@ static ssize_t applesmc_light_show(struct device *dev,
> }
>
> ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, buffer, data_length);
> + if (ret)
> + goto out;
> /* newer macbooks report a single 10-bit bigendian value */
> if (data_length == 10) {
> left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
> goto out;
> }
> left = buffer[2];
> +
> + ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
> if (ret)
> goto out;
> - ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
> right = buffer[2];
>
> out:
> @@ -810,12 +813,11 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
> to_index(attr));
>
> ret = applesmc_read_key(newkey, buffer, 2);
> - speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> -
> if (ret)
> return ret;
> - else
> - return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
> +
> + speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> + return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
> }
>
> static ssize_t applesmc_store_fan_speed(struct device *dev,
> @@ -851,12 +853,11 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
> u8 buffer[2];
>
> ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> - manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
> -
> if (ret)
> return ret;
> - else
> - return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
> +
> + manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
> + return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
> }
>
> static ssize_t applesmc_store_fan_manual(struct device *dev,
> @@ -872,10 +873,11 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
> return -EINVAL;
>
> ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> - val = (buffer[0] << 8 | buffer[1]);
> if (ret)
> goto out;
>
> + val = (buffer[0] << 8 | buffer[1]);
> +
> if (input)
> val = val | (0x01 << to_index(attr));
> else
> @@ -951,13 +953,12 @@ static ssize_t applesmc_key_count_show(struct device *dev,
> u32 count;
>
> ret = applesmc_read_key(KEY_COUNT_KEY, buffer, 4);
> - count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
> - ((u32)buffer[2]<<8) + buffer[3];
> -
> if (ret)
> return ret;
> - else
> - return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
> +
> + count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
> + ((u32)buffer[2]<<8) + buffer[3];
> + return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
> }
>
> static ssize_t applesmc_key_at_index_read_show(struct device *dev,
>

Looks good, thank you.

Reviewed-by: Henrik Rydberg <[email protected]>

Henrik

2020-08-21 18:34:21

by Guenter Roeck

Subject: Re: [PATCH] hwmon: applesmc: check status earlier.

On Thu, Aug 20, 2020 at 06:19:32AM -0700, [email protected] wrote:
> From: Tom Rix <[email protected]>
>
> clang static analysis reports this representative problem
>
> applesmc.c:758:10: warning: 1st function call argument is an
> uninitialized value
> left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> buffer is filled by the earlier call
>
> ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, ...
>
> This problem is reported because a goto skips the status check.
> Other similar problems use data from applesmc_read_key before checking
> the status. So move the checks to before the use.
>
> Signed-off-by: Tom Rix <[email protected]>

Applied.

Thanks,
Guenter

> ---
> drivers/hwmon/applesmc.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 316618409315..a18887990f4a 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -753,15 +753,18 @@ static ssize_t applesmc_light_show(struct device *dev,
> }
>
> ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, buffer, data_length);
> + if (ret)
> + goto out;
> /* newer macbooks report a single 10-bit bigendian value */
> if (data_length == 10) {
> left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
> goto out;
> }
> left = buffer[2];
> +
> + ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
> if (ret)
> goto out;
> - ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
> right = buffer[2];
>
> out:
> @@ -810,12 +813,11 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
> to_index(attr));
>
> ret = applesmc_read_key(newkey, buffer, 2);
> - speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> -
> if (ret)
> return ret;
> - else
> - return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
> +
> + speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> + return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
> }
>
> static ssize_t applesmc_store_fan_speed(struct device *dev,
> @@ -851,12 +853,11 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
> u8 buffer[2];
>
> ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> - manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
> -
> if (ret)
> return ret;
> - else
> - return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
> +
> + manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
> + return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
> }
>
> static ssize_t applesmc_store_fan_manual(struct device *dev,
> @@ -872,10 +873,11 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
> return -EINVAL;
>
> ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> - val = (buffer[0] << 8 | buffer[1]);
> if (ret)
> goto out;
>
> + val = (buffer[0] << 8 | buffer[1]);
> +
> if (input)
> val = val | (0x01 << to_index(attr));
> else
> @@ -951,13 +953,12 @@ static ssize_t applesmc_key_count_show(struct device *dev,
> u32 count;
>
> ret = applesmc_read_key(KEY_COUNT_KEY, buffer, 4);
> - count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
> - ((u32)buffer[2]<<8) + buffer[3];
> -
> if (ret)
> return ret;
> - else
> - return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
> +
> + count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
> + ((u32)buffer[2]<<8) + buffer[3];
> + return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
> }
>
> static ssize_t applesmc_key_at_index_read_show(struct device *dev,