linux-kernel - Re: [PATCH] hwmon: Fix unsigned 'reg' compared with zero in amc6821_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 11 Nov 2020 06:29:06 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     xiakaixu1987@...il.com, jdelvare@...e.com
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kaixu Xia <kaixuxia@...cent.com>
Subject: Re: [PATCH] hwmon: Fix unsigned 'reg' compared with zero in
 amc6821_update_device

On 11/11/20 12:13 AM, xiakaixu1987@...il.com wrote:
> From: Kaixu Xia <kaixuxia@...cent.com>
> 
> The unsigned variable reg is assigned a return value from the call
> to i2c_smbus_read_byte_data(), which may return negative error code.
> 
> Fixes coccicheck warning:
> 
> ./drivers/hwmon/amc6821.c:215:6-9: WARNING: Unsigned expression compared with zero: reg > 0
> ./drivers/hwmon/amc6821.c:228:6-9: WARNING: Unsigned expression compared with zero: reg > 0
> 
> Reported-by: Tosk Robot <tencent_os_robot@...cent.com>
> Signed-off-by: Kaixu Xia <kaixuxia@...cent.com>
> ---
>  drivers/hwmon/amc6821.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 6b1ce2242c61..ce7c9f412538 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -166,7 +166,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev)
>  	struct amc6821_data *data = dev_get_drvdata(dev);
>  	struct i2c_client *client = data->client;
>  	int timeout = HZ;
> -	u8 reg;
> +	s8 reg;

That is pointless since the return value is not checked. On top of that,
there are many similar assignments to u8 variables, return values
from calls to i2c_smbus_read_byte_data() are never checked in
this function, and the u8 part is actually needed. If you want to
fix it, please go ahead, but please fix the entire driver and please
do it correctly (here: the variable should be 'int' and the return
value should be checked).

Thanks,
Guenter