[PATCH v2 0/3] xpad packet size checks

[PATCH v2 0/3] xpad packet size checks

[Griffin Kroah-Hartman]

Small patchset to add size checks to the process_packet() functions in
xpad, ensuring that OOB data is not accessed.

Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
Changes in v2:
- Now a patchset, covering all process_packet functions
- Link to v1: https://patch.msgid.link/20260727-xpadone_length_checks-v1-1-19aa9331e82d@kroah.com

To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Griffin Kroah-Hartman (3):
      Input: xpad - add out-of-bounds checks for xpadone
      Input: xpad - add packet length check for xpad_process_packet()
      Input: xpad - add size checks to xpad360 packet processing

 drivers/input/joystick/xpad.c | 49 +++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 11 deletions(-)
---
base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
change-id: 20260727-xpadone_length_checks-08120b97764f

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

[PATCH v2 1/3] Input: xpad - add out-of-bounds checks for xpadone

[Griffin Kroah-Hartman]

Add size checks for the "len" variable in xpadone_process_packet().
This prevents out-of-bounds accesses to the "data" buffer, as "len"
comes directly from the hardware.

Assisted-by: gkh_clanker_t1000
Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
 drivers/input/joystick/xpad.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index feb8f368f834..9ce792503b3a 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 < 2)
+		return;
+	len = min(len, XPAD_PKT_LEN);
+
 	/* 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 +1058,8 @@ 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,9 +1087,13 @@ 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));
+		do_sync = true;
+
 		if (xpad->mapping & MAP_SHARE_BUTTON) {
 			u32 offset = (xpad->mapping & MAP_SHARE_OFFSET) ? 26 : 18;
 
@@ -1145,13 +1157,18 @@ 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) {
+			if (len < 35)
+				goto exit;
 			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 exit;
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1165,6 +1182,8 @@ 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 exit;
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1178,6 +1197,8 @@ 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 exit;
 				/* Mute paddles if controller has a custom mapping applied.
 				 * Checked by comparing the current mapping
 				 * config against the factory mapping config
@@ -1197,7 +1218,7 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
 
 		do_sync = true;
 	}
-
+exit:
 	if (do_sync)
 		input_sync(dev);
 }

-- 
2.55.0

Re: [PATCH v2 1/3] Input: xpad - add out-of-bounds checks for xpadone

[sashiko-bot]

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

Pre-existing issues:
- [High] The xpadone_process_packet function processes GIP_CMD_ANNOUNCE packets by directly calling xpad_start_input() from within the atomic xpad_irq_in URB completion handler, causing a kernel panic due to blocking operations.
--

--- Patch [1]: [PATCH v2 1/3] Input: xpad - add out-of-bounds checks for xpadone ---
commit d812a96c06a9e629fcf6e5c81511be2d91b754b4
Author: Griffin Kroah-Hartman <griffin@kroah.com>

Input: xpad - add out-of-bounds checks for xpadone
    
This commit adds size checks for the "len" variable in xpadone_process_packet().
It prevents out-of-bounds accesses to the "data" buffer by ensuring the packet
length is sufficient for various commands.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..9ce792503b3a4 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
[ ... ]
> @@ -1079,9 +1087,13 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
>  					 error);
>  		}

[Severity: High]
This is a pre-existing issue, but does calling xpad_start_input() here cause
a deadlock or scheduling while atomic bug?

In the preceding GIP_CMD_ANNOUNCE block (shown partially in the context above),
xpad_start_input() is called from xpadone_process_packet():

drivers/input/joystick/xpad.c:xpadone_process_packet() {
    ...
    } else if (data[0] == GIP_CMD_ANNOUNCE) {
        ...
        error = xpad_start_input(xpad);
    ...
}

Since xpadone_process_packet() is called from the atomic URB completion handler
(xpad_irq_in), wouldn't this execution context be atomic?

Looking at xpad_start_input(), it uses GFP_KERNEL and can call usb_kill_urb()
on xpad->irq_in if xpad_start_xbox_one() fails:

drivers/input/joystick/xpad.c:xpad_start_input() {
    if (usb_submit_urb(xpad->irq_in, GFP_KERNEL))
        return -EIO;

    if (xpad->xtype == XTYPE_XBOXONE) {
        error = xpad_start_xbox_one(xpad);
        if (error) {
            usb_kill_urb(xpad->irq_in);
            return error;
        }
    }
    ...
}

Could the GFP_KERNEL allocation cause scheduling while atomic? Also, if an error
occurs, wouldn't usb_kill_urb(xpad->irq_in) wait on the very URB handler that is
currently executing, leading to a deadlock?

>  	} 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/20260805-xpadone_length_checks-v2-0-f3e925e0280b@kroah.com?part=1

[PATCH v2 2/3] Input: xpad - add packet length check for xpad_process_packet()

[Griffin Kroah-Hartman]

Add a check to ensure that the packet being processed is not less than
20 bytes, which would enable an OOB read if true.

Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
 drivers/input/joystick/xpad.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 9ce792503b3a..83bc361b15f7 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -795,10 +795,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;
 
+	if (len < 20)
+		return;
+
 	if (!(xpad->mapping & MAP_STICKS_TO_NULL)) {
 		/* left stick */
 		input_report_abs(dev, ABS_X,
@@ -1259,7 +1262,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

Re: [PATCH v2 2/3] Input: xpad - add packet length check for xpad_process_packet()

[sashiko-bot]

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

New issues:
- [Low] Using `min(len, XPAD_PKT_LEN)` causes a compiler warning due to mismatched types.

Pre-existing issues:
- [High] The patch adds a packet length check to `xpad_process_packet` to prevent OOB reads, but misses identical vulnerabilities in `xpad360_process_packet` and `xpad360w_process_packet`.
- [High] The `xpad->work` workqueue item can execute after `xpad` is freed in the `xpad_probe()` error path for wireless Xbox 360 controllers.
--

--- Patch [2]: [PATCH v2 2/3] Input: xpad - add packet length check for xpad_process_packet() ---
commit 871137dca87547e72930981b59cce6b2b69f5992
Author: Griffin Kroah-Hartman <griffin@kroah.com>

Input: xpad - add packet length check for xpad_process_packet()

This patch adds a bounds check to xpad_process_packet() to ensure that the
processed packet is at least 20 bytes long. This prevents a potential
out-of-bounds read if a shorter packet is received.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 9ce792503b3a4..83bc361b15f76 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
[ ... ]
> @@ -1259,7 +1262,7 @@ static void xpad_irq_in(struct urb *urb)
>  		xpadone_process_packet(xpad, 0, xpad->idata, urb->actual_length);

[Severity: Low]
Does the min() check in xpadone_process_packet() cause a compiler warning?

Comparing a u32 (len) with a literal 64 (XPAD_PKT_LEN) using min() causes a
'comparison of distinct pointer types lacks a cast' warning due to strict
type checking. Would min_t(u32, len, XPAD_PKT_LEN) be more appropriate here
to avoid build failures when compiled with -Werror?

>  		break;
>  	default:
> -		xpad_process_packet(xpad, 0, xpad->idata);
> +		xpad_process_packet(xpad, 0, xpad->idata, urb->actual_length);

[Severity: High]
This isn't a bug introduced by this patch, but does this same vulnerability
exist for the Xbox 360 packet handlers?

In xpad_irq_in(), a malicious or malfunctioning USB device sending short
interrupt packets can cause xpad360_process_packet() or
xpad360w_process_packet() to be called without passing or verifying
urb->actual_length. This allows unconditional reads of up to 14 bytes
from the xpad->idata buffer, potentially reading stale data in the same
way this patch prevents for xpad_process_packet().

[Severity: High]
This isn't a bug introduced by this patch, but can the xpad->work workqueue
execute after xpad is freed in the xpad_probe() error path?

If an XTYPE_XBOX360W controller is detected during probe,
xpad360w_start_input() submits xpad->irq_in and then calls
xpad_inquiry_pad_presence(). If this fails, it kills the URB and returns
an error, causing xpad_probe() to free xpad via kfree().

However, if the USB device completes an interrupt transfer immediately
after irq_in is submitted, xpad_irq_in() will call
xpad360w_process_packet(), which schedules xpad->work:

drivers/input/joystick/xpad.c:xpad360w_process_packet() {
    ...
    if (xpad->pad_present != present) {
        xpad->pad_present = present;
        schedule_work(&xpad->work);
    }
    ...
}

Since the error path doesn't call cancel_work_sync(&xpad->work), can this
result in a use-after-free when xpad_presence_work() dereferences the freed
xpad struct?

>  	}
>  
>  exit:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-xpadone_length_checks-v2-0-f3e925e0280b@kroah.com?part=2

[PATCH v2 3/3] Input: xpad - add size checks to xpad360 packet processing

[Griffin Kroah-Hartman]

Add size checks to xbox360_process_packet() and
xbox360w_process_packet() to prevent OOB reads with the data[] array.

Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
 drivers/input/joystick/xpad.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 83bc361b15f7..6f15eafc6477 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -870,10 +870,10 @@ 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)
 {
 	/* valid pad data */
-	if (data[0] != 0x00)
+	if (data[0] != 0x00 || len < 14)
 		return;
 
 	/* digital pad */
@@ -998,11 +998,14 @@ 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;
 	bool present;
 
+	if (len < 2)
+		return;
+
 	/* Presence change */
 	if (data[0] & 0x08) {
 		present = (data[1] & 0x80) != 0;
@@ -1019,8 +1022,8 @@ static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned cha
 
 	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();
 }
 
@@ -1253,10 +1256,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 v2 3/3] Input: xpad - add size checks to xpad360 packet processing

[sashiko-bot]

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

New issues:
- [Medium] Logical out-of-bounds read due to incorrect order of length check.

Pre-existing issues:
- [High] Use-After-Free or NULL Pointer Dereference in error logging path.
--

--- Patch [3]: [PATCH v2 3/3] Input: xpad - add size checks to xpad360 packet processing ---
commit 2a90e36a133f256901ec08114f8164654060dd73
Author: Griffin Kroah-Hartman <griffin@kroah.com>

Input: xpad - add size checks to xpad360 packet processing

This commit adds size checks to xbox360_process_packet() and
xbox360w_process_packet() to prevent out-of-bounds reads with the data array.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 83bc361b15f76..6f15eafc64775 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -870,10 +870,10 @@ 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)
>  {
>  	/* valid pad data */
> -	if (data[0] != 0x00)
> +	if (data[0] != 0x00 || len < 14)

[Severity: Medium]
Will this evaluate data[0] before checking len? C logical OR short-circuits
from left to right, meaning data[0] could be read from the DMA buffer
before we verify len is large enough to contain it.

If we receive a zero-length packet (ZLP) where urb->actual_length is 0,
could this result in an out-of-bounds read of the data array?

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but I noticed a potential use-after-free or NULL pointer dereference in the
error logging path when xpad_init_input() fails.

If input_allocate_device() fails, xpad->dev remains NULL:

drivers/input/joystick/xpad.c:xpad_init_input() {
    ...
	input_dev = input_allocate_device();
	if (!input_dev)
		return -ENOMEM;

	xpad->dev = input_dev;
    ...
}

Or if a later initialization step fails, xpad_init_input() frees input_dev
but leaves xpad->dev pointing to the freed memory:

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

If xpad_init_input() returns an error, xpad_presence_work() logs it:

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

Could this dev_err() call dereference the NULL or dangling xpad->dev pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-xpadone_length_checks-v2-0-f3e925e0280b@kroah.com?part=3