Re: [PATCH v5 3/3] HID: nintendo: Add unified report format support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Insufficient bounds checking for NS2_REPORT_UNIFIED payload leads to out-of-bounds reads.
- [High] Divide-by-zero kernel panic in switch2_report_trigger() caused by attacker-controlled calibration data.
--

commit 6e45b564fde8014fc6ade8452a1b793097b3bee4
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: nintendo: Add unified report format support

This commit adds support for the "unified" report format to the nintendo hid
driver. It introduces new mappings for overlapping button and axis fields
across different controller types.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 9801799a90758..9e7eda7b70bfb 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
[ ... ]
> @@ -3690,11 +3791,48 @@ static int switch2_event(struct hid_device *hdev, struct hid_report *report, uin
>  
>  	switch (report->id) {
>  	case NS2_REPORT_UNIFIED:
> -		/*
> -		 * TODO
> -		 * This won't be sent unless the report type gets changed via command
> -		 * 03-0A, but we should support it at some point regardless.
> -		 */
> +		switch (ns2->ctlr_type) {
> +		case NS2_CTLR_TYPE_JCL:
> +			switch2_report_stick(input, &ns2->stick_calib[0],
> +				ABS_X, false, ABS_Y, true, &raw_data[11]);
> +			switch2_report_buttons(input, &raw_data[5],
> +				ns2_left_joycon_button_unified_mappings);
> +			break;
> +		case NS2_CTLR_TYPE_JCR:
> +			switch2_report_stick(input, &ns2->stick_calib[0],
> +				ABS_X, false, ABS_Y, true, &raw_data[14]);

Is this safe if the report size is exactly 15 bytes?

Earlier in switch2_event(), there is a global size check:

    if (size < 15)
        return -EINVAL;

However, switch2_report_stick() reads up to 3 bytes from the provided data
pointer (indices 0, 1, and 2). Passing &raw_data[14] means it will read
raw_data[14], raw_data[15], and raw_data[16].

Could this cause an out-of-bounds read if an undersized report is sent?

> +			switch2_report_buttons(input, &raw_data[5],
> +				ns2_right_joycon_button_unified_mappings);
> +			break;
> +		case NS2_CTLR_TYPE_GC:
> +			input_report_abs(input, ABS_HAT0X,
> +				!!(raw_data[7] & NS2_BTN_U3_RIGHT) -
> +				!!(raw_data[7] & NS2_BTN_U3_LEFT));
> +			input_report_abs(input, ABS_HAT0Y,
> +				!!(raw_data[7] & NS2_BTN_U3_DOWN) -
> +				!!(raw_data[7] & NS2_BTN_U3_UP));
> +			switch2_report_buttons(input, &raw_data[5], ns2_gccon_unified_mappings);
> +			switch2_report_stick(input, &ns2->stick_calib[0],
> +				ABS_X, false, ABS_Y, true, &raw_data[11]);
> +			switch2_report_stick(input, &ns2->stick_calib[1],
> +				ABS_RX, false, ABS_RY, true, &raw_data[14]);
> +			switch2_report_trigger(input, ns2->lt_zero, ABS_Z, raw_data[0x3d]);
> +			switch2_report_trigger(input, ns2->rt_zero, ABS_RZ, raw_data[0x3e]);

Similarly, does this code read past the end of the buffer?

Accessing raw_data[0x3d] (index 61) and raw_data[0x3e] (index 62) requires
the report size to be at least 63 bytes, but the global check only guarantees
15 bytes.

Additionally, could this new path trigger a divide-by-zero panic?

While this issue pre-dates this commit, switch2_report_trigger() contains the
following calculation:

    int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero);

The zero parameter here comes from the device's factory calibration data
(ns2->lt_zero and ns2->rt_zero). If a device advertises 232 as its zero
calibration point, this division evaluates to 0.

Since this patch exposes a new way to call switch2_report_trigger(), might it
be worth adding validation to prevent a zero divisor?

> +			break;
> +		case NS2_CTLR_TYPE_PRO:
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512200051.2534081-1-vi@endrift.com?part=3