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]
Message-ID: <20190725164653.GB11220@roeck-us.net>
Date:   Thu, 25 Jul 2019 09:46:53 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Jia-Ju Bai <baijiaju1990@...il.com>
Cc:     r.marek@...embler.cz, jdelvare@...e.com,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: w83793: Fix possible null-pointer dereferences in
 watchdog_open()

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@...il.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
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ