Re: [PATCH 5/5] docs: hwmon: Document the uhwmon userspace interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] hwmon: uhwmon: userfaultfd-based unkillable hangs via copy_to_user under mutex
- [High] hwmon: uhwmon: name collision logic blocks multiple devices and schema updates
- [High] hwmon: wait_event_killable() in uhwmon_request() allows daemon to hang sysfs
- [Medium] hwmon: uhwmon: uhwmon_exit() destroys parent miscdevice before child hwmon devices
--

commit b2f6043289b80f507b7c7bd6ff84bc7129c9ba48
Author: Jihong Min <hurryman2212@gmail.com>

docs: hwmon: Document the uhwmon userspace interface

This commit introduces the uhwmon userspace API documentation, which connects
userspace drivers to hwmon_ops through the /dev/uhwmon control device. It
details how developers can register devices, handle requests, notify changes,
and manage device lifetimes.

> diff --git a/drivers/hwmon/uhwmon.c b/drivers/hwmon/uhwmon.c
> new file mode 100644
> index 0000000000000..1855c87f06d9d
> --- /dev/null
> +++ b/drivers/hwmon/uhwmon.c
> @@ -0,0 +1,641 @@

[ ... ]

> +static int uhwmon_request(struct uhwmon_device *data, unsigned int index,
> +			  u32 op, s64 *value)
> +{

[ ... ]

> +	ret = wait_event_interruptible(data->wait, READ_ONCE(pending.done));
> +	mutex_lock(&data->lock);
> +	if (!pending.done && !data->readable) {
> +		/* Once delivered, only fatal signals may abort the wait. */
> +		mutex_unlock(&data->lock);
> +		ret = wait_event_killable(data->wait, READ_ONCE(pending.done));
> +		mutex_lock(&data->lock);
> +	}

[Severity: High]
Does wait_event_killable() here allow the daemon to hang sysfs?

Because TASK_KILLABLE only responds to fatal signals, monitoring utilities
(like sensors or prometheus-node-exporter) that catch SIGTERM or SIGINT will
become unkillable and fail to shut down cleanly, delaying system reboots by
up to 90 seconds.

Any user with sysfs access can read the sensor, and any bug in the daemon
can trigger the hang. Although the daemon requires root to open /dev/uhwmon,
a buggy daemon shouldn't cause unkillable kernel deadlocks.

[ ... ]

> +static int uhwmon_create(struct file *file, void __user *arg)
> +{
> +	struct uhwmon_device *data, *existing;

[ ... ]

> +	data = kzalloc_obj(*data);
> +	if (!data)
> +		return -ENOMEM;
> +	mutex_init(&data->lock);
> +	init_waitqueue_head(&data->wait);
> +	data->name = strndup_user(u64_to_user_ptr(create.name), NAME_MAX + 1);

[Severity: High]
Could this strndup_user() combined with memdup_array_user() below cause
an unkillable hang?

uhwmon_create() is called from uhwmon_ioctl() which holds the global
uhwmon_control_lock. If a daemon issues an ioctl(UHWMON_CREATE) providing a
buffer backed by userfaultfd, the kernel will block indefinitely while holding
uhwmon_control_lock.

This freezes all device creation, notification, and release, causing a local
denial of service.

[ ... ]

> +	list_for_each_entry(existing, &uhwmon_devices, node) {
> +		if (strcmp(existing->name, data->name))
> +			continue;
> +		ret = -EBUSY;
> +		if (existing->online)
> +			goto out;
> +		ret = -EINVAL;
> +		if (!uhwmon_same_schema(existing, data))
> +			goto out;
> +		mutex_lock(&existing->lock);

[Severity: High]
Is it possible that this rigid name-matching logic incorrectly rejects valid
registrations?

It appears to block multiple independent devices from sharing the same name by
returning -EBUSY, breaking standard hwmon functionality.

Additionally, if a daemon crashes (leaving the device offline) and restarts
with a modified schema, UHWMON_CREATE returns -EINVAL without attaching the
file descriptor.

This permanently locks out the device name because the daemon cannot issue
UHWMON_DESTROY to clean up the old offline device.

[ ... ]

> +static ssize_t uhwmon_read(struct file *file, char __user *buf, size_t count,
> +			   loff_t *offset)
> +{

[ ... ]

> +		if (mutex_lock_interruptible(&data->lock))
> +			return -ERESTARTSYS;

[ ... ]

> +	ret = -EFAULT;
> +	if (copy_to_user(buf, &data->pending->request,
> +			 sizeof(data->pending->request)))
> +		goto out;
> +	data->readable = false;
> +	ret = sizeof(data->pending->request);
> +out:
> +	mutex_unlock(&data->lock);

[Severity: High]
Can this copy_to_user() while holding data->lock allow userfaultfd to block
indefinitely?

If a daemon issues a read() providing a buffer backed by userfaultfd, the
kernel will block indefinitely while data->lock is held.

Since hwmon core serializes sysfs accesses, any standard monitoring utility
(like sensors) reading the sysfs attribute will invoke uhwmon_request(),
which calls mutex_lock(&data->lock) and blocks in TASK_UNINTERRUPTIBLE
(D-state).

Standard kernel design strictly forbids holding standard mutexes across
userspace memory accesses if those mutexes are shared with system-wide reader
processes.

[ ... ]

> +static ssize_t uhwmon_write(struct file *file, const char __user *buf,
> +			    size_t count, loff_t *offset)
> +{

[ ... ]

> +	mutex_lock(&data->lock);

[ ... ]

> +		if (size) {
> +			if (count < sizeof(reply) + size)
> +				goto out;
> +			ret = -EFAULT;
> +			if (copy_from_user(data->text, buf + sizeof(reply),
> +					   size))
> +				goto out;

[Severity: High]
Similar to the issue in uhwmon_read(), could this copy_from_user() while
holding data->lock also allow a userfaultfd-based unkillable hang?

> +			length = strnlen(data->text, size);
> +			ret = -EINVAL;
> +			if (length == size)
> +				goto out;

[Severity: High]
What happens if an invalid string reply is provided here, does it permanently
strand the sysfs reader?

If the daemon sends a non-null-terminated string, length == size evaluates
to true and the function branches to goto out without setting
pending->done = true.

This permanently strands the sysfs reader in the killable wait from
uhwmon_request().

Since hwmon core serializes sysfs accesses, this single hung reader deadlocks
the entire hwmon device.

[ ... ]

> +static void __exit uhwmon_exit(void)
> +{
> +	struct uhwmon_device *data, *next;
> +
> +	misc_deregister(&uhwmon_misc);
> +	list_for_each_entry_safe(data, next, &uhwmon_devices, node) {
> +		uhwmon_unregister(data);
> +		list_del(&data->node);
> +		uhwmon_free(data);
> +	}
> +}

[Severity: Medium]
Is it safe for uhwmon_exit() to destroy the parent miscdevice before the child
hwmon devices?

uhwmon_create() registers child hwmon devices using uhwmon_misc.this_device
as their parent. These child devices persist even after the userspace daemon
closes the control file descriptor.

During module unload, calling misc_deregister(&uhwmon_misc) before iterating
the uhwmon_devices list to unregister the child hwmon devices leaves
dangling child kobjects.

The driver core strictly requires children to be unregistered before their
parent. Unregistering the parent first triggers sysfs warnings and
use-after-free bugs when the children are subsequently unregistered.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791531025.git.hurryman2212@gmail.com?part=5