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

[sashiko-bot@kernel.org]

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