Re: [PATCH] hwmon: valid the data size before reading the sensor data

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

New issues:
- [High] The patch introduces a regression that breaks sensor updates for multiple legitimate devices by incorrectly validating the status report size against the control report size.

Pre-existing issues:
- [High] The patch fails to prevent out-of-bounds reads for several devices because their `buffer_size` is never initialized, leaving the vulnerability completely unfixed.
--

--- Patch [1]: [PATCH] hwmon: valid the data size before reading the sensor data ---
commit 551cce5fc615bfa2ca7512a6e77f7f9ea4f91fc7
Author: Edward Adam Davis <[email protected]>

hwmon: valid the data size before reading the sensor data

This patch adds a size check in aqc_raw_event() to abort reading if the
sensor data size is smaller than the expected buffer size. This aims to
prevent a KASAN out-of-bounds read when processing maliciously short
user-forged sensor data.

Fixes: 0e35f63f7f4e ("hwmon: add driver for Aquacomputer D5 Next")
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=9ee5f5dc18673d6b2f37
Tested-by: [email protected]
Signed-off-by: Edward Adam Davis <[email protected]>

> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 1ca70e7262988..1cc6c220ffe96 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
> @@ -1334,6 +1334,9 @@ static int aqc_raw_event(struct hid_device *hdev, struct hid_report *report, u8
>  
>  	priv = hid_get_drvdata(hdev);
>  
> +	if (size < priv->buffer_size)
> +		return 0;
> +

[Severity: High]
Will this check inadvertently drop legitimate hardware sensor updates for
devices like d5next or aquaero?

The incoming asynchronous input status report size is being compared against
priv->buffer_size, which stores the size of the device's control feature
report. For non-legacy devices, priv->buffer_size is initialized during probe
to large values (e.g., 809 bytes for d5next and 2707 bytes for aquaero), while
valid status input reports are significantly smaller.

Since the legitimate status report size naturally falls short of the control
report size, won't this cause the driver to silently discard all valid sensor
updates for these devices?

[Severity: High]
This is a pre-existing issue, but does this check fail to prevent the
out-of-bounds read for several devices where priv->buffer_size is 0?

The aqc_data struct is allocated with devm_kzalloc(), meaning
priv->buffer_size defaults to 0. While devices like d5next initialize this
field, it is intentionally left unassigned (and thus 0) for devices like
leakshield, highflownext, aquastreamult, and farbwerk:

drivers/hwmon/aquacomputer_d5next.c:aqc_probe() {
    ...
	case USB_PRODUCT_ID_HIGHFLOWNEXT:
		priv->kind = highflownext;
		priv->num_fans = 0;
		priv->num_temp_sensors = HIGHFLOWNEXT_NUM_SENSORS;
    ...
}

When a maliciously small report is received for one of these devices, the
check evaluates to size < 0, which is always false. Doesn't this bypass the
validation entirely, allowing the driver to read from hardcoded offsets well
past the end of the short report?

>  	/* Info provided with every report */
>  	priv->serial_number[0] = get_unaligned_be16(data + priv->serial_number_start_offset);
>  	priv->serial_number[1] = get_unaligned_be16(data + priv->serial_number_start_offset +

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