[PATCH] Input: xpad: validate Xbox One report lengths

[PATCH] Input: xpad: validate Xbox One report lengths

[Pengpeng Hou]

xpadone_process_packet() receives a 64-byte USB buffer, but only
urb->actual_length belongs to the current report. The function still reads
fixed offsets for virtual-key, firmware, input, profile, and paddle layouts
without proving that the report reaches those offsets.

Ignore malformed short reports before consuming mandatory fields, and skip
optional profile or paddle fields when their layout-specific minimum length
is absent. This preserves processing of the validated common input prefix
while avoiding reads of bytes outside the current USB item.

Fixes: 1a48ff81b391 ("Input: xpad - add support for Xbox One controllers")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/input/joystick/xpad.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f834..9a32e7eb464d 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -1035,8 +1035,19 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 	struct input_dev *dev = xpad->dev;
 	bool do_sync = false;
 
+	/*
+	 * The interrupt buffer has room for XPAD_PKT_LEN bytes, but only
+	 * urb->actual_length belongs to this report. Do not interpret bytes
+	 * left over from an earlier, longer report as fields of this one.
+	 */
+	if (!len)
+		return;
+
 	/* the xbox button has its own special report */
 	if (data[0] == GIP_CMD_VIRTUAL_KEY) {
+		if (len < 5)
+			return;
+
 		/*
 		 * The Xbox One S controller requires these reports to be
 		 * acked otherwise it continues sending them forever and
@@ -1052,6 +1063,9 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 	} else if (data[0] == GIP_CMD_FIRMWARE) {
 		/* Some packet formats force us to use this separate to poll paddle inputs */
 		if (xpad->packet_type == PKT_XBE2_FW_5_11) {
+			if (len < 20)
+				return;
+
 			/* Mute paddles if controller is in a custom profile slot
 			 * Checked by looking at the active profile slot to
 			 * verify it's the default slot
@@ -1079,6 +1093,9 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 					 error);
 		}
 	} else if (data[0] == GIP_CMD_INPUT) { /* The main valid packet type for inputs */
+		if (len < 18)
+			return;
+
 		/* menu/view buttons */
 		input_report_key(dev, BTN_START,  data[4] & BIT(2));
 		input_report_key(dev, BTN_SELECT, data[4] & BIT(3));
@@ -1145,13 +1162,16 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 		}
 
 		/* Profile button has a value of 0-3, so it is reported as an axis */
-		if (xpad->mapping & MAP_PROFILE_BUTTON)
+		if ((xpad->mapping & MAP_PROFILE_BUTTON) && len >= 35)
 			input_report_abs(dev, ABS_PROFILE, data[34]);
 
 		/* paddle handling */
 		/* based on SDL's SDL_hidapi_xboxone.c */
 		if (xpad->mapping & MAP_PADDLES) {
 			if (xpad->packet_type == PKT_XBE1) {
+				if (len < 33)
+					goto input_done;
+
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1165,6 +1185,9 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 				input_report_key(dev, BTN_GRIPL, data[32] & BIT(0));
 				input_report_key(dev, BTN_GRIPL2, data[32] & BIT(2));
 			} else if (xpad->packet_type == PKT_XBE2_FW_OLD) {
+				if (len < 20)
+					goto input_done;
+
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1178,6 +1201,9 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 				input_report_key(dev, BTN_GRIPL, data[18] & BIT(2));
 				input_report_key(dev, BTN_GRIPL2, data[18] & BIT(3));
 			} else if (xpad->packet_type == PKT_XBE2_FW_5_EARLY) {
+				if (len < 24)
+					goto input_done;
+
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1195,6 +1221,7 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 			}
 		}
 
+input_done:
 		do_sync = true;
 	}
 
-- 
2.43.0

Re: [PATCH] Input: xpad: validate Xbox One report lengths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Handling GIP_CMD_ANNOUNCE in xpadone_process_packet calls xpad_start_input(), leading to sleeping functions being called in atomic context and a potential deadlock.
--

--- Patch [1]: [PATCH] Input: xpad: validate Xbox One report lengths ---
commit b48d8bd27dd206f904c7c21ce961f7a9225d4190
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: Input: xpad: validate Xbox One report lengths

This commit adds bounds checking to xpadone_process_packet() to prevent
out-of-bounds reads on short USB reports. It validates that mandatory and
optional fields fall within urb->actual_length before processing them.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..9a32e7eb464d5 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c

[ ... ]

> @@ -1079,6 +1093,9 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
>  					 error);
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does the GIP_CMD_ANNOUNCE
handling in the code right above this snippet sleep in atomic context?

xpadone_process_packet() executes in the atomic context of the xpad_irq_in
URB completion handler. When handling a GIP_CMD_ANNOUNCE packet, it calls
xpad_start_input().

This leads to a sleep-in-atomic violation due to a sleeping allocation:

xpadone_process_packet()
  -> xpad_start_input()
       -> usb_submit_urb(xpad->irq_in, GFP_KERNEL)

It also takes a mutex during USB control messages, which can sleep:

xpadone_process_packet()
  -> xpad_start_input()
       -> xpad_start_xbox_one()
            -> usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0)

Finally, if xpad_start_input() fails, does the error path deadlock? 
It appears to wait indefinitely for the completion handler it is 
currently executing inside of:

xpadone_process_packet()
  -> xpad_start_input()
       -> usb_kill_urb(xpad->irq_in)

>  	} else if (data[0] == GIP_CMD_INPUT) { /* The main valid packet type for inputs */
> +		if (len < 18)
> +			return;
> +
>  		/* menu/view buttons */
>  		input_report_key(dev, BTN_START,  data[4] & BIT(2));
>  		input_report_key(dev, BTN_SELECT, data[4] & BIT(3));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715083424.25944-1-pengpeng@iscas.ac.cn?part=1