Re: [PATCH 3/5] hwmon: Add a generic interface for userspace implementations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hwmon: O(N^2) attribute validation loop causes CPU soft lockup
- [High] sysfs: 1-byte buffer overflow in hwmon_attr_show_string due to PAGE_SIZE limit
- [Medium] driver-core: invalid teardown order in uhwmon_exit() destroys parent before children
--

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

    hwmon: Add a generic interface for userspace implementations
    
    Add /dev/uhwmon so userspace drivers can expose standard hwmon
    attributes. Use the shared hwmon identifiers directly and forward
    numeric values or labels through read and write requests.

> 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_create(struct file *file, void __user *arg)
> +{
> [ ... ]
> +	for (i = 0; i < data->num_attrs; i++) {
> +		struct uhwmon_attr *attr = &data->attrs[i];
> +		u32 type;
> +		bool string;
> +
> +		ret = -EINVAL;
> +		if (spec[i].attr >= 32 ||
> +		    !hwmon_is_valid_attr(spec[i].type, spec[i].attr) ||
> +		    !spec[i].mode || spec[i].mode & ~0644 ||
> +		    spec[i].flags & ~UHWMON_ATTR_CONSTANT)
> +			goto out;
> +		if (spec[i].type == hwmon_chip && spec[i].channel)
> +			goto out;
> +		/*
> +		 * N is a zero-based channel index: channels 0..N need at least
> +		 * N + 1 attribute slots, plus a NULL terminator. The core uses
> +		 * int counts, so N + 2 <= INT_MAX. The full count is checked below.
> +		 */
> +		if (spec[i].channel > INT_MAX - 2) {
> +			ret = -EOVERFLOW;
> +			goto out;
> +		}
> +		string = hwmon_is_string_attr(spec[i].type, spec[i].attr);
> +		if (string) {
> +			if (!spec[i].size || spec[i].size > PAGE_SIZE ||
> +			    spec[i].mode & 0222 || spec[i].value)
> +				goto out;

[Severity: High]
Does this PAGE_SIZE check leave enough room for the newline character appended
by the hwmon core?

If a daemon writes a string of length PAGE_SIZE - 1 plus a null terminator,
when read via sysfs, the hwmon core passes this string to
sprintf(buf, "%s\n", s) where buf is exactly PAGE_SIZE bytes.

This would write PAGE_SIZE - 1 characters, followed by the newline and null
terminator, totaling PAGE_SIZE + 1 bytes.

Could this cause a 1-byte overflow and memory corruption?

> +		} else if (spec[i].size || spec[i].text) {
> +			goto out;
> +		}
> [ ... ]
> +		type = spec[i].type == hwmon_energy64 ? hwmon_energy :
> +							spec[i].type;
> +		for (j = 0; j < i; j++) {
> +			u32 other_type = spec[j].type == hwmon_energy64 ?
> +						 hwmon_energy :
> +						 spec[j].type;
> +
> +			if (type == other_type &&
> +			    spec[i].attr == spec[j].attr &&
> +			    spec[i].channel == spec[j].channel)
> +				goto out;
> +		}

[Severity: High]
Can this nested loop cause a CPU soft lockup?

Since uhwmon_create() accepts a user-provided num_attrs bounded only by
kmalloc limits (which could exceed 100,000 attributes depending on max slab
size), this loop compares every attribute against all preceding attributes to
prevent duplicates.

For a large number of attributes, this results in billions of iterations.

As the loop lacks preemption points like cond_resched(), could it hold the
CPU exclusively for multiple seconds and trigger the soft lockup watchdog?

> +		attr->spec = spec[i];
> +		attr->spec.text = 0;
> +		counts[spec[i].type] =
> +			max(counts[spec[i].type], spec[i].channel + 1);
> +	}
> [ ... ]
> +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 to deregister the parent miscdevice before unregistering the child
hwmon devices?

Calling misc_deregister() destroys uhwmon_misc.this_device, which is the
parent of all registered hwmon devices. Destroying a parent device before its
children violates the driver core's topological ordering and lifecycle rules.

Could this cause sysfs warnings and potential use-after-free when the children
subsequently attempt to unregister their sysfs directories from the
already-deleted parent?

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