Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] hwmon: w83793: Fix possible null-pointer dereferences in watchdog_open()
@ 2019-07-25  8:41 Jia-Ju Bai
  2019-07-25 16:46 ` Guenter Roeck
  0 siblings, 1 reply; 2+ messages in thread
From: Jia-Ju Bai @ 2019-07-25  8:41 UTC (permalink / raw)
  To: r.marek, jdelvare, linux; +Cc: linux-hwmon, linux-kernel, Jia-Ju Bai

In watchdog_open(), data is initialized as NULL.
After the loop "list_for_each_entry" on lines 1302-1307, 
data may not be assigned, thus data is still NULL.

In this case, data is used on line 1310:
    watchdog_is_open = test_and_set_bit(0, &data->watchdog_is_open);
and on line 1317:
    kref_get(&data->kref);
and on line 1326:
    watchdog_enable(data);

Thus, possible null-pointer dereferences may occur.

To fix these bugs, data is checked after the loop.
If it is NULL, the mutex lock is released and -EINVAL is returned.

These bugs are found by a static analysis tool STCheck written by us.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/hwmon/w83793.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
index 46f5dfec8d0a..f299716d5d94 100644
--- a/drivers/hwmon/w83793.c
+++ b/drivers/hwmon/w83793.c
@@ -1306,6 +1306,11 @@ static int watchdog_open(struct inode *inode, struct file *filp)
 		}
 	}
 
+	if (!data) {
+		mutex_unlock(&watchdog_data_mutex);
+		return -EINVAL;
+	}
+
 	/* Check, if device is already open */
 	watchdog_is_open = test_and_set_bit(0, &data->watchdog_is_open);
 
-- 
2.17.0


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

* Re: [PATCH] hwmon: w83793: Fix possible null-pointer dereferences in watchdog_open()
  2019-07-25  8:41 [PATCH] hwmon: w83793: Fix possible null-pointer dereferences in watchdog_open() Jia-Ju Bai
@ 2019-07-25 16:46 ` Guenter Roeck
  0 siblings, 0 replies; 2+ messages in thread
From: Guenter Roeck @ 2019-07-25 16:46 UTC (permalink / raw)
  To: Jia-Ju Bai; +Cc: r.marek, jdelvare, linux-hwmon, linux-kernel

On Thu, Jul 25, 2019 at 04:41:56PM +0800, Jia-Ju Bai wrote:
> In watchdog_open(), data is initialized as NULL.
> After the loop "list_for_each_entry" on lines 1302-1307, 
> data may not be assigned, thus data is still NULL.
> 
> In this case, data is used on line 1310:
>     watchdog_is_open = test_and_set_bit(0, &data->watchdog_is_open);
> and on line 1317:
>     kref_get(&data->kref);
> and on line 1326:
>     watchdog_enable(data);
> 
> Thus, possible null-pointer dereferences may occur.
> 
> To fix these bugs, data is checked after the loop.
> If it is NULL, the mutex lock is released and -EINVAL is returned.
> 
> These bugs are found by a static analysis tool STCheck written by us.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>

In practice this can't happen because the device can only be opened
if the device node exists (because otherwise there is nothing to
open). The potential race condition is addressed with watchdog_data_mutex,
which ensures that the device is added to watchdog_data_list by the
time the open function is called.

I understand this is tricky, but in situations like this it would really
be better to rework the driver completely. It should use the watchdog core,
and the hwmon driver part is in equally bad shape and should at least use
hwmon_device_register_with_groups(). That is quite unlikely to happen,
given the age of the chip. As such, it is better to leave the driver alone
unless something is really broken with it (ie breakage is observed, meaning
someone is actually using it).

Thanks,
Guenter

> ---
>  drivers/hwmon/w83793.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
> index 46f5dfec8d0a..f299716d5d94 100644
> --- a/drivers/hwmon/w83793.c
> +++ b/drivers/hwmon/w83793.c
> @@ -1306,6 +1306,11 @@ static int watchdog_open(struct inode *inode, struct file *filp)
>  		}
>  	}
>  
> +	if (!data) {
> +		mutex_unlock(&watchdog_data_mutex);
> +		return -EINVAL;
> +	}
> +
>  	/* Check, if device is already open */
>  	watchdog_is_open = test_and_set_bit(0, &data->watchdog_is_open);
>  
> -- 
> 2.17.0
> 

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

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25  8:41 [PATCH] hwmon: w83793: Fix possible null-pointer dereferences in watchdog_open() Jia-Ju Bai
2019-07-25 16:46 ` Guenter Roeck

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox