[PATCH] hwmon: (fschmd) avoid client->dev dereference in watchdog_release

[PATCH] hwmon: (fschmd) avoid client->dev dereference in watchdog_release

[Chen-Shi-Hong]

fschmd_remove() can detach the i2c client while the watchdog device
is still open.

If watchdog_release() later takes the unexpected-close path, it calls
watchdog_trigger() and then logs the event with dev_crit() using
data->client->dev. However, the client may already have been cleared
by the remove path.

watchdog_trigger() already checks whether data->client is still valid
and returns -ENODEV if the client is gone. Use its return value to
decide whether to emit the message, and log with watchdog_name instead
of dereferencing data->client->dev.

This avoids relying on a device pointer that may no longer be valid
during release after device removal.

Signed-off-by: Chen-Shi-Hong <eric039eric@gmail.com>
---
 drivers/hwmon/fschmd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/fschmd.c b/drivers/hwmon/fschmd.c
index 1211fa2259e5..d1e7fc72e81f 100644
--- a/drivers/hwmon/fschmd.c
+++ b/drivers/hwmon/fschmd.c
@@ -834,9 +834,9 @@ static int watchdog_release(struct inode *inode, struct file *filp)
 		watchdog_stop(data);
 		data->watchdog_expect_close = 0;
 	} else {
-		watchdog_trigger(data);
-		dev_crit(&data->client->dev,
-			"unexpected close, not stopping watchdog!\n");
+		if (!watchdog_trigger(data))
+			pr_crit("%s: unexpected close, not stopping watchdog!\n",
+				data->watchdog_name);
 	}
 
 	clear_bit(0, &data->watchdog_is_open);
-- 
2.53.0

Re: [PATCH] hwmon: (fschmd) avoid client->dev dereference in watchdog_release

[sashiko-bot]

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

Pre-existing issues:
- [High] A race condition in `watchdog_open()` allows concurrent module removal to cause an immediate NULL pointer dereference.
- [High] A TOCTOU race condition in `fschmd_remove()` allows concurrent watchdog operations to restart the hardware watchdog right before the driver detaches, causing an unpreventable system reboot.
- [Medium] `watchdog_trigger()` and `watchdog_stop()` silently ignore the error return value from `i2c_smbus_write_byte_data()`, masking hardware failures.
- [Medium] I2C read failures are silently truncated to `u8` and blindly written back to hardware registers, causing silent corruption.
--

commit 39762c6966aa166ac2bc648c9e46734689cef97a
Author: Chen-Shi-Hong <eric039eric@gmail.com>

hwmon: (fschmd) avoid client->dev dereference in watchdog_release
    
fschmd_remove() can detach the i2c client while the watchdog device
is still open. If watchdog_release() later takes the unexpected-close path,
it logs the event with dev_crit using data->client->dev, which could trigger
a use-after-free or NULL dereference. This patch changes the logging to use
pr_crit with data->watchdog_name instead.

> diff --git a/drivers/hwmon/fschmd.c b/drivers/hwmon/fschmd.c
> index 1211fa2259e52..d1e7fc72e81f9 100644
> --- a/drivers/hwmon/fschmd.c
> +++ b/drivers/hwmon/fschmd.c
> @@ -834,9 +834,9 @@ static int watchdog_release(struct inode *inode, struct file *filp)
>  		watchdog_stop(data);
>  		data->watchdog_expect_close = 0;
>  	} else {
> -		watchdog_trigger(data);
> -		dev_crit(&data->client->dev,
> -			"unexpected close, not stopping watchdog!\n");
> +		if (!watchdog_trigger(data))

[Severity: Medium]
This isn't a bug introduced by this patch, but watchdog_trigger() and
watchdog_stop() silently ignore the error return value from
i2c_smbus_write_byte_data().

If i2c_smbus_write_byte_data() returns a negative error code (e.g., -EIO) on 
hardware failure, these functions still return 0 (success) as long as 
data->client is valid.

Does this mask hardware failures? If ping commands consistently fail silently,
will the hardware watchdog eventually time out and cause an unexpected system 
reset while the userspace daemon assumes everything is fine?

> +			pr_crit("%s: unexpected close, not stopping watchdog!\n",
> +				data->watchdog_name);
>  	}
>  
>  	clear_bit(0, &data->watchdog_is_open);

[Severity: Medium]
This isn't a bug introduced by this patch, but I2C read failures appear to be
silently truncated to u8 and blindly written back to hardware registers,
causing silent corruption.

In store functions such as alert_led_store() and fan_div_store():

drivers/hwmon/fschmd.c:alert_led_store() {
...
	reg = i2c_smbus_read_byte_data(to_i2c_client(dev), FSCHMD_REG_CONTROL);

	if (v)
		reg |= FSCHMD_CONTROL_ALERT_LED;
	else
		reg &= ~FSCHMD_CONTROL_ALERT_LED;

	i2c_smbus_write_byte_data(to_i2c_client(dev), FSCHMD_REG_CONTROL, reg);
...
}

If i2c_smbus_read_byte_data() fails, it returns a negative error code like
-EIO (-5). Truncating -5 to u8 results in 0xFB. The code then performs
bitwise logic on this garbage value and writes it back to the hardware
register via i2c_smbus_write_byte_data().

Does this corrupt the hardware state (and explicitly modify reserved bits in 
fan_div_store()) instead of propagating the I/O error to userspace?

[Severity: High]
This is a pre-existing issue, but there appears to be a race condition in 
watchdog_open() that allows concurrent module removal to cause an immediate 
NULL pointer dereference.

drivers/hwmon/fschmd.c:watchdog_open() {
	struct fschmd_data *pos, *data = NULL;
...
	if (!mutex_trylock(&watchdog_data_mutex))
		return -ERESTARTSYS;
	list_for_each_entry(pos, &watchdog_data_list, list) {
		if (pos->watchdog_miscdev.minor == iminor(inode)) {
			data = pos;
			break;
		}
	}
	/* Note we can never not have found data, so we don't check for this */
	watchdog_is_open = test_and_set_bit(0, &data->watchdog_is_open);
...
}

If VFS misc_open() has already resolved the minor and dropped misc_mtx, can a 
concurrent fschmd_remove() successfully call misc_deregister() and 
list_del(&data->list) before watchdog_open() acquires watchdog_data_mutex?

If this sequence happens, watchdog_open() will iterate the list, fail to find
the minor, and exit the loop with data = NULL. It will then unconditionally
call test_and_set_bit(0, &data->watchdog_is_open), triggering a kernel panic.

[Severity: High]
This isn't a bug introduced by this patch, but there is a TOCTOU race condition
in fschmd_remove() that allows concurrent watchdog operations to restart the 
hardware watchdog right before the driver detaches, causing an unpreventable 
system reboot.

When removing the device, fschmd_remove() calls watchdog_stop() to halt the 
hardware watchdog if it is open:

drivers/hwmon/fschmd.c:fschmd_remove() {
...
		if (data->watchdog_is_open) {
...
			watchdog_stop(data);
		}
		mutex_lock(&watchdog_data_mutex);
		list_del(&data->list);
		mutex_unlock(&watchdog_data_mutex);
		/* Tell the watchdog code the client is gone */
		mutex_lock(&data->watchdog_lock);
		data->client = NULL;
		mutex_unlock(&data->watchdog_lock);
...
}

The watchdog_stop() function acquires data->watchdog_lock, stops the hardware,
and then releases the lock.

Because the lock is dropped between stopping the watchdog and invalidating the 
client, is it possible for a concurrent watchdog_write() (ping) to interleave
and execute watchdog_trigger()? 

Since data->client is still valid at that exact moment, this would restart the
hardware watchdog. fschmd_remove() then proceeds to set data->client = NULL.
Any subsequent cleanup attempts will fail to stop the hardware watchdog because
watchdog_stop() immediately returns -ENODEV if data->client is NULL, leaving 
the hardware watchdog ticking until a hard system reset.

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

Re: [PATCH] hwmon: (fschmd) avoid client->dev dereference in watchdog_release

[Guenter Roeck]

On 5/22/26 22:10, Chen-Shi-Hong wrote:
> fschmd_remove() can detach the i2c client while the watchdog device
> is still open.
> 
> If watchdog_release() later takes the unexpected-close path, it calls
> watchdog_trigger() and then logs the event with dev_crit() using
> data->client->dev. However, the client may already have been cleared
> by the remove path.
> 
> watchdog_trigger() already checks whether data->client is still valid
> and returns -ENODEV if the client is gone. Use its return value to
> decide whether to emit the message, and log with watchdog_name instead
> of dereferencing data->client->dev.
> 
> This avoids relying on a device pointer that may no longer be valid
> during release after device removal.
> 

That driver has lots of problems, which would best be fixed by converting its
watchdog driver to a real watchdog driver and actually fixing the other real
problems, such as missing i2c error checks or the real existing race conditions.
This patch is like bandaging a blemish on a finger while ignoring that the leg
is half torn off.

Please, please, please, stop sending such patches. You bury the real important
fixes in the noise.

Guenter

> Signed-off-by: Chen-Shi-Hong <eric039eric@gmail.com>
> ---
>   drivers/hwmon/fschmd.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/fschmd.c b/drivers/hwmon/fschmd.c
> index 1211fa2259e5..d1e7fc72e81f 100644
> --- a/drivers/hwmon/fschmd.c
> +++ b/drivers/hwmon/fschmd.c
> @@ -834,9 +834,9 @@ static int watchdog_release(struct inode *inode, struct file *filp)
>   		watchdog_stop(data);
>   		data->watchdog_expect_close = 0;
>   	} else {
> -		watchdog_trigger(data);
> -		dev_crit(&data->client->dev,
> -			"unexpected close, not stopping watchdog!\n");
> +		if (!watchdog_trigger(data))
> +			pr_crit("%s: unexpected close, not stopping watchdog!\n",
> +				data->watchdog_name);
>   	}
>   
>   	clear_bit(0, &data->watchdog_is_open);

Re: [PATCH] hwmon: (fschmd) avoid client->dev dereference in watchdog_release

[Chen-Shi-Hong]

Hi Guenter,

Understood, and thank you for the direct feedback.

I see your point that this patch does not address the real problems in
the driver and only adds noise. I won't pursue this kind of change for
fschmd further.

Thanks,
Chen-Shi-Hong