Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 25 Jul 2019 09:46:53 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>
Cc:     r.marek@assembler.cz, jdelvare@suse.com,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: w83793: Fix possible null-pointer dereferences in
 watchdog_open()
Message-ID: <20190725164653.GB11220@roeck-us.net>
References: <20190725084156.15554-1-baijiaju1990@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190725084156.15554-1-baijiaju1990@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>