[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

[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