Re: [PATCH] hwmon: (pmbus/core) notify on the hwmon device, not the i2c client

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

Pre-existing issues:
- [High] Type confusion in pmbus_notify() causes out-of-bounds read and dropped hardware alarms.
--

--- Patch [1]: [PATCH] hwmon: (pmbus/core) notify on the hwmon device, not the i2c client ---
commit 4976d3d0a1b77e2b4e30c18177904c9cbbe066a2
Author: Vincent Jardin <[email protected]>

hwmon: (pmbus/core) notify on the hwmon device, not the i2c client

This commit fixes the event notification mechanism in pmbus_notify by
redirecting sysfs_notify and kobject_uevent from the i2c client's kobject
to the hwmon class device. This ensures poll waiters and udev listeners are
properly awakened on SMBALERT faults.

Cc: [email protected] # v6.4+

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 3143b9e0316c4..0081f16c3a95a 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2985,8 +2985,9 @@ static void pmbus_notify(struct pmbus_data *data, int page, int reg, int flags)
>  
>  		if (reg == sreg && page == spage && (smask & flags)) {
>  			dev_dbg(data->dev, "sysfs notify: %s", da->attr.name);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential type
confusion issue here while reviewing the notification logic.

Does the loop in pmbus_notify() safely handle heterogeneous attribute types?

Looking at the start of the loop in pmbus_notify(), it unconditionally casts
every attribute to a struct sensor_device_attribute:

drivers/hwmon/pmbus/pmbus_core.c:pmbus_notify() {
        for (i = 0; i < data->num_attributes; i++) {
                struct device_attribute *da = to_dev_attr(data->group.attrs[i]);
                struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
                int index = attr->index;
...
}

However, data->group.attrs can contain other types like struct
pmbus_samples_reg or struct pmbus_sensor, which only embed a base
struct device_attribute.

If da is a struct pmbus_samples_reg, dev_attr is the last member. Casting
it to struct sensor_device_attribute and reading the index field appears
to access memory past the end of the allocation, which might trigger a
slab-out-of-bounds read.

Additionally, if da is a struct pmbus_sensor, casting it causes the index
field to overlap with the page, phase, and reg fields. Could this produce
a garbage mask on little-endian systems that spuriously matches the target
reg, page, and flags during an alert?

If so, it seems this could prematurely clear bits from flags via
flags &= ~smask and silently drop legitimate hardware alarms before
reaching the actual boolean alarm attribute.

> -			sysfs_notify(&data->dev->kobj, NULL, da->attr.name);
> -			kobject_uevent(&data->dev->kobj, KOBJ_CHANGE);
> +			sysfs_notify(&data->hwmon_dev->kobj, NULL,
> +				     da->attr.name);
> +			kobject_uevent(&data->hwmon_dev->kobj, KOBJ_CHANGE);
>  			flags &= ~smask;
>  		}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1