Re: [PATCH] hwmon: (w83793) release probe data through kref

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a NULL pointer dereference in watchdog_close() when logging unexpected close after device removal.
- [High] This problem wasn't introduced by this patch, but there is a data race and hardware bank corruption due to inconsistent locking between hwmon and watchdog.
--

commit 62e1b0f8c7da574fa35b600e27a4223020469380
Author: Guangshuo Li <lgs201920130244@gmail.com>

hwmon: (w83793) release probe data through kref

w83793_probe() manages driver data lifetime through kref, but the probe
error path frees data directly with kfree(). This drops the initial
reference without a matching kref_put(), leaving reference accounting
unbalanced. This patch drops the probe-owned reference with kref_put()
instead and lets w83793_release_resources() perform the final free.

> diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
> index a548586369e1d..c6ef04c69856e 100644
> --- a/drivers/hwmon/w83793.c
> +++ b/drivers/hwmon/w83793.c
> @@ -1928,7 +1928,9 @@ static int w83793_probe(struct i2c_client *client)
>  	for (i = 0; i < ARRAY_SIZE(w83793_temp); i++)
>  		device_remove_file(dev, &w83793_temp[i].dev_attr);
>  free_mem:
> -	kfree(data);
> +	mutex_lock(&watchdog_data_mutex);
> +	kref_put(&data->kref, w83793_release_resources);
> +	mutex_unlock(&watchdog_data_mutex);
>  exit:
>  	return err;
>  }

[Severity: High]
This problem wasn't introduced by this patch, but there is a NULL pointer
dereference in watchdog_close() when logging unexpected close after device
removal.

If the w83793 device is unbound or removed while the /dev/watchdog file
descriptor is open, w83793_remove() clears data->client to NULL:

drivers/hwmon/w83793.c:w83793_remove() {
    ...
		/* Tell the watchdog code the client is gone */
		mutex_lock(&data->watchdog_lock);
		data->client = NULL;
		mutex_unlock(&data->watchdog_lock);
    ...
}

When the user subsequently closes the file descriptor without writing the
'V' magic close character, watchdog_close() executes the else branch and
calls dev_crit(&data->client->dev, ...) without verifying if data->client
is still valid:

drivers/hwmon/w83793.c:watchdog_close() {
    ...
	} else {
		watchdog_trigger(data);
		dev_crit(&data->client->dev,
			"unexpected close, not stopping watchdog!\n");
	}
    ...
}

Can this lead to a kernel panic (Oops) due to a NULL pointer dereference
when the unguarded data->client is accessed during teardown?


[Severity: High]
This problem wasn't introduced by this patch, but there is a data race and
hardware bank corruption due to inconsistent locking between hwmon and
watchdog.

The W83793 chip uses a paged register system selected via W83793_REG_BANKSEL.
Hwmon sysfs accesses use mutex_lock(&data->update_lock) before calling
w83793_write_value(), which modifies data->bank and updates the active chip
bank over I2C:

drivers/hwmon/w83793.c:w83793_write_value() {
    ...
	new_bank |= data->bank & 0xfc;
	if (data->bank != new_bank) {
		res = i2c_smbus_write_byte_data(client, W83793_REG_BANKSEL,
						new_bank);
    ...
		data->bank = new_bank;
	}
    ...
}

However, watchdog functions perform I2C operations via w83793_write_value()
while holding ONLY data->watchdog_lock:

drivers/hwmon/w83793.c:watchdog_disable() {
    ...
	mutex_lock(&data->watchdog_lock);
	if (!data->client) {
		ret = -ENODEV;
		goto leave;
	}

	/* Disable Soft Watchdog */
	w83793_write_value(data->client, W83793_REG_WDT_LOCK, 0xAA);
    ...
}

Will concurrent execution of a watchdog operation and a hwmon sysfs
read/write race on data->bank and the hardware BANKSEL register? Does this
allow operations from one context to be inadvertently executed on the bank
switched by the other context, corrupting hardware registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914073638.1662500-1-lgs201920130244@gmail.com?part=1