[PATCH] Input: xpad - ignore short Xbox One packets

[PATCH] Input: xpad - ignore short Xbox One packets

[Guangshuo Li]

xpadone_process_packet() parses Xbox One reports from fixed offsets in
the interrupt transfer buffer.

A malformed device can complete an interrupt transfer with a short
actual_length. The transfer buffer is reused across URBs, so bytes beyond
actual_length can still contain data left by a previous, longer transfer.
Without checking the received length, the parser can report stale button,
axis or paddle state as if it belonged to the new packet.

A previous fix added bounds checking for the len-relative Share button
access. Add length checks for the fixed-offset Xbox One packet fields as
well, and skip optional extended fields when the corresponding bytes are
not present.

Fixes: 1a48ff81b391 ("Input: xpad - add support for Xbox One controllers")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/input/joystick/xpad.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f834..649f59b3a162 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -1035,8 +1035,14 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 	struct input_dev *dev = xpad->dev;
 	bool do_sync = false;
 
+	if (!len)
+		return;
+
 	/* the xbox button has its own special report */
 	if (data[0] == GIP_CMD_VIRTUAL_KEY) {
+		if (len < 3)
+			return;
+
 		/*
 		 * The Xbox One S controller requires these reports to be
 		 * acked otherwise it continues sending them forever and
@@ -1044,6 +1050,8 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 		 */
 		if (data[1] == (GIP_OPT_ACK | GIP_OPT_INTERNAL))
 			xpadone_ack_mode_report(xpad, data[2]);
+		if (len < 5)
+			return;
 
 		input_report_key(dev, BTN_MODE, data[4] & GENMASK(1, 0));
 		input_sync(dev);
@@ -1052,6 +1060,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 +1090,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 +1159,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 > 34)
 			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 no_paddles;
+
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1165,6 +1182,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 no_paddles;
+
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1178,6 +1198,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 no_paddles;
+
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1194,7 +1217,7 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 				input_report_key(dev, BTN_GRIPL2, data[22] & BIT(3));
 			}
 		}
-
+no_paddles:
 		do_sync = true;
 	}
 
-- 
2.43.0

Re: [PATCH] Input: xpad - ignore short Xbox One packets

[sashiko-bot]

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

Pre-existing issues:
- [High] Atomic context violation and double URB submission via xpad_start_input
--

--- Patch [1]: [PATCH] Input: xpad - ignore short Xbox One packets ---
commit d7d9f4eb601031ea325de5332a0575c0310240ad
Author: Guangshuo Li <lgs201920130244@gmail.com>

Input: xpad - ignore short Xbox One packets

This commit adds length checks for fixed-offset Xbox One packet fields and
skips optional extended fields when the corresponding bytes are not present.
This prevents parsing stale data left in the transfer buffer from previous
longer transfers when a malformed device sends a short packet.

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

[ ... ]

> @@ -1079,6 +1090,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 is it safe to call
xpad_start_input() from here?

Since xpadone_process_packet() is called from the URB completion handler,
it runs in atomic softirq context. However, xpad_start_input() appears to
perform sleeping operations:

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

It also calls xpad_start_xbox_one(), which uses synchronous USB control
messages that can sleep:

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

Could this sequence trigger might_sleep() warnings or cause a deadlock under
memory pressure? Additionally, could submitting xpad->irq_in while its
completion handler is still running cause a double-submission 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));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708135939.774433-1-lgs201920130244@gmail.com?part=1