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

[jiale yao]

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.

>
>Also, the code added in commit 47669bec44fe is:
>
>        if (size < 2) {
>                hid_dbg(hdev, "Unexpected keyboard report size %d\n", size);
>                return 0;
>        }
>
>which makes sense because asus_raw_event() never accesses more than two bytes
>in the report. The statement "Commit 47669bec44fe ("HID: asus: refactor the two
>workqueues and init sequence") added equivalent raw-event length validation
>to hid-asus" is either hallucinated or intentionally misleading.
>
>Guenter