hwmon: Error handling in w83793.c, w83791d.c, w83792d.c

hwmon: Error handling in w83793.c, w83791d.c, w83792d.c

[Nadezda Lutovinova]

In w83793_detect_subclients(): if driver read tmp value sufficient for 
(tmp & 0x08) && (!(tmp & 0x80)) && ((tmp & 0x7) == ((tmp >> 4) & 0x7))
from device then Null pointer dereference occurs.
(It is possible if tmp = 0b0xyz1xyz, where same chars mean same numbers).

It can be fixed just by adding a checking for null pointer, patch for 
this is in the next letter. But a question arised:
The array w83793_data->lm75 is used once in this function after switching 
to devm_i2c_new_dummy_device() instead of i2c_new_dummy(). And this 
function is called once (from w83793_probe()). Maybe this array should be 
deleted from struct w83793_data?

The same situation in w83791d.c and in w83792d.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>

[PATCH] hwmon: Correct the error handling in w83793.c, w83791d.c, w83792d.c

[Nadezda Lutovinova]

If driver read tmp (or val) value sufficient for 
(tmp & 0x08) && (!(tmp & 0x80)) && ((tmp & 0x7) == ((tmp >> 4) & 0x7))
from device then Null pointer dereference occurs. 
(It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)

The patch adds checking if data->lm75[0] is NULL.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>
---
 drivers/hwmon/w83791d.c | 2 +-
 drivers/hwmon/w83792d.c | 2 +-
 drivers/hwmon/w83793.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/w83791d.c b/drivers/hwmon/w83791d.c
index 37b25a1474c4..8b30bbfafaa7 100644
--- a/drivers/hwmon/w83791d.c
+++ b/drivers/hwmon/w83791d.c
@@ -1284,7 +1284,7 @@ static int w83791d_detect_subclients(struct i2c_client *client)
 		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
 							  0x48 + (val & 0x7));
 	if (!(val & 0x80)) {
-		if (!IS_ERR(data->lm75[0]) &&
+		if (!IS_ERR_OR_NULL(data->lm75[0]) &&
 				((val & 0x7) == ((val >> 4) & 0x7))) {
 			dev_err(&client->dev,
 				"duplicate addresses 0x%x, "
diff --git a/drivers/hwmon/w83792d.c b/drivers/hwmon/w83792d.c
index abd5c3a722b9..85ae12d950e1 100644
--- a/drivers/hwmon/w83792d.c
+++ b/drivers/hwmon/w83792d.c
@@ -950,7 +950,7 @@ w83792d_detect_subclients(struct i2c_client *new_client)
 		data->lm75[0] = devm_i2c_new_dummy_device(&new_client->dev, adapter,
 							  0x48 + (val & 0x7));
 	if (!(val & 0x80)) {
-		if (!IS_ERR(data->lm75[0]) &&
+		if (!IS_ERR_OR_NULL(data->lm75[0]) &&
 			((val & 0x7) == ((val >> 4) & 0x7))) {
 			dev_err(&new_client->dev,
 				"duplicate addresses 0x%x, use force_subclient\n",
diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
index e7d0484eabe4..9d8c44e2fa6e 100644
--- a/drivers/hwmon/w83793.c
+++ b/drivers/hwmon/w83793.c
@@ -1590,7 +1590,7 @@ w83793_detect_subclients(struct i2c_client *client)
 		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
 							  0x48 + (tmp & 0x7));
 	if (!(tmp & 0x80)) {
-		if (!IS_ERR(data->lm75[0])
+		if (!IS_ERR_OR_NULL(data->lm75[0])
 		    && ((tmp & 0x7) == ((tmp >> 4) & 0x7))) {
 			dev_err(&client->dev,
 				"duplicate addresses 0x%x, "
-- 
2.17.1

Re: [PATCH] hwmon: Correct the error handling in w83793.c, w83791d.c, w83792d.c

[Guenter Roeck]

On Wed, Aug 11, 2021 at 07:15:15PM +0300, Nadezda Lutovinova wrote:
> If driver read tmp (or val) value sufficient for 
> (tmp & 0x08) && (!(tmp & 0x80)) && ((tmp & 0x7) == ((tmp >> 4) & 0x7))
> from device then Null pointer dereference occurs. 
> (It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)
> 
> The patch adds checking if data->lm75[0] is NULL.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 

One patch per driver, please.

> Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>
> ---
>  drivers/hwmon/w83791d.c | 2 +-
>  drivers/hwmon/w83792d.c | 2 +-
>  drivers/hwmon/w83793.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/w83791d.c b/drivers/hwmon/w83791d.c
> index 37b25a1474c4..8b30bbfafaa7 100644
> --- a/drivers/hwmon/w83791d.c
> +++ b/drivers/hwmon/w83791d.c
> @@ -1284,7 +1284,7 @@ static int w83791d_detect_subclients(struct i2c_client *client)
>  		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
>  							  0x48 + (val & 0x7));
>  	if (!(val & 0x80)) {
> -		if (!IS_ERR(data->lm75[0]) &&
> +		if (!IS_ERR_OR_NULL(data->lm75[0]) &&
>  				((val & 0x7) == ((val >> 4) & 0x7))) {
>  			dev_err(&client->dev,
>  				"duplicate addresses 0x%x, "

As you pointed out in te other e-mail, lm75[] does not really serve 
a purpose anymore. It might be much better to replace this code with
something like

	if (!(val & 0x88) && (val & 0x7) == ((val >> 4) & 0x7)) {
		dev_err(&new_client->dev,
		        "duplicate addresses 0x%x, use force_subclient\n",
                        0x48 + (val & 0x7));
		return -ENODEV;
	}

Same for the other chips.

Guenter

> diff --git a/drivers/hwmon/w83792d.c b/drivers/hwmon/w83792d.c
> index abd5c3a722b9..85ae12d950e1 100644
> --- a/drivers/hwmon/w83792d.c
> +++ b/drivers/hwmon/w83792d.c
> @@ -950,7 +950,7 @@ w83792d_detect_subclients(struct i2c_client *new_client)
>  		data->lm75[0] = devm_i2c_new_dummy_device(&new_client->dev, adapter,
>  							  0x48 + (val & 0x7));
>  	if (!(val & 0x80)) {
> -		if (!IS_ERR(data->lm75[0]) &&
> +		if (!IS_ERR_OR_NULL(data->lm75[0]) &&
>  			((val & 0x7) == ((val >> 4) & 0x7))) {
>  			dev_err(&new_client->dev,
>  				"duplicate addresses 0x%x, use force_subclient\n",
> diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
> index e7d0484eabe4..9d8c44e2fa6e 100644
> --- a/drivers/hwmon/w83793.c
> +++ b/drivers/hwmon/w83793.c
> @@ -1590,7 +1590,7 @@ w83793_detect_subclients(struct i2c_client *client)
>  		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
>  							  0x48 + (tmp & 0x7));
>  	if (!(tmp & 0x80)) {
> -		if (!IS_ERR(data->lm75[0])
> +		if (!IS_ERR_OR_NULL(data->lm75[0])
>  		    && ((tmp & 0x7) == ((tmp >> 4) & 0x7))) {
>  			dev_err(&client->dev,
>  				"duplicate addresses 0x%x, "
> -- 
> 2.17.1
> 

[PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field

[Nadezda Lutovinova]

If driver read val value sufficient for 
(val & 0x08) && (!(val & 0x80)) && ((val & 0x7) == ((val >> 4) & 0x7))
from device then Null pointer dereference occurs. 
(It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)
Also lm75[] does not serve a purpose anymore after switching to
devm_i2c_new_dummy_device() in w83791d_detect_subclients().

The patch fixes possible NULL pointer dereference by removing lm75[].

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>
---
v2: 
 - split one file per patch 
 - remove lm75[] instead of adding checking  
---
 drivers/hwmon/w83791d.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/w83791d.c b/drivers/hwmon/w83791d.c
index 37b25a1474c4..b4eae45859c1 100644
--- a/drivers/hwmon/w83791d.c
+++ b/drivers/hwmon/w83791d.c
@@ -273,9 +273,6 @@ struct w83791d_data {
 	char valid;			/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 
-	/* array of 2 pointers to subclients */
-	struct i2c_client *lm75[2];
-
 	/* volts */
 	u8 in[NUMBER_OF_VIN];		/* Register value */
 	u8 in_max[NUMBER_OF_VIN];	/* Register value */
@@ -1257,7 +1254,6 @@ static const struct attribute_group w83791d_group_fanpwm45 = {
 static int w83791d_detect_subclients(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
-	struct w83791d_data *data = i2c_get_clientdata(client);
 	int address = client->addr;
 	int i, id;
 	u8 val;
@@ -1280,21 +1276,21 @@ static int w83791d_detect_subclients(struct i2c_client *client)
 	}
 
 	val = w83791d_read(client, W83791D_REG_I2C_SUBADDR);
+
+	if (!(val & 0x88) && (val & 0x7) == ((val >> 4) & 0x7)) {
+		dev_err(&client->dev,
+			"duplicate addresses 0x%x, use force_subclient\n",
+				0x48 + (val & 0x7));
+		return -ENODEV;
+	}
+
 	if (!(val & 0x08))
-		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
-							  0x48 + (val & 0x7));
-	if (!(val & 0x80)) {
-		if (!IS_ERR(data->lm75[0]) &&
-				((val & 0x7) == ((val >> 4) & 0x7))) {
-			dev_err(&client->dev,
-				"duplicate addresses 0x%x, "
-				"use force_subclient\n",
-				data->lm75[0]->addr);
-			return -ENODEV;
-		}
-		data->lm75[1] = devm_i2c_new_dummy_device(&client->dev, adapter,
-							  0x48 + ((val >> 4) & 0x7));
-	}
+		devm_i2c_new_dummy_device(&client->dev, adapter,
+						0x48 + (val & 0x7));
+
+	if (!(val & 0x80))
+		devm_i2c_new_dummy_device(&client->dev, adapter,
+						0x48 + ((val >> 4) & 0x7));
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field

[Guenter Roeck]

On Tue, Sep 21, 2021 at 06:51:51PM +0300, Nadezda Lutovinova wrote:
> If driver read val value sufficient for 
> (val & 0x08) && (!(val & 0x80)) && ((val & 0x7) == ((val >> 4) & 0x7))
> from device then Null pointer dereference occurs. 
> (It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)
> Also lm75[] does not serve a purpose anymore after switching to
> devm_i2c_new_dummy_device() in w83791d_detect_subclients().
> 
> The patch fixes possible NULL pointer dereference by removing lm75[].
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>

Applied, after fixing multi-line alignments.

Guenter

> ---
> v2: 
>  - split one file per patch 
>  - remove lm75[] instead of adding checking  
> ---
>  drivers/hwmon/w83791d.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/w83791d.c b/drivers/hwmon/w83791d.c
> index 37b25a1474c4..b4eae45859c1 100644
> --- a/drivers/hwmon/w83791d.c
> +++ b/drivers/hwmon/w83791d.c
> @@ -273,9 +273,6 @@ struct w83791d_data {
>  	char valid;			/* !=0 if following fields are valid */
>  	unsigned long last_updated;	/* In jiffies */
>  
> -	/* array of 2 pointers to subclients */
> -	struct i2c_client *lm75[2];
> -
>  	/* volts */
>  	u8 in[NUMBER_OF_VIN];		/* Register value */
>  	u8 in_max[NUMBER_OF_VIN];	/* Register value */
> @@ -1257,7 +1254,6 @@ static const struct attribute_group w83791d_group_fanpwm45 = {
>  static int w83791d_detect_subclients(struct i2c_client *client)
>  {
>  	struct i2c_adapter *adapter = client->adapter;
> -	struct w83791d_data *data = i2c_get_clientdata(client);
>  	int address = client->addr;
>  	int i, id;
>  	u8 val;
> @@ -1280,21 +1276,21 @@ static int w83791d_detect_subclients(struct i2c_client *client)
>  	}
>  
>  	val = w83791d_read(client, W83791D_REG_I2C_SUBADDR);
> +
> +	if (!(val & 0x88) && (val & 0x7) == ((val >> 4) & 0x7)) {
> +		dev_err(&client->dev,
> +			"duplicate addresses 0x%x, use force_subclient\n",
> +				0x48 + (val & 0x7));
> +		return -ENODEV;
> +	}
> +
>  	if (!(val & 0x08))
> -		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
> -							  0x48 + (val & 0x7));
> -	if (!(val & 0x80)) {
> -		if (!IS_ERR(data->lm75[0]) &&
> -				((val & 0x7) == ((val >> 4) & 0x7))) {
> -			dev_err(&client->dev,
> -				"duplicate addresses 0x%x, "
> -				"use force_subclient\n",
> -				data->lm75[0]->addr);
> -			return -ENODEV;
> -		}
> -		data->lm75[1] = devm_i2c_new_dummy_device(&client->dev, adapter,
> -							  0x48 + ((val >> 4) & 0x7));
> -	}
> +		devm_i2c_new_dummy_device(&client->dev, adapter,
> +						0x48 + (val & 0x7));
> +
> +	if (!(val & 0x80))
> +		devm_i2c_new_dummy_device(&client->dev, adapter,
> +						0x48 + ((val >> 4) & 0x7));
>  
>  	return 0;
>  }

[PATCH v2 2/3] hwmon: (w83792d) Fix NULL pointer dereference by removing unnecessary structure field

[Nadezda Lutovinova]

If driver read val value sufficient for 
(val & 0x08) && (!(val & 0x80)) && ((val & 0x7) == ((val >> 4) & 0x7))
from device then Null pointer dereference occurs. 
(It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)
Also lm75[] does not serve a purpose anymore after switching to
devm_i2c_new_dummy_device() in w83791d_detect_subclients().

The patch fixes possible NULL pointer dereference by removing lm75[].

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>
---
v2: 
 - split one file per patch 
 - remove lm75[] instead of adding checking  
---
 drivers/hwmon/w83792d.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/w83792d.c b/drivers/hwmon/w83792d.c
index abd5c3a722b9..8a72be4ad74f 100644
--- a/drivers/hwmon/w83792d.c
+++ b/drivers/hwmon/w83792d.c
@@ -264,9 +264,6 @@ struct w83792d_data {
 	char valid;		/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 
-	/* array of 2 pointers to subclients */
-	struct i2c_client *lm75[2];
-
 	u8 in[9];		/* Register value */
 	u8 in_max[9];		/* Register value */
 	u8 in_min[9];		/* Register value */
@@ -927,7 +924,6 @@ w83792d_detect_subclients(struct i2c_client *new_client)
 	int address = new_client->addr;
 	u8 val;
 	struct i2c_adapter *adapter = new_client->adapter;
-	struct w83792d_data *data = i2c_get_clientdata(new_client);
 
 	id = i2c_adapter_id(adapter);
 	if (force_subclients[0] == id && force_subclients[1] == address) {
@@ -946,20 +942,21 @@ w83792d_detect_subclients(struct i2c_client *new_client)
 	}
 
 	val = w83792d_read_value(new_client, W83792D_REG_I2C_SUBADDR);
+
+	if (!(val & 0x88) && (val & 0x7) == ((val >> 4) & 0x7)) {
+		dev_err(&new_client->dev,
+			"duplicate addresses 0x%x, use force_subclient\n",
+				0x48 + (val & 0x7));
+		return -ENODEV;
+	}
+
 	if (!(val & 0x08))
-		data->lm75[0] = devm_i2c_new_dummy_device(&new_client->dev, adapter,
-							  0x48 + (val & 0x7));
-	if (!(val & 0x80)) {
-		if (!IS_ERR(data->lm75[0]) &&
-			((val & 0x7) == ((val >> 4) & 0x7))) {
-			dev_err(&new_client->dev,
-				"duplicate addresses 0x%x, use force_subclient\n",
-				data->lm75[0]->addr);
-			return -ENODEV;
-		}
-		data->lm75[1] = devm_i2c_new_dummy_device(&new_client->dev, adapter,
-							  0x48 + ((val >> 4) & 0x7));
-	}
+		devm_i2c_new_dummy_device(&new_client->dev, adapter,
+						0x48 + (val & 0x7));
+
+	if (!(val & 0x80))
+		devm_i2c_new_dummy_device(&new_client->dev, adapter,
+						0x48 + ((val >> 4) & 0x7));
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v2 2/3] hwmon: (w83792d) Fix NULL pointer dereference by removing unnecessary structure field

[Guenter Roeck]

On Tue, Sep 21, 2021 at 06:51:52PM +0300, Nadezda Lutovinova wrote:
> If driver read val value sufficient for 
> (val & 0x08) && (!(val & 0x80)) && ((val & 0x7) == ((val >> 4) & 0x7))
> from device then Null pointer dereference occurs. 
> (It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)
> Also lm75[] does not serve a purpose anymore after switching to
> devm_i2c_new_dummy_device() in w83791d_detect_subclients().
> 
> The patch fixes possible NULL pointer dereference by removing lm75[].
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>

Applied, after fixing multi-line alignments

> ---
> v2: 
>  - split one file per patch 
>  - remove lm75[] instead of adding checking  
> ---
>  drivers/hwmon/w83792d.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hwmon/w83792d.c b/drivers/hwmon/w83792d.c
> index abd5c3a722b9..8a72be4ad74f 100644
> --- a/drivers/hwmon/w83792d.c
> +++ b/drivers/hwmon/w83792d.c
> @@ -264,9 +264,6 @@ struct w83792d_data {
>  	char valid;		/* !=0 if following fields are valid */
>  	unsigned long last_updated;	/* In jiffies */
>  
> -	/* array of 2 pointers to subclients */
> -	struct i2c_client *lm75[2];
> -
>  	u8 in[9];		/* Register value */
>  	u8 in_max[9];		/* Register value */
>  	u8 in_min[9];		/* Register value */
> @@ -927,7 +924,6 @@ w83792d_detect_subclients(struct i2c_client *new_client)
>  	int address = new_client->addr;
>  	u8 val;
>  	struct i2c_adapter *adapter = new_client->adapter;
> -	struct w83792d_data *data = i2c_get_clientdata(new_client);
>  
>  	id = i2c_adapter_id(adapter);
>  	if (force_subclients[0] == id && force_subclients[1] == address) {
> @@ -946,20 +942,21 @@ w83792d_detect_subclients(struct i2c_client *new_client)
>  	}
>  
>  	val = w83792d_read_value(new_client, W83792D_REG_I2C_SUBADDR);
> +
> +	if (!(val & 0x88) && (val & 0x7) == ((val >> 4) & 0x7)) {
> +		dev_err(&new_client->dev,
> +			"duplicate addresses 0x%x, use force_subclient\n",
> +				0x48 + (val & 0x7));
> +		return -ENODEV;
> +	}
> +
>  	if (!(val & 0x08))
> -		data->lm75[0] = devm_i2c_new_dummy_device(&new_client->dev, adapter,
> -							  0x48 + (val & 0x7));
> -	if (!(val & 0x80)) {
> -		if (!IS_ERR(data->lm75[0]) &&
> -			((val & 0x7) == ((val >> 4) & 0x7))) {
> -			dev_err(&new_client->dev,
> -				"duplicate addresses 0x%x, use force_subclient\n",
> -				data->lm75[0]->addr);
> -			return -ENODEV;
> -		}
> -		data->lm75[1] = devm_i2c_new_dummy_device(&new_client->dev, adapter,
> -							  0x48 + ((val >> 4) & 0x7));
> -	}
> +		devm_i2c_new_dummy_device(&new_client->dev, adapter,
> +						0x48 + (val & 0x7));
> +
> +	if (!(val & 0x80))
> +		devm_i2c_new_dummy_device(&new_client->dev, adapter,
> +						0x48 + ((val >> 4) & 0x7));
>  
>  	return 0;
>  }

[PATCH v2 3/3] hwmon: (w83793) Fix NULL pointer dereference by removing unnecessary structure field

[Nadezda Lutovinova]

If driver read tmp value sufficient for 
(tmp & 0x08) && (!(tmp & 0x80)) && ((tmp & 0x7) == ((tmp >> 4) & 0x7))
from device then Null pointer dereference occurs. 
(It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)
Also lm75[] does not serve a purpose anymore after switching to
devm_i2c_new_dummy_device() in w83791d_detect_subclients().

The patch fixes possible NULL pointer dereference by removing lm75[].

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>
---
v2: 
 - split one file per patch 
 - remove lm75[] instead of adding checking  
---
 drivers/hwmon/w83793.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
index e7d0484eabe4..4ee96756ed49 100644
--- a/drivers/hwmon/w83793.c
+++ b/drivers/hwmon/w83793.c
@@ -202,7 +202,6 @@ static inline s8 TEMP_TO_REG(long val, s8 min, s8 max)
 }
 
 struct w83793_data {
-	struct i2c_client *lm75[2];
 	struct device *hwmon_dev;
 	struct mutex update_lock;
 	char valid;			/* !=0 if following fields are valid */
@@ -1566,7 +1565,6 @@ w83793_detect_subclients(struct i2c_client *client)
 	int address = client->addr;
 	u8 tmp;
 	struct i2c_adapter *adapter = client->adapter;
-	struct w83793_data *data = i2c_get_clientdata(client);
 
 	id = i2c_adapter_id(adapter);
 	if (force_subclients[0] == id && force_subclients[1] == address) {
@@ -1586,20 +1584,21 @@ w83793_detect_subclients(struct i2c_client *client)
 	}
 
 	tmp = w83793_read_value(client, W83793_REG_I2C_SUBADDR);
+
+	if (!(tmp & 0x88) && (tmp & 0x7) == ((tmp >> 4) & 0x7)) {
+		dev_err(&client->dev,
+			"duplicate addresses 0x%x, use force_subclient\n",
+				0x48 + (tmp & 0x7));
+		return -ENODEV;
+	}
+
 	if (!(tmp & 0x08))
-		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
-							  0x48 + (tmp & 0x7));
-	if (!(tmp & 0x80)) {
-		if (!IS_ERR(data->lm75[0])
-		    && ((tmp & 0x7) == ((tmp >> 4) & 0x7))) {
-			dev_err(&client->dev,
-				"duplicate addresses 0x%x, "
-				"use force_subclients\n", data->lm75[0]->addr);
-			return -ENODEV;
-		}
-		data->lm75[1] = devm_i2c_new_dummy_device(&client->dev, adapter,
-							  0x48 + ((tmp >> 4) & 0x7));
-	}
+		devm_i2c_new_dummy_device(&client->dev, adapter,
+						0x48 + (tmp & 0x7));
+
+	if (!(tmp & 0x80))
+		devm_i2c_new_dummy_device(&client->dev, adapter,
+						0x48 + ((tmp >> 4) & 0x7));
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v2 3/3] hwmon: (w83793) Fix NULL pointer dereference by removing unnecessary structure field

[Guenter Roeck]

On Tue, Sep 21, 2021 at 06:51:53PM +0300, Nadezda Lutovinova wrote:
> If driver read tmp value sufficient for 
> (tmp & 0x08) && (!(tmp & 0x80)) && ((tmp & 0x7) == ((tmp >> 4) & 0x7))
> from device then Null pointer dereference occurs. 
> (It is possible if tmp = 0b0xyz1xyz, where same literals mean same numbers)
> Also lm75[] does not serve a purpose anymore after switching to
> devm_i2c_new_dummy_device() in w83791d_detect_subclients().
> 
> The patch fixes possible NULL pointer dereference by removing lm75[].
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>

Applied, after fixing multi-line alignments

Thanks,
Guenter

> ---
> v2: 
>  - split one file per patch 
>  - remove lm75[] instead of adding checking  
> ---
>  drivers/hwmon/w83793.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
> index e7d0484eabe4..4ee96756ed49 100644
> --- a/drivers/hwmon/w83793.c
> +++ b/drivers/hwmon/w83793.c
> @@ -202,7 +202,6 @@ static inline s8 TEMP_TO_REG(long val, s8 min, s8 max)
>  }
>  
>  struct w83793_data {
> -	struct i2c_client *lm75[2];
>  	struct device *hwmon_dev;
>  	struct mutex update_lock;
>  	char valid;			/* !=0 if following fields are valid */
> @@ -1566,7 +1565,6 @@ w83793_detect_subclients(struct i2c_client *client)
>  	int address = client->addr;
>  	u8 tmp;
>  	struct i2c_adapter *adapter = client->adapter;
> -	struct w83793_data *data = i2c_get_clientdata(client);
>  
>  	id = i2c_adapter_id(adapter);
>  	if (force_subclients[0] == id && force_subclients[1] == address) {
> @@ -1586,20 +1584,21 @@ w83793_detect_subclients(struct i2c_client *client)
>  	}
>  
>  	tmp = w83793_read_value(client, W83793_REG_I2C_SUBADDR);
> +
> +	if (!(tmp & 0x88) && (tmp & 0x7) == ((tmp >> 4) & 0x7)) {
> +		dev_err(&client->dev,
> +			"duplicate addresses 0x%x, use force_subclient\n",
> +				0x48 + (tmp & 0x7));
> +		return -ENODEV;
> +	}
> +
>  	if (!(tmp & 0x08))
> -		data->lm75[0] = devm_i2c_new_dummy_device(&client->dev, adapter,
> -							  0x48 + (tmp & 0x7));
> -	if (!(tmp & 0x80)) {
> -		if (!IS_ERR(data->lm75[0])
> -		    && ((tmp & 0x7) == ((tmp >> 4) & 0x7))) {
> -			dev_err(&client->dev,
> -				"duplicate addresses 0x%x, "
> -				"use force_subclients\n", data->lm75[0]->addr);
> -			return -ENODEV;
> -		}
> -		data->lm75[1] = devm_i2c_new_dummy_device(&client->dev, adapter,
> -							  0x48 + ((tmp >> 4) & 0x7));
> -	}
> +		devm_i2c_new_dummy_device(&client->dev, adapter,
> +						0x48 + (tmp & 0x7));
> +
> +	if (!(tmp & 0x80))
> +		devm_i2c_new_dummy_device(&client->dev, adapter,
> +						0x48 + ((tmp >> 4) & 0x7));
>  
>  	return 0;
>  }

Re: hwmon: Error handling in w83793.c, w83791d.c, w83792d.c

[Guenter Roeck]

On Wed, Aug 11, 2021 at 07:15:14PM +0300, Nadezda Lutovinova wrote:
> In w83793_detect_subclients(): if driver read tmp value sufficient for 
> (tmp & 0x08) && (!(tmp & 0x80)) && ((tmp & 0x7) == ((tmp >> 4) & 0x7))
> from device then Null pointer dereference occurs.
> (It is possible if tmp = 0b0xyz1xyz, where same chars mean same numbers).
> 
> It can be fixed just by adding a checking for null pointer, patch for 
> this is in the next letter. But a question arised:
> The array w83793_data->lm75 is used once in this function after switching 
> to devm_i2c_new_dummy_device() instead of i2c_new_dummy(). And this 
> function is called once (from w83793_probe()). Maybe this array should be 
> deleted from struct w83793_data?
> 

That is part of it. However, the entire code is wrong. There is no need
to check for I2C address overlap in the first place, and the i2c address
for the second 'virtual' chip should start with 0x4c, not with 0x48.
See w83793g/w83793ag datasheet, section 8.3.2.2.
I assume the code was copied from w83791d and w83792d, where the addresses
can indeed overlap.

Guenter

> The same situation in w83791d.c and in w83792d.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>

Re: hwmon: Error handling in w83793.c, w83791d.c, w83792d.c

[Guenter Roeck]

On Wed, Aug 11, 2021 at 10:52:03AM -0700, Guenter Roeck wrote:
> On Wed, Aug 11, 2021 at 07:15:14PM +0300, Nadezda Lutovinova wrote:
> > In w83793_detect_subclients(): if driver read tmp value sufficient for 
> > (tmp & 0x08) && (!(tmp & 0x80)) && ((tmp & 0x7) == ((tmp >> 4) & 0x7))
> > from device then Null pointer dereference occurs.
> > (It is possible if tmp = 0b0xyz1xyz, where same chars mean same numbers).
> > 
> > It can be fixed just by adding a checking for null pointer, patch for 
> > this is in the next letter. But a question arised:
> > The array w83793_data->lm75 is used once in this function after switching 
> > to devm_i2c_new_dummy_device() instead of i2c_new_dummy(). And this 
> > function is called once (from w83793_probe()). Maybe this array should be 
> > deleted from struct w83793_data?
> > 
> 
> That is part of it. However, the entire code is wrong. There is no need
> to check for I2C address overlap in the first place, and the i2c address
> for the second 'virtual' chip should start with 0x4c, not with 0x48.
> See w83793g/w83793ag datasheet, section 8.3.2.2.

Wait, that is wrong. Those are just the initial register values.
Forget the noise above; sorry for the confusion.

Guenter

> I assume the code was copied from w83791d and w83792d, where the addresses
> can indeed overlap.
> 
> Guenter
> 
> > The same situation in w83791d.c and in w83792d.
> > 
> > Found by Linux Driver Verification project (linuxtesting.org).
> > 
> > Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru>