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

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

[Jiale Yao]

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@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 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;
 
 	priv = hid_get_drvdata(hdev);
 
-- 
2.34.1

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

[Guenter Roeck]

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@vger.kernel.org
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  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.

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

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

[jiale yao]

At 2026-09-25 00:05:19, "Guenter Roeck" <linux@roeck-us.net> 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@vger.kernel.org
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> ---
>>  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

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@roeck-us.net> 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@vger.kernel.org
> >> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> >> ---
> >>  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