linux-kernel - Re: [PATCH] hwmon: (lm93) Return error values on read failure

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <bdca4f35-ec3e-4fac-bbcf-ed5326feb6f4@roeck-us.net>
Date: Wed, 7 Aug 2024 11:38:34 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Abhishek Tamboli <abhishektamboli9@...il.com>, jdelvare@...e.com
Cc: skhan@...uxfoundation.org, rbmarliere@...il.com,
 linux-kernel-mentees@...ts.linuxfoundation.org, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (lm93) Return error values on read failure

On 8/7/24 11:17, Abhishek Tamboli wrote:
> Fix the issue of lm93_read_byte() and lm93_read_word() return 0 on
> read failure after retries, which could be confused with valid data.
> 
> Address the TODO: what to return in case of error?
> 
> Signed-off-by: Abhishek Tamboli <abhishektamboli9@...il.com>
> ---
>   drivers/hwmon/lm93.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
> index be4853fad80f..b76f3c1c6297 100644
> --- a/drivers/hwmon/lm93.c
> +++ b/drivers/hwmon/lm93.c
> @@ -798,6 +798,7 @@ static unsigned LM93_ALARMS_FROM_REG(struct block1_t b1)
>   static u8 lm93_read_byte(struct i2c_client *client, u8 reg)

This is still returning an u8.

>   {
>   	int value, i;
> +	int ret;
>   
>   	/* retry in case of read errors */
>   	for (i = 1; i <= MAX_RETRIES; i++) {
> @@ -808,14 +809,14 @@ static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
>   			dev_warn(&client->dev,
>   				 "lm93: read byte data failed, address 0x%02x.\n",
>   				 reg);
> +			ret = value;
>   			mdelay(i + 3);
>   		}
>   
>   	}
>   
> -	/* <TODO> what to return in case of error? */
>   	dev_err(&client->dev, "lm93: All read byte retries failed!!\n");

Those messages only make sense if there is no error return.

> -	return 0;
> +	return ret;

This is pointless and actually dangerous unless the calling code actually checks
the return value and aborts on error.



>   }
>   
>   static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
> @@ -836,6 +837,7 @@ static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
>   static u16 lm93_read_word(struct i2c_client *client, u8 reg)
>   {
>   	int value, i;
> +	int ret;
>   
>   	/* retry in case of read errors */
>   	for (i = 1; i <= MAX_RETRIES; i++) {
> @@ -846,14 +848,14 @@ static u16 lm93_read_word(struct i2c_client *client, u8 reg)
>   			dev_warn(&client->dev,
>   				 "lm93: read word data failed, address 0x%02x.\n",
>   				 reg);
> +			ret = value;
>   			mdelay(i + 3);
>   		}
>   
>   	}
>   
> -	/* <TODO> what to return in case of error? */
>   	dev_err(&client->dev, "lm93: All read word retries failed!!\n");
> -	return 0;
> +	return ret;

Same as above.

Actually, your patch makes the problem worse because the errors are still ignored
and at the same time report more or less random values to the user (the error code
truncated to an unsigned 8 or 16 bit value).

Is this just a blind patch, submitted as kind of an exercise, or do you have an
actual use case for this driver ? The driver is in such bad shape that it should
be left alone unless someone actually needs it and is able to test any changes.
Otherwise changes like this just increase risk (or, rather, make it even worse)
without real benefit.

Thanks,
Guenter