Re: [PATCH] HID: nintendo: fix out-of-bounds read in joycon_ctlr_read_handler()

[Silvan Jegen]

heyhey!

Ibrahim Hashimov <security@xxxxxxxxxxxx> wrote:
> joycon_ctlr_read_handler() casts an incoming HID input report to
> struct joycon_input_report and parses it, guarding the cast only with a
> 12-byte length check:
> 
> 	if (size >= 12) /* make sure it contains the input report */
> 		joycon_parse_report(ctlr, (struct joycon_input_report *)data);
> 
> struct joycon_input_report is 49 bytes: a 13-byte header followed by a
> union whose IMU arm is 36 bytes. For an IMU report joycon_parse_report()
> -> joycon_parse_imu_report() walks that union (struct offsets 13..48),
> so a report of exactly 12 bytes with data[0] == JC_INPUT_IMU_DATA passes
> the guard yet is read up to 37 bytes past its declared length. The
> over-read bytes are decoded into accelerometer/gyroscope values and
> forwarded to userspace through the "(IMU)" input device, leaking
> driver-internal memory. data[0] and size are fully controlled by a
> malicious or spoofed Joy-Con/Pro Controller.
> 
> Receive buffers are sized to the maximum report length, so this is an
> over-read within the allocation rather than a slab OOB, but the decoded
> bytes still reach userspace.
> 
> The sibling subcmd path in joycon_ctlr_handle_event() already bounds the
> same cast correctly:
> 
> 	if (size < sizeof(struct joycon_input_report) ||
> 	    data[0] != JC_INPUT_SUBCMD_REPLY)
> 		break;
> 
> Use the same sizeof(struct joycon_input_report) bound here.
> 
> Fixes: 2af16c1f846b ("HID: nintendo: add nintendo switch controller driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Ibrahim Hashimov <security@xxxxxxxxxxxx>
> Assisted-by: AuditCode-AI:2026.07
> ---
>  drivers/hid/hid-nintendo.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

This LGTM!

Reviewed-by: Silvan Jegen <s.jegen@xxxxxxxxx>

Cheers,
Silvan

> 
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index e7302ec01ff1..11b5fe05acf4 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2607,7 +2607,12 @@ static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
>  {
>  	if (data[0] == JC_INPUT_SUBCMD_REPLY || data[0] == JC_INPUT_IMU_DATA ||
>  	    data[0] == JC_INPUT_MCU_DATA) {
> -		if (size >= 12) /* make sure it contains the input report */
> +		/*
> +		 * The whole struct is cast and parsed below, including the
> +		 * IMU/subcmd union, not just the 12-byte partial header this
> +		 * used to check for.
> +		 */
> +		if (size >= sizeof(struct joycon_input_report))
>  			joycon_parse_report(ctlr,
>  					    (struct joycon_input_report *)data);
>  	}