LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* hwmon: Error handling in w83793.c, w83791d.c, w83792d.c
@ 2021-08-11 16:15 Nadezda Lutovinova
  2021-08-11 16:15 ` [PATCH] hwmon: Correct the error " Nadezda Lutovinova
  2021-08-11 17:51 ` hwmon: Error handling in w83793.c, w83791d.c, w83792d.c Guenter Roeck
  0 siblings, 2 replies; 11+ messages in thread
From: Nadezda Lutovinova @ 2021-08-11 16:15 UTC (permalink / raw)
  To: Marc Hulsman
  Cc: Nadezda Lutovinova, Jean Delvare, Guenter Roeck, Rudolf Marek,
	linux-hwmon, linux-kernel, ldv-project

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] hwmon: Correct the error handling in w83793.c, w83791d.c, w83792d.c
  2021-08-11 16:15 hwmon: Error handling in w83793.c, w83791d.c, w83792d.c Nadezda Lutovinova
@ 2021-08-11 16:15 ` Nadezda Lutovinova
  2021-08-11 18:18   ` Guenter Roeck
  2021-08-11 17:51 ` hwmon: Error handling in w83793.c, w83791d.c, w83792d.c Guenter Roeck
  1 sibling, 1 reply; 11+ messages in thread
From: Nadezda Lutovinova @ 2021-08-11 16:15 UTC (permalink / raw)
  To: Marc Hulsman
  Cc: Nadezda Lutovinova, Jean Delvare, Guenter Roeck, Rudolf Marek,
	linux-hwmon, linux-kernel, ldv-project

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: hwmon: Error handling in w83793.c, w83791d.c, w83792d.c
  2021-08-11 16:15 hwmon: Error handling in w83793.c, w83791d.c, w83792d.c Nadezda Lutovinova
  2021-08-11 16:15 ` [PATCH] hwmon: Correct the error " Nadezda Lutovinova
@ 2021-08-11 17:51 ` Guenter Roeck
  2021-08-11 18:19   ` Guenter Roeck
  1 sibling, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2021-08-11 17:51 UTC (permalink / raw)
  To: Nadezda Lutovinova
  Cc: Marc Hulsman, Jean Delvare, Rudolf Marek, linux-hwmon,
	linux-kernel, ldv-project

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hwmon: Correct the error handling in w83793.c, w83791d.c, w83792d.c
  2021-08-11 16:15 ` [PATCH] hwmon: Correct the error " Nadezda Lutovinova
@ 2021-08-11 18:18   ` Guenter Roeck
  2021-09-21 15:51     ` [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field Nadezda Lutovinova
                       ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-08-11 18:18 UTC (permalink / raw)
  To: Nadezda Lutovinova
  Cc: Marc Hulsman, Jean Delvare, Rudolf Marek, linux-hwmon,
	linux-kernel, ldv-project

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: hwmon: Error handling in w83793.c, w83791d.c, w83792d.c
  2021-08-11 17:51 ` hwmon: Error handling in w83793.c, w83791d.c, w83792d.c Guenter Roeck
@ 2021-08-11 18:19   ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-08-11 18:19 UTC (permalink / raw)
  To: Nadezda Lutovinova
  Cc: Marc Hulsman, Jean Delvare, Rudolf Marek, linux-hwmon,
	linux-kernel, ldv-project

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field
  2021-08-11 18:18   ` Guenter Roeck
@ 2021-09-21 15:51     ` Nadezda Lutovinova
  2021-10-02 12:07       ` Guenter Roeck
  2021-09-21 15:51     ` [PATCH v2 2/3] hwmon: (w83792d) " Nadezda Lutovinova
  2021-09-21 15:51     ` [PATCH v2 3/3] hwmon: (w83793) " Nadezda Lutovinova
  2 siblings, 1 reply; 11+ messages in thread
From: Nadezda Lutovinova @ 2021-09-21 15:51 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Nadezda Lutovinova, Marc Hulsman, Rudolf Marek, Jean Delvare,
	linux-hwmon, linux-kernel, ldv-project

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/3] hwmon: (w83792d) Fix NULL pointer dereference by removing unnecessary structure field
  2021-08-11 18:18   ` Guenter Roeck
  2021-09-21 15:51     ` [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field Nadezda Lutovinova
@ 2021-09-21 15:51     ` Nadezda Lutovinova
  2021-10-02 12:12       ` Guenter Roeck
  2021-09-21 15:51     ` [PATCH v2 3/3] hwmon: (w83793) " Nadezda Lutovinova
  2 siblings, 1 reply; 11+ messages in thread
From: Nadezda Lutovinova @ 2021-09-21 15:51 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Nadezda Lutovinova, Marc Hulsman, Rudolf Marek, Jean Delvare,
	linux-hwmon, linux-kernel, ldv-project

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 3/3] hwmon: (w83793) Fix NULL pointer dereference by removing unnecessary structure field
  2021-08-11 18:18   ` Guenter Roeck
  2021-09-21 15:51     ` [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field Nadezda Lutovinova
  2021-09-21 15:51     ` [PATCH v2 2/3] hwmon: (w83792d) " Nadezda Lutovinova
@ 2021-09-21 15:51     ` Nadezda Lutovinova
  2021-10-02 12:15       ` Guenter Roeck
  2 siblings, 1 reply; 11+ messages in thread
From: Nadezda Lutovinova @ 2021-09-21 15:51 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Nadezda Lutovinova, Marc Hulsman, Rudolf Marek, Jean Delvare,
	linux-hwmon, linux-kernel, ldv-project

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field
  2021-09-21 15:51     ` [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field Nadezda Lutovinova
@ 2021-10-02 12:07       ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-10-02 12:07 UTC (permalink / raw)
  To: Nadezda Lutovinova
  Cc: Marc Hulsman, Rudolf Marek, Jean Delvare, linux-hwmon,
	linux-kernel, ldv-project

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;
>  }

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/3] hwmon: (w83792d) Fix NULL pointer dereference by removing unnecessary structure field
  2021-09-21 15:51     ` [PATCH v2 2/3] hwmon: (w83792d) " Nadezda Lutovinova
@ 2021-10-02 12:12       ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-10-02 12:12 UTC (permalink / raw)
  To: Nadezda Lutovinova
  Cc: Marc Hulsman, Rudolf Marek, Jean Delvare, linux-hwmon,
	linux-kernel, ldv-project

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;
>  }

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/3] hwmon: (w83793) Fix NULL pointer dereference by removing unnecessary structure field
  2021-09-21 15:51     ` [PATCH v2 3/3] hwmon: (w83793) " Nadezda Lutovinova
@ 2021-10-02 12:15       ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-10-02 12:15 UTC (permalink / raw)
  To: Nadezda Lutovinova
  Cc: Marc Hulsman, Rudolf Marek, Jean Delvare, linux-hwmon,
	linux-kernel, ldv-project

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;
>  }

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-10-02 12:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11 16:15 hwmon: Error handling in w83793.c, w83791d.c, w83792d.c Nadezda Lutovinova
2021-08-11 16:15 ` [PATCH] hwmon: Correct the error " Nadezda Lutovinova
2021-08-11 18:18   ` Guenter Roeck
2021-09-21 15:51     ` [PATCH v2 1/3] hwmon: (w83791d) Fix NULL pointer dereference by removing unnecessary structure field Nadezda Lutovinova
2021-10-02 12:07       ` Guenter Roeck
2021-09-21 15:51     ` [PATCH v2 2/3] hwmon: (w83792d) " Nadezda Lutovinova
2021-10-02 12:12       ` Guenter Roeck
2021-09-21 15:51     ` [PATCH v2 3/3] hwmon: (w83793) " Nadezda Lutovinova
2021-10-02 12:15       ` Guenter Roeck
2021-08-11 17:51 ` hwmon: Error handling in w83793.c, w83791d.c, w83792d.c Guenter Roeck
2021-08-11 18:19   ` 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).