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

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

[Pengpeng Hou]

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.

Use data_length as the parent boundary, prove the fixed poll header and
each current block header before reading them, and prove the complete block
before advancing. Keep parsed sensor metadata local until the complete
response has passed validation, then publish it. Propagate
malformed-response errors before publishing the OCC as active.

Fixes: aa195fe49b03 ("hwmon (occ): Parse OCC poll response")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260706093317.80867-1-pengpeng@iscas.ac.cn/
- use dev_err() for every malformed response that aborts registration
- publish parsed sensor metadata only after every block passes validation
- limit this revision to the current-response and block-extent contract
- rebase onto v7.2-rc4

 drivers/hwmon/occ/common.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index e18e80e832fd..175208d712b0 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)
 }
 
 /* only need to do this once at startup, as OCC won't change sensors on us */
-static void occ_parse_poll_response(struct occ *occ)
+static int occ_parse_poll_response(struct occ *occ)
 {
 	unsigned int i, old_offset, offset = 0, size = 0;
+	u16 data_length;
 	struct occ_sensor *sensor;
-	struct occ_sensors *sensors = &occ->sensors;
+	struct occ_sensors parsed = {};
+	struct occ_sensors *sensors = &parsed;
 	struct occ_response *resp = &occ->resp;
 	struct occ_poll_response *poll =
 		(struct occ_poll_response *)&resp->data[0];
 	struct occ_poll_response_header *header = &poll->header;
 	struct occ_sensor_data_block *block = &poll->block;
 
+	data_length = get_unaligned_be16(&resp->data_length);
+	if (data_length < sizeof(*header) || data_length > OCC_RESP_DATA_BYTES) {
+		dev_err(occ->bus_dev, "invalid OCC poll response length %u\n",
+			data_length);
+		return -EMSGSIZE;
+	}
+
 	dev_info(occ->bus_dev, "OCC found, code level: %.16s\n",
 		 header->occ_code_level);
 
 	for (i = 0; i < header->num_sensor_data_blocks; ++i) {
 		block = (struct occ_sensor_data_block *)((u8 *)block + offset);
+		if (size + sizeof(*header) + sizeof(block->header) >
+		    data_length) {
+			dev_err(occ->bus_dev,
+				"truncated OCC sensor block header\n");
+			return -EMSGSIZE;
+		}
+
 		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;
 
 		dev_dbg(occ->bus_dev, " %04x..%04x: %.4s (%d sensors)\n",
 			old_offset, offset - 1, block->header.eye_catcher,
@@ -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;
 }
 
 int occ_active(struct occ *occ, bool active)
@@ -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);
 		if (rc) {
 			dev_err(occ->bus_dev,

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

[sashiko-bot]

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

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

[Guenter Roeck]

On Mon, Jul 20, 2026 at 07:58:26PM +0800, Pengpeng Hou wrote:
> 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.
> 
> Use data_length as the parent boundary, prove the fixed poll header and
> each current block header before reading them, and prove the complete block
> before advancing. Keep parsed sensor metadata local until the complete
> response has passed validation, then publish it. Propagate
> malformed-response errors before publishing the OCC as active.
> 
> Fixes: aa195fe49b03 ("hwmon (occ): Parse OCC poll response")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

The issues reported by Sashiko are pre-existing issues worth looking into,
but not as part of this patch.

Applied.

Thanks,
Guenter