Re: [PATCH v2] hwmon: occ: validate poll response sensor blocks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Repeatedly toggling the `occ_active` sysfs attribute leaks memory due to `devm_kcalloc` allocations.
- [High] Validation of sensor block size is insufficient to prevent OOB reads because it does not verify `sensor_length` against the expected C structure sizes used by sysfs readers.
- [High] Lockless reads of the response buffer create a data race leading to torn reads for multi-byte sensor values.
--

--- Patch [1]: [PATCH v2] hwmon: occ: validate poll response sensor blocks ---
commit 04905329085b910b887e74baa72bc72f52624e45
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

hwmon: occ: validate poll response sensor blocks

The OCC poll response parser walks a counted list of sensor data blocks.
It used the static backing-array capacity as the parse boundary, but a
transport response makes only data_length bytes current and valid. A
truncated response can therefore make the parser consume a block header or
block extent outside the current response.

Link: https://lore.kernel.org/all/20260706093317.80867-1-pengpeng@iscas.ac.cn/

> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index e18e80e832fd3..175208d712b06 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -1052,32 +1052,49 @@ static int occ_setup_sensor_attrs(struct occ *occ)

[ ... ]

>  		old_offset = offset;
>  		offset = (block->header.num_sensors *
>  			  block->header.sensor_length) + sizeof(block->header);
> -		size += offset;
>  
>  		/* validate all the length/size fields */
> -		if ((size + sizeof(*header)) >= OCC_RESP_DATA_BYTES) {
> -			dev_warn(occ->bus_dev, "exceeded response buffer\n");
> -			return;
> +		if (size + sizeof(*header) + offset > data_length) {
> +			dev_err(occ->bus_dev,
> +				"exceeded OCC poll response length\n");
> +			return -EMSGSIZE;
>  		}
> +		size += offset;

[Severity: High]
This isn't a bug introduced by this patch, but is the validation of the
sensor block size sufficient to prevent out-of-bounds reads?

The validation uses the device-provided sensor_length without verifying it
matches the expected C structure sizes used by sysfs readers.

For example, sysfs readers like occ_show_power_a0() use native pointer
arithmetic based on static C structures:

drivers/hwmon/occ/common.c:occ_show_power_a0() {
    ...
    power = ((struct power_sensor_a0 *)sensors->power.data) + sattr->index;
    ...
}

If a compromised or malfunctioning OCC device returns a poll response with
a maliciously small sensor_length (e.g., 1) but a large num_sensors, could
sysfs readers read beyond the validated bounds of occ->resp.data, leaking
adjacent kernel heap memory to userspace?

[ ... ]

> @@ -1107,6 +1124,9 @@ static void occ_parse_poll_response(struct occ *occ)
>  
>  	dev_dbg(occ->bus_dev, "Max resp size: %u+%zd=%zd\n", size,
>  		sizeof(*header), size + sizeof(*header));
> +	occ->sensors = parsed;
> +
> +	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but can lockless reads of the response
buffer create a data race leading to torn reads for multi-byte sensor
values?

The hwmon subsystem core locking rules mandate that drivers registering
attributes via groups must implement their own locking for those
attributes.

In occ_update_response(), the response buffer is updated under occ->lock,
but callers drop the lock before consuming the data. For example, sysfs
readers directly access parsed multi-byte sensor data without locks:

drivers/hwmon/occ/common.c:occ_show_power_a0() {
    ...
    val = occ_get_powr_avg(get_unaligned_be64(&power->system.accumulator),
                           get_unaligned_be32(&power->system.update_tag));
    ...
}

If concurrent sysfs reads trigger occ_update_response(), which overwrites
the shared occ->resp buffer via an underlying bus transfer while another
thread is reading from it, could this cause torn reads and wild spikes in
reported statistics?

[ ... ]

> @@ -1138,10 +1158,12 @@ int occ_active(struct occ *occ, bool active)
>  			goto unlock;
>  		}
>  
> -		occ->active = true;
>  		occ->next_update = jiffies + OCC_UPDATE_FREQUENCY;
> -		occ_parse_poll_response(occ);
> +		rc = occ_parse_poll_response(occ);
> +		if (rc)
> +			goto unlock;
>  
> +		occ->active = true;
>  		rc = occ_setup_sensor_attrs(occ);

[Severity: High]
This is a pre-existing issue, but does repeatedly toggling the occ_active
sysfs attribute leak memory?

Each activation calls occ_setup_sensor_attrs(), which uses devm_kcalloc()
to allocate memory on the parent device:

drivers/hwmon/occ/common.c:occ_setup_sensor_attrs() {
    ...
    occ->attrs = devm_kcalloc(dev, num_attrs, sizeof(*occ->attrs),
                              GFP_KERNEL);
    ...
}

On deactivation, the hwmon device is unregistered, but the devm_ managed
memory is tied to the parent device and is never freed until the module is
removed.

Can a user with write access to the hwmon sysfs attributes repeatedly write
'0' then '1' to the occ_active attribute and cause an unbounded memory leak
leading to system OOM?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720115826.14813-1-pengpeng@iscas.ac.cn?part=1