Re: Re: [PATCH] hwmon: (aquacomputer_d5next) reject short status reports

[Guenter Roeck]

On Sun, Sep 27, 2026 at 05:06:38PM +0800, jiale yao wrote:
> At 2026-09-25 00:05:19, "Guenter Roeck" <linux@xxxxxxxxxxxx> wrote:
> >On Thu, Sep 24, 2026 at 11:11:21PM +0800, Jiale Yao wrote:
> >> The HID driver raw_event callback runs before HID core validates the
> >> received report length.  aqc_raw_event() reads device-specific status
> >> fields at fixed offsets, including multi-byte values, without checking
> >> that the received buffer contains the complete report.  A truncated status
> >> report can therefore cause out-of-bounds reads and update hwmon state with
> >> data beyond the received report.
> >> 
> >> Reject status reports shorter than the length derived from their report
> >> descriptor before accessing any fields.  Commit 47669bec44fe ("HID: asus:
> >> refactor the two workqueues and init sequence") added equivalent raw-event
> >> length validation to hid-asus.
> >> 
> >> Fixes: 0e35f63f7f4e ("hwmon: add driver for Aquacomputer D5 Next")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
> >> ---
> >>  drivers/hwmon/aquacomputer_d5next.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> >> index 1ca70e726298..1ebc8bc4c090 100644
> >> --- a/drivers/hwmon/aquacomputer_d5next.c
> >> +++ b/drivers/hwmon/aquacomputer_d5next.c
> >> @@ -1331,6 +1331,8 @@ static int aqc_raw_event(struct hid_device *hdev, struct hid_report *report, u8
> >>  
> >>  	if (report->id != STATUS_REPORT_ID)
> >>  		return 0;
> >> +	if (size < hid_report_len(report))
> >> +		return 0;
> >
> >That doesn't ensure that the report is not at least as long as accessed
> >later in the function, and thus does not fix anything. The same applies
> >to the other patch.
> >
> >A proper fix would be to ensure that size is at least as long as accessed
> >in the function. hid_report_len() does not and can not know that value.
> >It is purely driver specific.
> 
> I tried to find a cleaner way to derive the actual required length, but it seems
> this is entirely driver-specific. I'll replace this with an explicit minimum-length
> table for the supported layouts, just like:
> ---
> +/* Minimum size required for all fields read by aqc_raw_event(). */
> +static const u16 aqc_min_status_report_size[] = {
> +	[d5next] = D5NEXT_PUMP_OFFSET + AQC_FAN_SPEED_OFFSET + sizeof(u16),
> +	[farbwerk] = FARBWERK_SENSOR_START + FARBWERK_NUM_SENSORS * AQC_SENSOR_SIZE,
> +	[farbwerk360] = FARBWERK360_VIRTUAL_SENSORS_START +
> +			FARBWERK360_NUM_VIRTUAL_SENSORS * AQC_SENSOR_SIZE,
> +	[octo] = 0xd8 + AQC_FAN_SPEED_OFFSET + sizeof(u16),
> +	[quadro] = 0x97 + AQC_FAN_SPEED_OFFSET + sizeof(u16),
> +	[highflownext] = HIGHFLOWNEXT_5V_VOLTAGE_USB + sizeof(u16),
> +	[aquaero] = 0x18b + AQUAERO_FAN_POWER_OFFSET + sizeof(u16),
> +	[aquastreamult] = AQUASTREAMULT_PRESSURE_OFFSET + sizeof(u16),
> +	[leakshield] = LEAKSHIELD_RESERVOIR_VOLUME + sizeof(u16),
> +	[highflow] = 0,
> +};
> +
> ---
> But it's so ulgy, that's ok? or any other suggestion.

apc_probe already has a switch statement for each supported cooler. The
per-cooler size can be assigned to struct aqc_data as, say, min_report_size
variable which can then easily be checked at runtime. The minimum
report size can be a simple per-cooler define just like all other per-cooler
defines. That isn't more ugly than the current code. Is made necessary
by the vendor of the supported coolers, so it can't be helped.

Guenter