linux-kernel - Re: [PATCH v3] hwmon: lm75: Handle broken device nodes gracefully

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:   Tue, 2 Feb 2021 11:29:45 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     "Matwey V. Kornilov" <matwey@....msu.ru>,
        Jean Delvare <jdelvare@...e.com>,
        Javier Martinez Canillas <javier@....samsung.com>,
        "open list:HARDWARE MONITORING" <linux-hwmon@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Cc:     matwey.kornilov@...il.com
Subject: Re: [PATCH v3] hwmon: lm75: Handle broken device nodes gracefully

On 2/2/21 10:37 AM, Matwey V. Kornilov wrote:
> There is a logical flaw in lm75_probe() function introduced in
> 
>     commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> 
> Note, that of_device_get_match_data() returns NULL when no match
> is found. This is the case when OF node exists but has unknown
> compatible line, while the module is still loaded via i2c
> detection.
> 
> arch/powerpc/boot/dts/fsl/p2041rdb.dts:
> 
>     lm75b@48 {
>     	compatible = "nxp,lm75a";
>     	reg = <0x48>;
>     };
> 
> In this case, the sensor is mistakenly considered as ADT75 variant.
> 
> Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> Signed-off-by: Matwey V. Kornilov <matwey@....msu.ru>

Looks good, but we'll also need a solution for the existing devicetree
nodes since they are being used. The best solution would probably
be to document and add "nxp,lm75a" to the list of devicetree nodes.
The same applies to other used but not documented compatible strings
(I think you sent a list earlier if I recall correctly).

Sorry that I didn't bring this up earlier, but I am concerned that
if we don't do this, this patch might be considered a regression.

Thanks,
Guenter

> ---
>  Changes since v2:
>  * fixed typo in the message
>  * fixed brackets
> 
>  drivers/hwmon/lm75.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index e447febd121a..53882c334a0d 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -561,10 +561,17 @@ static int lm75_probe(struct i2c_client *client)
>  	int status, err;
>  	enum lm75_type kind;
>  
> -	if (client->dev.of_node)
> -		kind = (enum lm75_type)of_device_get_match_data(&client->dev);
> -	else
> +	if (dev->of_node) {
> +		const struct of_device_id *match =
> +			of_match_device(dev->driver->of_match_table, dev);
> +
> +		if (!match)
> +			return -ENODEV;
> +
> +		kind = (enum lm75_type)(match->data);
> +	} else {
>  		kind = i2c_match_id(lm75_ids, client)->driver_data;
> +	}
>  
>  	if (!i2c_check_functionality(client->adapter,
>  			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
>