[PATCH v2] Input: iforce - validate input packet lengths

[PATCH v2] Input: iforce - validate input packet lengths

[Pengpeng Hou]

iforce_process_packet() reads fixed fields from joystick, wheel and status
packets without first checking their lengths. In particular, the shared
hats-and-buttons helper unconditionally reads data[6]. The status tail is
a sequence of 16-bit effect addresses, but an incomplete final address is
also consumed. A successful zero-length USB URB additionally reads the
packet ID before the common parser is called.

Reject the zero-length USB transfer, require the seven-byte joystick and
wheel prefixes and the two-byte status prefix, and consume only complete
status-tail addresses.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260706092016.78176-1-pengpeng@iscas.ac.cn/
- cover the data[6] access in the shared hats-and-buttons helper
- cover zero-length USB completions and incomplete status-tail u16 values
- rebase onto v7.2-rc4

 drivers/input/joystick/iforce/iforce-packets.c | 11 ++++++++++-
 drivers/input/joystick/iforce/iforce-usb.c     |  3 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c
index effa76bfd8f9..e8a34d68a51f 100644
--- a/drivers/input/joystick/iforce/iforce-packets.c
+++ b/drivers/input/joystick/iforce/iforce-packets.c
@@ -155,6 +155,9 @@ void iforce_process_packet(struct iforce *iforce,
 	switch (packet_id) {
 
 	case 0x01:	/* joystick position data */
+		if (len < 7)
+			break;
+
 		input_report_abs(dev, ABS_X,
 				 (__s16) get_unaligned_le16(data));
 		input_report_abs(dev, ABS_Y,
@@ -170,6 +173,9 @@ void iforce_process_packet(struct iforce *iforce,
 		break;
 
 	case 0x03:	/* wheel position data */
+		if (len < 7)
+			break;
+
 		input_report_abs(dev, ABS_WHEEL,
 				 (__s16) get_unaligned_le16(data));
 		input_report_abs(dev, ABS_GAS,   255 - data[2]);
@@ -181,6 +187,9 @@ void iforce_process_packet(struct iforce *iforce,
 		break;
 
 	case 0x02:	/* status report */
+		if (len < 2)
+			break;
+
 		input_report_key(dev, BTN_DEAD, data[0] & 0x02);
 		input_sync(dev);
 
@@ -200,7 +209,7 @@ void iforce_process_packet(struct iforce *iforce,
 			}
 		}
 
-		for (j = 3; j < len; j += 2)
+		for (j = 3; j + sizeof(u16) <= len; j += 2)
 			mark_core_as_ready(iforce, get_unaligned_le16(data + j));
 
 		break;
diff --git a/drivers/input/joystick/iforce/iforce-usb.c b/drivers/input/joystick/iforce/iforce-usb.c
index 0482eaaecf39..f04370e4191e 100644
--- a/drivers/input/joystick/iforce/iforce-usb.c
+++ b/drivers/input/joystick/iforce/iforce-usb.c
@@ -158,6 +158,9 @@ static void iforce_usb_irq(struct urb *urb)
 		goto exit;
 	}
 
+	if (!urb->actual_length)
+		goto exit;
+
 	iforce_process_packet(iforce, iforce_usb->data_in[0],
 			      iforce_usb->data_in + 1, urb->actual_length - 1);
 

Re: [PATCH v2] Input: iforce - validate input packet lengths

[sashiko-bot]

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

Pre-existing issues:
- [High] Data race on core_effect->flags due to non-atomic __test_and_set_bit.
- [Medium] Missing input_sync() after reporting force-feedback status (EV_FF_STATUS) events.
--

--- Patch [1]: [PATCH v2] Input: iforce - validate input packet lengths ---
commit 32891702a73654a7ce9748db1e4baa8f0cbb9362
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

Input: iforce - validate input packet lengths

iforce_process_packet() reads fixed fields from joystick, wheel and status
packets without first checking their lengths. In particular, the shared
hats-and-buttons helper unconditionally reads data[6]. The status tail is
a sequence of 16-bit effect addresses, but an incomplete final address is
also consumed. A successful zero-length USB URB additionally reads the
packet ID before the common parser is called.

Reject the zero-length USB transfer, require the seven-byte joystick and
wheel prefixes and the two-byte status prefix, and consume only complete
status-tail addresses.

> diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c
> index effa76bfd8f9a..e8a34d68a51f2 100644
> --- a/drivers/input/joystick/iforce/iforce-packets.c
> +++ b/drivers/input/joystick/iforce/iforce-packets.c
[ ... ]
> @@ -200,7 +209,7 @@ void iforce_process_packet(struct iforce *iforce,
>  			}
>  		}

[Severity: Medium]
This is a pre-existing issue, but is there a missing input_sync() after the
input_report_ff_status() calls just above this code?

The input subsystem guidelines require that every logical group of events must
be followed by input_sync(), or the events stay in the buffer. While there is
an input_sync() earlier for BTN_DEAD, the EV_FF_STATUS events emitted here
might remain buffered and not reach userspace until a subsequent event
triggers another sync.

> -		for (j = 3; j < len; j += 2)
> +		for (j = 3; j + sizeof(u16) <= len; j += 2)
>  			mark_core_as_ready(iforce, get_unaligned_le16(data + j));

[Severity: High]
This isn't a bug introduced by this patch, but does the call to
mark_core_as_ready() here expose a data race with iforce_upload_effect()?

The interrupt completion handler calls mark_core_as_ready(), which uses atomic
bit operations on core_effects[i].flags:

drivers/input/joystick/iforce/iforce-packets.c:mark_core_as_ready() {
    ...
    clear_bit(FF_CORE_UPDATE, iforce->core_effects[i].flags);
    ...
}

However, iforce_upload_effect() in iforce-main.c executes in process context
with ff->mutex held (which does not disable interrupts) and uses a non-atomic
read-modify-write:

drivers/input/joystick/iforce/iforce-main.c:iforce_upload_effect() {
    ...
    if (__test_and_set_bit(FF_CORE_IS_USED, core_effect->flags)) {
    ...
}

If the interrupt executes between the read and write phases of
__test_and_set_bit(), the interrupt's modification could be overwritten.
Could this potentially leave FF_CORE_UPDATE stuck at 1 and render the effect
impossible to update again?

>  		break;

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