[PATCH 0/3] XPAD safety strengthening

[PATCH 0/3] XPAD safety strengthening

[Griffin Kroah-Hartman]

A patch series dedicated to increasing the security of the Xbox
controller family input signals.

Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
Griffin Kroah-Hartman (3):
      Input: xpad - add safer data access framework
      Input: xpad - add sdata_check() to xpad controllers
      Input: xpad - add sdata_check() to xpad360 branches

 drivers/input/joystick/xpad.c | 240 +++++++++++++++++++++++++-----------------
 1 file changed, 142 insertions(+), 98 deletions(-)
---
base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
change-id: 20260729-xpadone_packet_fix-3cdceb6fc8ff

Best regards,
--  
Griffin Kroah-Hartman <griffin@kroah.com>

[PATCH 1/3] Input: xpad - add safer data access framework

[Griffin Kroah-Hartman]

USB xpad devices could send short messages which would cause reads and
writes outside of the data buffer.

Fix this by adding the safe_data struct and the sdata_check() function when
accessing packet data for input events, and add the usage of this to
xpadone_process_packet(), which was vulnerable to OOB reads/writes.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
 drivers/input/joystick/xpad.c | 115 ++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 44 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f834..c516860711a8 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -780,6 +780,24 @@ struct usb_xpad {
 	bool delayed_init_done;
 };
 
+struct safe_data {
+	unsigned char *data;
+	u32 len;
+};
+
+/*
+ * Safe Data Check
+ *
+ * Returns the correct data when inside the array's bounds,
+ * returns 0 when accessing an out-of-bounds index.
+ */
+static u8 sdata_check(struct safe_data *sdata, int idx)
+{
+	if (idx >= sdata->len)
+		return 0;
+	return sdata->data[idx];
+}
+
 static int xpad_init_input(struct usb_xpad *xpad);
 static void xpad_deinit_input(struct usb_xpad *xpad);
 static int xpad_start_input(struct usb_xpad *xpad);
@@ -1033,41 +1051,45 @@ static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned cha
 static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
 {
 	struct input_dev *dev = xpad->dev;
+	struct safe_data sdata;
 	bool do_sync = false;
 
+	sdata.data = data;
+	sdata.len = len;
+
 	/* the xbox button has its own special report */
-	if (data[0] == GIP_CMD_VIRTUAL_KEY) {
+	if (sdata_check(&sdata, 0) == GIP_CMD_VIRTUAL_KEY) {
 		/*
 		 * The Xbox One S controller requires these reports to be
 		 * acked otherwise it continues sending them forever and
 		 * won't report further mode button events.
 		 */
-		if (data[1] == (GIP_OPT_ACK | GIP_OPT_INTERNAL))
-			xpadone_ack_mode_report(xpad, data[2]);
+		if (sdata_check(&sdata, 1) == (GIP_OPT_ACK | GIP_OPT_INTERNAL))
+			xpadone_ack_mode_report(xpad, sdata_check(&sdata, 2));
 
-		input_report_key(dev, BTN_MODE, data[4] & GENMASK(1, 0));
+		input_report_key(dev, BTN_MODE, sdata_check(&sdata, 4) & GENMASK(1, 0));
 		input_sync(dev);
 
 		do_sync = true;
-	} else if (data[0] == GIP_CMD_FIRMWARE) {
+	} else if (sdata_check(&sdata, 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) {
 			/* 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
 			 */
-			if (data[19] != 0)
+			if (sdata_check(&sdata, 19) != 0)
 				data[18] = 0;
 
 			/* Elite Series 2 split packet paddle bits */
-			input_report_key(dev, BTN_GRIPR, data[18] & BIT(0));
-			input_report_key(dev, BTN_GRIPR2, data[18] & BIT(1));
-			input_report_key(dev, BTN_GRIPL, data[18] & BIT(2));
-			input_report_key(dev, BTN_GRIPL2, data[18] & BIT(3));
+			input_report_key(dev, BTN_GRIPR, sdata_check(&sdata, 18) & BIT(0));
+			input_report_key(dev, BTN_GRIPR2, sdata_check(&sdata, 18) & BIT(1));
+			input_report_key(dev, BTN_GRIPL, sdata_check(&sdata, 18) & BIT(2));
+			input_report_key(dev, BTN_GRIPL2, sdata_check(&sdata, 18) & BIT(3));
 
 			do_sync = true;
 		}
-	} else if (data[0] == GIP_CMD_ANNOUNCE) {
+	} else if (sdata_check(&sdata, 0) == GIP_CMD_ANNOUNCE) {
 		int error;
 
 		if (xpad->delay_init && !xpad->delayed_init_done) {
@@ -1078,44 +1100,49 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 					 "unable to start delayed input: %d\n",
 					 error);
 		}
-	} else if (data[0] == GIP_CMD_INPUT) { /* The main valid packet type for inputs */
+	} else if (sdata_check(&sdata, 0) == GIP_CMD_INPUT) {
+		/* The main valid packet type for inputs */
+
 		/* menu/view buttons */
-		input_report_key(dev, BTN_START,  data[4] & BIT(2));
-		input_report_key(dev, BTN_SELECT, data[4] & BIT(3));
+		input_report_key(dev, BTN_START,  sdata_check(&sdata, 4) & BIT(2));
+		input_report_key(dev, BTN_SELECT, sdata_check(&sdata, 4) & BIT(3));
 		if (xpad->mapping & MAP_SHARE_BUTTON) {
 			u32 offset = (xpad->mapping & MAP_SHARE_OFFSET) ? 26 : 18;
 
 			if (len >= offset)
-				input_report_key(dev, KEY_RECORD, data[len - offset] & BIT(0));
+				input_report_key(dev, KEY_RECORD,
+						 sdata_check(&sdata, len - offset) & BIT(0));
 		}
 
 		/* buttons A,B,X,Y */
-		input_report_key(dev, BTN_A,	data[4] & BIT(4));
-		input_report_key(dev, BTN_B,	data[4] & BIT(5));
-		input_report_key(dev, BTN_X,	data[4] & BIT(6));
-		input_report_key(dev, BTN_Y,	data[4] & BIT(7));
+		input_report_key(dev, BTN_A,	sdata_check(&sdata, 4) & BIT(4));
+		input_report_key(dev, BTN_B,	sdata_check(&sdata, 4) & BIT(5));
+		input_report_key(dev, BTN_X,	sdata_check(&sdata, 4) & BIT(6));
+		input_report_key(dev, BTN_Y,	sdata_check(&sdata, 4) & BIT(7));
 
 		/* digital pad */
 		if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
 			/* dpad as buttons (left, right, up, down) */
-			input_report_key(dev, BTN_DPAD_LEFT, data[5] & BIT(2));
-			input_report_key(dev, BTN_DPAD_RIGHT, data[5] & BIT(3));
-			input_report_key(dev, BTN_DPAD_UP, data[5] & BIT(0));
-			input_report_key(dev, BTN_DPAD_DOWN, data[5] & BIT(1));
+			input_report_key(dev, BTN_DPAD_LEFT, sdata_check(&sdata, 5) & BIT(2));
+			input_report_key(dev, BTN_DPAD_RIGHT, sdata_check(&sdata, 5) & BIT(3));
+			input_report_key(dev, BTN_DPAD_UP, sdata_check(&sdata, 5) & BIT(0));
+			input_report_key(dev, BTN_DPAD_DOWN, sdata_check(&sdata, 5) & BIT(1));
 		} else {
 			input_report_abs(dev, ABS_HAT0X,
-					!!(data[5] & 0x08) - !!(data[5] & 0x04));
+					!!(sdata_check(&sdata, 5) & 0x08) -
+					!!(sdata_check(&sdata, 5) & 0x04));
 			input_report_abs(dev, ABS_HAT0Y,
-					!!(data[5] & 0x02) - !!(data[5] & 0x01));
+					!!(sdata_check(&sdata, 5) & 0x02) -
+					!!(sdata_check(&sdata, 5) & 0x01));
 		}
 
 		/* TL/TR */
-		input_report_key(dev, BTN_TL,	data[5] & BIT(4));
-		input_report_key(dev, BTN_TR,	data[5] & BIT(5));
+		input_report_key(dev, BTN_TL,	sdata_check(&sdata, 5) & BIT(4));
+		input_report_key(dev, BTN_TR,	sdata_check(&sdata, 5) & BIT(5));
 
 		/* stick press left/right */
-		input_report_key(dev, BTN_THUMBL, data[5] & BIT(6));
-		input_report_key(dev, BTN_THUMBR, data[5] & BIT(7));
+		input_report_key(dev, BTN_THUMBL, sdata_check(&sdata, 5) & BIT(6));
+		input_report_key(dev, BTN_THUMBR, sdata_check(&sdata, 5) & BIT(7));
 
 		if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
 			/* left stick */
@@ -1146,7 +1173,7 @@ 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)
-			input_report_abs(dev, ABS_PROFILE, data[34]);
+			input_report_abs(dev, ABS_PROFILE, sdata_check(&sdata, 34));
 
 		/* paddle handling */
 		/* based on SDL's SDL_hidapi_xboxone.c */
@@ -1160,38 +1187,38 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 					data[32] = 0;
 
 				/* OG Elite Series Controller paddle bits */
-				input_report_key(dev, BTN_GRIPR, data[32] & BIT(1));
-				input_report_key(dev, BTN_GRIPR2, data[32] & BIT(3));
-				input_report_key(dev, BTN_GRIPL, data[32] & BIT(0));
-				input_report_key(dev, BTN_GRIPL2, data[32] & BIT(2));
+				input_report_key(dev, BTN_GRIPR, sdata_check(&sdata, 32) & BIT(1));
+				input_report_key(dev, BTN_GRIPR2, sdata_check(&sdata, 32) & BIT(3));
+				input_report_key(dev, BTN_GRIPL, sdata_check(&sdata, 32) & BIT(0));
+				input_report_key(dev, BTN_GRIPL2, sdata_check(&sdata, 32) & BIT(2));
 			} else if (xpad->packet_type == PKT_XBE2_FW_OLD) {
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
 				 */
-				if (data[19] != 0)
+				if (sdata_check(&sdata, 19) != 0)
 					data[18] = 0;
 
 				/* Elite Series 2 4.x firmware paddle bits */
-				input_report_key(dev, BTN_GRIPR, data[18] & BIT(0));
-				input_report_key(dev, BTN_GRIPR2, data[18] & BIT(1));
-				input_report_key(dev, BTN_GRIPL, data[18] & BIT(2));
-				input_report_key(dev, BTN_GRIPL2, data[18] & BIT(3));
+				input_report_key(dev, BTN_GRIPR, sdata_check(&sdata, 18) & BIT(0));
+				input_report_key(dev, BTN_GRIPR2, sdata_check(&sdata, 18) & BIT(1));
+				input_report_key(dev, BTN_GRIPL, sdata_check(&sdata, 18) & BIT(2));
+				input_report_key(dev, BTN_GRIPL2, sdata_check(&sdata, 18) & BIT(3));
 			} else if (xpad->packet_type == PKT_XBE2_FW_5_EARLY) {
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
 				 */
-				if (data[23] != 0)
+				if (sdata_check(&sdata, 23) != 0)
 					data[22] = 0;
 
 				/* Elite Series 2 5.x firmware paddle bits
 				 * (before the packet was split)
 				 */
-				input_report_key(dev, BTN_GRIPR, data[22] & BIT(0));
-				input_report_key(dev, BTN_GRIPR2, data[22] & BIT(1));
-				input_report_key(dev, BTN_GRIPL, data[22] & BIT(2));
-				input_report_key(dev, BTN_GRIPL2, data[22] & BIT(3));
+				input_report_key(dev, BTN_GRIPR, sdata_check(&sdata, 22) & BIT(0));
+				input_report_key(dev, BTN_GRIPR2, sdata_check(&sdata, 22) & BIT(1));
+				input_report_key(dev, BTN_GRIPL, sdata_check(&sdata, 22) & BIT(2));
+				input_report_key(dev, BTN_GRIPL2, sdata_check(&sdata, 22) & BIT(3));
 			}
 		}
 

-- 
2.55.0

[PATCH 2/3] Input: xpad - add sdata_check() to xpad controllers

[Griffin Kroah-Hartman]

Add the sdata_check() safety wrapper to the xpad_process_packet()
branch. Which should include the original Xbox Controller.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
 drivers/input/joystick/xpad.c | 50 ++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index c516860711a8..319a4c4a695f 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -813,9 +813,13 @@ static void xpad360w_poweroff_controller(struct usb_xpad *xpad);
  *	The used report descriptor was taken from ITO Takayuki's website:
  *	 http://euc.jp/periphs/xbox-controller.ja.html
  */
-static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
+static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
 {
 	struct input_dev *dev = xpad->dev;
+	struct safe_data sdata;
+
+	sdata.data = data;
+	sdata.len = len;
 
 	if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
 		/* left stick */
@@ -833,42 +837,44 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
 
 	/* triggers left/right */
 	if (xpad->mapping & MAP_TRIGGERS_TO_BUTTONS) {
-		input_report_key(dev, BTN_TL2, data[10]);
-		input_report_key(dev, BTN_TR2, data[11]);
+		input_report_key(dev, BTN_TL2, sdata_check(&sdata, 10));
+		input_report_key(dev, BTN_TR2, sdata_check(&sdata, 11));
 	} else {
-		input_report_abs(dev, ABS_Z, data[10]);
-		input_report_abs(dev, ABS_RZ, data[11]);
+		input_report_abs(dev, ABS_Z, sdata_check(&sdata, 10));
+		input_report_abs(dev, ABS_RZ, sdata_check(&sdata, 11));
 	}
 
 	/* digital pad */
 	if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
 		/* dpad as buttons (left, right, up, down) */
-		input_report_key(dev, BTN_DPAD_LEFT, data[2] & BIT(2));
-		input_report_key(dev, BTN_DPAD_RIGHT, data[2] & BIT(3));
-		input_report_key(dev, BTN_DPAD_UP, data[2] & BIT(0));
-		input_report_key(dev, BTN_DPAD_DOWN, data[2] & BIT(1));
+		input_report_key(dev, BTN_DPAD_LEFT, sdata_check(&sdata, 2) & BIT(2));
+		input_report_key(dev, BTN_DPAD_RIGHT, sdata_check(&sdata, 2) & BIT(3));
+		input_report_key(dev, BTN_DPAD_UP, sdata_check(&sdata, 2) & BIT(0));
+		input_report_key(dev, BTN_DPAD_DOWN, sdata_check(&sdata, 2) & BIT(1));
 	} else {
 		input_report_abs(dev, ABS_HAT0X,
-				 !!(data[2] & 0x08) - !!(data[2] & 0x04));
+				 !!(sdata_check(&sdata, 2) & 0x08) -
+				 !!(sdata_check(&sdata, 2) & 0x04));
 		input_report_abs(dev, ABS_HAT0Y,
-				 !!(data[2] & 0x02) - !!(data[2] & 0x01));
+				 !!(sdata_check(&sdata, 2) & 0x02) -
+				 !!(sdata_check(&sdata, 2) & 0x01));
 	}
 
 	/* start/back buttons and stick press left/right */
-	input_report_key(dev, BTN_START,  data[2] & BIT(4));
-	input_report_key(dev, BTN_SELECT, data[2] & BIT(5));
-	input_report_key(dev, BTN_THUMBL, data[2] & BIT(6));
-	input_report_key(dev, BTN_THUMBR, data[2] & BIT(7));
+	input_report_key(dev, BTN_START,  sdata_check(&sdata, 2) & BIT(4));
+	input_report_key(dev, BTN_SELECT, sdata_check(&sdata, 2) & BIT(5));
+	input_report_key(dev, BTN_THUMBL, sdata_check(&sdata, 2) & BIT(6));
+	input_report_key(dev, BTN_THUMBR, sdata_check(&sdata, 2) & BIT(7));
 
 	/* "analog" buttons A, B, X, Y */
-	input_report_key(dev, BTN_A, data[4]);
-	input_report_key(dev, BTN_B, data[5]);
-	input_report_key(dev, BTN_X, data[6]);
-	input_report_key(dev, BTN_Y, data[7]);
+	input_report_key(dev, BTN_A, sdata_check(&sdata, 4));
+	input_report_key(dev, BTN_B, sdata_check(&sdata, 5));
+	input_report_key(dev, BTN_X, sdata_check(&sdata, 6));
+	input_report_key(dev, BTN_Y, sdata_check(&sdata, 7));
 
 	/* "analog" buttons black, white */
-	input_report_key(dev, BTN_C, data[8]);
-	input_report_key(dev, BTN_Z, data[9]);
+	input_report_key(dev, BTN_C, sdata_check(&sdata, 8));
+	input_report_key(dev, BTN_Z, sdata_check(&sdata, 9));
 
 
 	input_sync(dev);
@@ -1265,7 +1271,7 @@ static void xpad_irq_in(struct urb *urb)
 		xpadone_process_packet(xpad, 0, xpad->idata, urb->actual_length);
 		break;
 	default:
-		xpad_process_packet(xpad, 0, xpad->idata);
+		xpad_process_packet(xpad, 0, xpad->idata, urb->actual_length);
 	}
 
 exit:

-- 
2.55.0

[PATCH 3/3] Input: xpad - add sdata_check() to xpad360 branches

[Griffin Kroah-Hartman]

Add the sdata_check() safety wrapper to the xpad360_process_packet() and
xpad360w_process_packet() functions, covering the Xbox 360 wired and
wireless controllers.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
 drivers/input/joystick/xpad.c | 75 +++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 32 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 319a4c4a695f..304229782e45 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -891,19 +891,24 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
  */
 
 static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
-				   u16 cmd, unsigned char *data)
+				   u16 cmd, unsigned char *data, u32 len)
 {
+	struct safe_data sdata;
+
+	sdata.data = data;
+	sdata.len = len;
+
 	/* valid pad data */
-	if (data[0] != 0x00)
+	if (sdata_check(&sdata, 0) != 0x00)
 		return;
 
 	/* digital pad */
 	if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {
 		/* dpad as buttons (left, right, up, down) */
-		input_report_key(dev, BTN_DPAD_LEFT, data[2] & BIT(2));
-		input_report_key(dev, BTN_DPAD_RIGHT, data[2] & BIT(3));
-		input_report_key(dev, BTN_DPAD_UP, data[2] & BIT(0));
-		input_report_key(dev, BTN_DPAD_DOWN, data[2] & BIT(1));
+		input_report_key(dev, BTN_DPAD_LEFT, sdata_check(&sdata, 2) & BIT(2));
+		input_report_key(dev, BTN_DPAD_RIGHT, sdata_check(&sdata, 2) & BIT(3));
+		input_report_key(dev, BTN_DPAD_UP, sdata_check(&sdata, 2) & BIT(0));
+		input_report_key(dev, BTN_DPAD_DOWN, sdata_check(&sdata, 2) & BIT(1));
 	}
 
 	/*
@@ -915,27 +920,29 @@ static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
 	if (!(xpad->mapping & MAP_DPAD_TO_BUTTONS) ||
 	    xpad->xtype == XTYPE_XBOX360W) {
 		input_report_abs(dev, ABS_HAT0X,
-				 !!(data[2] & 0x08) - !!(data[2] & 0x04));
+				 !!(sdata_check(&sdata, 2) & 0x08) -
+				 !!(sdata_check(&sdata, 2) & 0x04));
 		input_report_abs(dev, ABS_HAT0Y,
-				 !!(data[2] & 0x02) - !!(data[2] & 0x01));
+				 !!(sdata_check(&sdata, 2) & 0x02) -
+				 !!(sdata_check(&sdata, 2) & 0x01));
 	}
 
 	/* start/back buttons */
-	input_report_key(dev, BTN_START,  data[2] & BIT(4));
-	input_report_key(dev, BTN_SELECT, data[2] & BIT(5));
+	input_report_key(dev, BTN_START,  sdata_check(&sdata, 2) & BIT(4));
+	input_report_key(dev, BTN_SELECT, sdata_check(&sdata, 2) & BIT(5));
 
 	/* stick press left/right */
-	input_report_key(dev, BTN_THUMBL, data[2] & BIT(6));
-	input_report_key(dev, BTN_THUMBR, data[2] & BIT(7));
+	input_report_key(dev, BTN_THUMBL, sdata_check(&sdata, 2) & BIT(6));
+	input_report_key(dev, BTN_THUMBR, sdata_check(&sdata, 2) & BIT(7));
 
 	/* buttons A,B,X,Y,TL,TR and MODE */
-	input_report_key(dev, BTN_A,	data[3] & BIT(4));
-	input_report_key(dev, BTN_B,	data[3] & BIT(5));
-	input_report_key(dev, BTN_X,	data[3] & BIT(6));
-	input_report_key(dev, BTN_Y,	data[3] & BIT(7));
-	input_report_key(dev, BTN_TL,	data[3] & BIT(0));
-	input_report_key(dev, BTN_TR,	data[3] & BIT(1));
-	input_report_key(dev, BTN_MODE,	data[3] & BIT(2));
+	input_report_key(dev, BTN_A,	sdata_check(&sdata, 3) & BIT(4));
+	input_report_key(dev, BTN_B,	sdata_check(&sdata, 3) & BIT(5));
+	input_report_key(dev, BTN_X,	sdata_check(&sdata, 3) & BIT(6));
+	input_report_key(dev, BTN_Y,	sdata_check(&sdata, 3) & BIT(7));
+	input_report_key(dev, BTN_TL,	sdata_check(&sdata, 3) & BIT(0));
+	input_report_key(dev, BTN_TR,	sdata_check(&sdata, 3) & BIT(1));
+	input_report_key(dev, BTN_MODE,	sdata_check(&sdata, 3) & BIT(2));
 
 	if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
 		/* left stick */
@@ -953,11 +960,11 @@ static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
 
 	/* triggers left/right */
 	if (xpad->mapping & MAP_TRIGGERS_TO_BUTTONS) {
-		input_report_key(dev, BTN_TL2, data[4]);
-		input_report_key(dev, BTN_TR2, data[5]);
+		input_report_key(dev, BTN_TL2, sdata_check(&sdata, 4));
+		input_report_key(dev, BTN_TR2, sdata_check(&sdata, 5));
 	} else {
-		input_report_abs(dev, ABS_Z, data[4]);
-		input_report_abs(dev, ABS_RZ, data[5]);
+		input_report_abs(dev, ABS_Z, sdata_check(&sdata, 4));
+		input_report_abs(dev, ABS_RZ, sdata_check(&sdata, 5));
 	}
 
 	input_sync(dev);
@@ -973,7 +980,7 @@ static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
 		}
 
 		/* mode button down/up */
-		if (data[3] & BIT(2))
+		if (sdata_check(&sdata, 3) & BIT(2))
 			xpad->mode_btn_down_ts = ktime_get_seconds();
 		else
 			xpad->mode_btn_down_ts = 0;
@@ -1019,14 +1026,18 @@ static void xpad_presence_work(struct work_struct *work)
  * 01.1 - Pad state (Bytes 4+) valid
  *
  */
-static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
+static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
 {
 	struct input_dev *dev;
+	struct safe_data sdata;
 	bool present;
 
+	sdata.data = data;
+	sdata.len = len;
+
 	/* Presence change */
-	if (data[0] & 0x08) {
-		present = (data[1] & 0x80) != 0;
+	if (sdata_check(&sdata, 0) & 0x08) {
+		present = (sdata_check(&sdata, 1) & 0x80) != 0;
 
 		if (xpad->pad_present != present) {
 			xpad->pad_present = present;
@@ -1035,13 +1046,13 @@ static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned cha
 	}
 
 	/* Valid pad data */
-	if (data[1] != 0x1)
+	if (sdata_check(&sdata, 1) != 0x1)
 		return;
 
 	rcu_read_lock();
 	dev = rcu_dereference(xpad->x360w_dev);
-	if (dev)
-		xpad360_process_packet(xpad, dev, cmd, &data[4]);
+	if (dev && len > 4)
+		xpad360_process_packet(xpad, dev, cmd, &data[4], len - 4);
 	rcu_read_unlock();
 }
 
@@ -1262,10 +1273,10 @@ static void xpad_irq_in(struct urb *urb)
 
 	switch (xpad->xtype) {
 	case XTYPE_XBOX360:
-		xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata);
+		xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata, urb->actual_length);
 		break;
 	case XTYPE_XBOX360W:
-		xpad360w_process_packet(xpad, 0, xpad->idata);
+		xpad360w_process_packet(xpad, 0, xpad->idata, urb->actual_length);
 		break;
 	case XTYPE_XBOXONE:
 		xpadone_process_packet(xpad, 0, xpad->idata, urb->actual_length);

-- 
2.55.0

Re: [PATCH 1/3] Input: xpad - add safer data access framework

[sashiko-bot]

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

Pre-existing issues:
- [High] Processing GIP_CMD_ANNOUNCE can trigger sleeping functions from an URB completion handler.
--

--- Patch [1]: [PATCH 1/3] Input: xpad - add safer data access framework ---
commit 852b38f29eb8d0217357629e77fd629842b2aed4
Author: Griffin Kroah-Hartman <griffin@kroah.com>

Input: xpad - add safer data access framework

This commit introduces a safe_data struct and a safe data check function to
prevent reads and writes outside of the data buffer bounds.

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

[ ... ]

> @@ -1078,44 +1100,49 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
[ ... ]
> -	} else if (data[0] == GIP_CMD_ANNOUNCE) {
> +	} else if (sdata_check(&sdata, 0) == GIP_CMD_ANNOUNCE) {
>  		int error;
>  
>  		if (xpad->delay_init && !xpad->delayed_init_done) {
>  			xpad->delayed_init_done = true;
>  			error = xpad_start_input(xpad);

[Severity: High]
This is a pre-existing issue, but does this code path trigger sleeping
functions from an atomic context?

When an Xbox One controller sends a GIP_CMD_ANNOUNCE packet on the irq_in
endpoint, the URB completion handler (which runs in atomic softirq context)
processes it via xpadone_process_packet().

If xpad->delay_init is true and initialization hasn't completed, it calls
xpad_start_input(), which subsequently uses GFP_KERNEL:

xpad_start_input() {
    ...
    if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
        return -EIO;
    ...
}

Additionally, xpad_start_input() calls xpad_start_xbox_one(), which makes a
synchronous blocking call that sleeps:

xpad_start_xbox_one() {
    ...
    error = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
    ...
}

Could this lead to a scheduling while atomic kernel panic when a device sends
this command during URB completion?

>  			if (error)
>  				dev_warn(&xpad->dev->dev,
>  					 "unable to start delayed input: %d\n",
>  					 error);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-xpadone_packet_fix-v1-0-280da203f15c@kroah.com?part=1

Re: [PATCH 2/3] Input: xpad - add sdata_check() to xpad controllers

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a use-after-free of `input_dev` in the error path of `xpad_presence_work()` when `xpad_init_input()` fails.
--

--- Patch [2]: [PATCH 2/3] Input: xpad - add sdata_check() to xpad controllers ---
commit 559bf574edac1c4551addcfb6bbff7b5065fbbc5
Author: Griffin Kroah-Hartman <griffin@kroah.com>

Input: xpad - add sdata_check() to xpad controllers

This commit adds the sdata_check() safety wrapper to the
xpad_process_packet() branch. It passes the actual length of the urb
to ensure packet bounds are checked properly. This prevents out-of-bounds
reads during interrupt handling for the original Xbox Controller.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index c516860711a82..319a4c4a695fe 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -813,9 +813,13 @@ static void xpad360w_poweroff_controller(struct usb_xpad *xpad);
>   *	The used report descriptor was taken from ITO Takayuki's website:
>   *	 http://euc.jp/periphs/xbox-controller.ja.html
>   */
> -static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> +static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
>  {
>  	struct input_dev *dev = xpad->dev;
> +	struct safe_data sdata;
> +
> +	sdata.data = data;
> +	sdata.len = len;

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but there is a use-after-free
of input_dev in the error path of xpad_presence_work() when xpad_init_input()
fails.

In xpad_init_input(), xpad->dev is assigned to the newly allocated input_dev.
If initialization fails later, the function jumps to err_free_input and frees
input_dev, but leaves xpad->dev pointing to the freed memory:

drivers/input/joystick/xpad.c:xpad_init_input() {
    ...
	xpad->dev = input_dev;
    ...
err_free_input:
	input_free_device(input_dev);
	return error;
}

When connecting a wireless Xbox 360 controller, xpad_presence_work() calls
xpad_init_input(). If this fails, the error path dereferences the dangling
xpad->dev pointer to log the error:

drivers/input/joystick/xpad.c:xpad_presence_work() {
    ...
		error = xpad_init_input(xpad);
		if (error) {
			/* complain only, not much else we can do here */
			dev_err(&xpad->dev->dev,
				"unable to init device: %d\n", error);
    ...
}

Can we avoid dereferencing xpad->dev here, or wait to assign xpad->dev until
xpad_init_input() is fully successful?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-xpadone_packet_fix-v1-0-280da203f15c@kroah.com?part=2

Re: [PATCH 3/3] Input: xpad - add sdata_check() to xpad360 branches

[sashiko-bot]

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

New issues:
- [Medium] Zero-length packets incorrectly pass validation and are processed as valid empty states, spuriously releasing all controller buttons.

Pre-existing issues:
- [High] Blocking USB APIs and GFP_KERNEL allocations are unsafely invoked from the atomic URB completion handler.
- [High] Error paths during initialization destroy the input device while leaving a dangling pointer for presence_work to dereference.
- [Medium] 16-bit multi-byte reads and direct array modifications completely bypass the new length validation, leading to stale data leakage.
--

--- Patch [3]: [PATCH 3/3] Input: xpad - add sdata_check() to xpad360 branches ---
commit 0a84609d4ff81bead2c2c2c26f83c5c8f9cc506a
Author: Griffin Kroah-Hartman <griffin@kroah.com>

Input: xpad - add sdata_check() to xpad360 branches

This commit adds the sdata_check() safety wrapper to the xpad360_process_packet()
and xpad360w_process_packet() functions. It bounds-checks array accesses to prevent
out-of-bounds reads on the controller data buffer.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 319a4c4a695fe..304229782e455 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -891,19 +891,24 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
>   */
>  
>  static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
> -				   u16 cmd, unsigned char *data)
> +				   u16 cmd, unsigned char *data, u32 len)
>  {
> +	struct safe_data sdata;
> +
> +	sdata.data = data;
> +	sdata.len = len;
> +
>  	/* valid pad data */
> -	if (data[0] != 0x00)
> +	if (sdata_check(&sdata, 0) != 0x00)
>  		return;

[Severity: Medium]
Does this inadvertently allow zero-length packets to be treated as valid
pad data? Because sdata_check() safely returns 0 when the index is out of
bounds, a zero-length packet will successfully pass this != 0x00 check
and spuriously release all controller buttons.

>  
>  	/* digital pad */
>  	if (xpad->mapping & MAP_DPAD_TO_BUTTONS) {

[ ... ]

> @@ -915,27 +920,29 @@ static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
>  	if (!(xpad->mapping & MAP_DPAD_TO_BUTTONS) ||
>  	    xpad->xtype == XTYPE_XBOX360W) {

[ ... ]

>  	/* buttons A,B,X,Y,TL,TR and MODE */
> -	input_report_key(dev, BTN_A,	data[3] & BIT(4));
> -	input_report_key(dev, BTN_B,	data[3] & BIT(5));
> -	input_report_key(dev, BTN_X,	data[3] & BIT(6));
> -	input_report_key(dev, BTN_Y,	data[3] & BIT(7));
> -	input_report_key(dev, BTN_TL,	data[3] & BIT(0));
> -	input_report_key(dev, BTN_TR,	data[3] & BIT(1));
> -	input_report_key(dev, BTN_MODE,	data[3] & BIT(2));
> +	input_report_key(dev, BTN_A,	sdata_check(&sdata, 3) & BIT(4));
> +	input_report_key(dev, BTN_B,	sdata_check(&sdata, 3) & BIT(5));
> +	input_report_key(dev, BTN_X,	sdata_check(&sdata, 3) & BIT(6));
> +	input_report_key(dev, BTN_Y,	sdata_check(&sdata, 3) & BIT(7));
> +	input_report_key(dev, BTN_TL,	sdata_check(&sdata, 3) & BIT(0));
> +	input_report_key(dev, BTN_TR,	sdata_check(&sdata, 3) & BIT(1));
> +	input_report_key(dev, BTN_MODE,	sdata_check(&sdata, 3) & BIT(2));
>  
>  	if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
>  		/* left stick */

[Severity: Medium]
This isn't a bug introduced by this patch, but will the 16-bit stick axis 
reads immediately following this code bypass the new length checks?

xpad_process_packet(), xpad360_process_packet(), and xpadone_process_packet()
read multi-byte values using le16_to_cpup() directly against the raw data
buffer:

    input_report_abs(dev, ABS_X,
                     (__s16) le16_to_cpup((__le16 *)(data + 6)));

A short packet would still cause out-of-bounds reads here since these
accesses completely bypass the sdata_check() wrapper, potentially leaking
uninitialized DMA memory into input events.

[ ... ]

> @@ -1019,14 +1026,18 @@ static void xpad_presence_work(struct work_struct *work)
>   * 01.1 - Pad state (Bytes 4+) valid
>   *
>   */
> -static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> +static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
>  {
>  	struct input_dev *dev;
> +	struct safe_data sdata;
>  	bool present;
>  
> +	sdata.data = data;
> +	sdata.len = len;
> +
>  	/* Presence change */
> -	if (data[0] & 0x08) {
> -		present = (data[1] & 0x80) != 0;
> +	if (sdata_check(&sdata, 0) & 0x08) {
> +		present = (sdata_check(&sdata, 1) & 0x80) != 0;
>  
>  		if (xpad->pad_present != present) {
>  			xpad->pad_present = present;
>  			schedule_work(&xpad->work);

[Severity: High]
This is a pre-existing issue, but can the xpad_presence_work thread
dereference a freed pointer if initialization fails?

If xpad_init_input() encounters an error (like during device registration),
it takes the err_free_input path:

xpad_init_input() {
    ...
err_free_input:
    input_free_device(input_dev);
    return error;
}

This frees the device but leaves xpad->dev pointing to the freed memory.
When xpad_presence_work() processes the failure, it logs the error:

xpad_presence_work() {
    ...
    dev_err(&xpad->dev->dev, "unable to init device: %d\n", error);
}

Could this use-after-free lead to memory corruption or a crash?

[ ... ]

> @@ -1262,10 +1273,10 @@ static void xpad_irq_in(struct urb *urb)
>  
>  	switch (xpad->xtype) {
>  	case XTYPE_XBOX360:
> -		xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata);
> +		xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata, urb->actual_length);
>  		break;
>  	case XTYPE_XBOX360W:
> -		xpad360w_process_packet(xpad, 0, xpad->idata);
> +		xpad360w_process_packet(xpad, 0, xpad->idata, urb->actual_length);
>  		break;
>  	case XTYPE_XBOXONE:
>  		xpadone_process_packet(xpad, 0, xpad->idata, urb->actual_length);

[Severity: High]
This is a pre-existing issue, but does xpadone_process_packet() execute
blocking calls and GFP_KERNEL allocations from an atomic context?

When xpad_irq_in() processes an Xbox One GIP_CMD_ANNOUNCE packet from
interrupt context, it follows this call chain:

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

xpad_start_input() also calls xpad_start_xbox_one(), which invokes
usb_set_interface(), a synchronous and blocking USB API. Calling these
from the atomic URB completion handler could lead to scheduling while
atomic warnings and potential deadlocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-xpadone_packet_fix-v1-0-280da203f15c@kroah.com?part=3

Re: [PATCH 1/3] Input: xpad - add safer data access framework

[Dmitry Torokhov]

Hi Griffin,

On Mon, Aug 03, 2026 at 05:07:24PM +0200, Griffin Kroah-Hartman wrote:
> USB xpad devices could send short messages which would cause reads and
> writes outside of the data buffer.
> 
> Fix this by adding the safe_data struct and the sdata_check() function when
> accessing packet data for input events, and add the usage of this to
> xpadone_process_packet(), which was vulnerable to OOB reads/writes.
> 
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
> ---
>  drivers/input/joystick/xpad.c | 115 ++++++++++++++++++++++++++----------------
>  1 file changed, 71 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834..c516860711a8 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -780,6 +780,24 @@ struct usb_xpad {
>  	bool delayed_init_done;
>  };
>  
> +struct safe_data {
> +	unsigned char *data;
> +	u32 len;
> +};
> +
> +/*
> + * Safe Data Check
> + *
> + * Returns the correct data when inside the array's bounds,
> + * returns 0 when accessing an out-of-bounds index.
> + */
> +static u8 sdata_check(struct safe_data *sdata, int idx)
> +{
> +	if (idx >= sdata->len)
> +		return 0;
> +	return sdata->data[idx];
> +}

I'd rather we had explicit length checks for various packets and skipped
the processing if the packet is short instead of making large number of
what can be considered repeated checks.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Input: xpad - add safer data access framework

[Griffin Kroah-Hartman]

Hi Dmitry,

On 8/3/26 6:23 PM, Dmitry Torokhov wrote:
> Hi Griffin,
>
> On Mon, Aug 03, 2026 at 05:07:24PM +0200, Griffin Kroah-Hartman wrote:
>> USB xpad devices could send short messages which would cause reads and
>> writes outside of the data buffer.
>>
>> Fix this by adding the safe_data struct and the sdata_check() function when
>> accessing packet data for input events, and add the usage of this to
>> xpadone_process_packet(), which was vulnerable to OOB reads/writes.
>>
>> Suggested-by: Ingo Molnar <mingo@kernel.org>
>> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
>> ---
>>   drivers/input/joystick/xpad.c | 115 ++++++++++++++++++++++++++----------------
>>   1 file changed, 71 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
>> index feb8f368f834..c516860711a8 100644
>> --- a/drivers/input/joystick/xpad.c
>> +++ b/drivers/input/joystick/xpad.c
>> @@ -780,6 +780,24 @@ struct usb_xpad {
>>   	bool delayed_init_done;
>>   };
>>   
>> +struct safe_data {
>> +	unsigned char *data;
>> +	u32 len;
>> +};
>> +
>> +/*
>> + * Safe Data Check
>> + *
>> + * Returns the correct data when inside the array's bounds,
>> + * returns 0 when accessing an out-of-bounds index.
>> + */
>> +static u8 sdata_check(struct safe_data *sdata, int idx)
>> +{
>> +	if (idx >= sdata->len)
>> +		return 0;
>> +	return sdata->data[idx];
>> +}
> I'd rather we had explicit length checks for various packets and skipped
> the processing if the packet is short instead of making large number of
> what can be considered repeated checks.

Sure thing, I can instead replicate something similar to my original 
patch here:

https://lore.kernel.org/all/20260727-xpadone_length_checks-v1-1-19aa9331e82d@kroah.com/

Ingo had suggested this method instead.


Thanks,

Griffin