* [PATCH] hwmon: applesmc: check status earlier.
@ 2020-08-20 13:19 trix
2020-08-20 20:57 ` Henrik Rydberg
2020-08-21 18:33 ` Guenter Roeck
0 siblings, 2 replies; 3+ messages in thread
From: trix @ 2020-08-20 13:19 UTC (permalink / raw)
To: rydberg, jdelvare, linux; +Cc: linux-hwmon, linux-kernel, Tom Rix
From: Tom Rix <trix@redhat.com>
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 <trix@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hwmon: applesmc: check status earlier.
2020-08-20 13:19 [PATCH] hwmon: applesmc: check status earlier trix
@ 2020-08-20 20:57 ` Henrik Rydberg
2020-08-21 18:33 ` Guenter Roeck
1 sibling, 0 replies; 3+ messages in thread
From: Henrik Rydberg @ 2020-08-20 20:57 UTC (permalink / raw)
To: trix, jdelvare, linux; +Cc: linux-hwmon, linux-kernel
Hi Tom,
> From: Tom Rix <trix@redhat.com>
>
> 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 <trix@redhat.com>
> ---
> 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 <rydberg@bitmath.org>
Henrik
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hwmon: applesmc: check status earlier.
2020-08-20 13:19 [PATCH] hwmon: applesmc: check status earlier trix
2020-08-20 20:57 ` Henrik Rydberg
@ 2020-08-21 18:33 ` Guenter Roeck
1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2020-08-21 18:33 UTC (permalink / raw)
To: trix; +Cc: rydberg, jdelvare, linux-hwmon, linux-kernel
On Thu, Aug 20, 2020 at 06:19:32AM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
>
> 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 <trix@redhat.com>
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,
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-21 18:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-20 13:19 [PATCH] hwmon: applesmc: check status earlier trix
2020-08-20 20:57 ` Henrik Rydberg
2020-08-21 18:33 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).