Re: [PATCH v7 02/12] Input: xpad - Remove Xbox One support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Accumulation of leaked input_dev structures in the devres list across logical device resets.
- [High] Race condition in gip_shutdown() where asynchronous force-feedback work is cancelled before the input device is unregistered, leading to a Use-After-Free.
- [Medium] gip_parse_metadata() returns success (0) instead of an error code when the metadata message is unexpectedly truncated, causing the caller to accept an uninitialized metadata structure.
--

commit 3f01a3076f0a08c3208bada0c0eb3d4a4be23e2c
Author: Vicki Pfau <vi@endrift.com>

    Input: xpad - Remove Xbox One support
    
    It has been superseded by xbox_gip. As the new driver is already at feature
    parity, removing the fairly rough Xbox One support from xpad is safe and
    will prevent any potential conflicts.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> -static bool gip_parse_metadata(struct device *dev,
> +static int gip_parse_metadata(struct device *dev,
>  	struct gip_metadata *metadata, const uint8_t *bytes, int num_bytes)
>  {
>  	int header_size;
[ ... ]
>  	rc = gip_parse_device_metadata(dev, metadata, bytes, num_bytes, &offset);
>  	if (rc)
>  		goto parse_err;
>  
>  	if (offset >= num_bytes)
>  		goto parse_err;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this error path return 
success instead of an error code?

If the offset exceeds num_bytes, rc is still 0 from the preceding successful
call to gip_parse_device_metadata(). Jumping to parse_err will return 0.

Could this cause the driver to accept a zeroed-out metadata structure
if a misbehaving device sends a truncated message?

[ ... ]
>  static int gip_setup_input_device(struct gip_attachment *attachment)
>  {
>  	struct input_dev *input;
>  	int rc;
>  
>  	if (!attachment->driver || !attachment->driver->setup_input)
>  		return -ENODEV;
>  
>  	rcu_read_lock();
>  	input = rcu_dereference(attachment->input);
>  	rcu_read_unlock();
>  	if (input)
>  		return 0;
>  
>  	input = devm_input_allocate_device(to_gip_device(attachment));

[Severity: High]
This is a pre-existing issue, but does this lead to a memory leak of
input_dev structures?

When the controller resets (e.g. repeatedly sending GIP_CMD_HELLO_DEVICE),
this calls input_unregister_device(), but because the device was allocated
with devm_input_allocate_device() on the parent USB interface, the memory is
intentionally deferred to devres teardown.

Since the devm reference isn't dropped, if the driver allocates a new
input_dev during re-initialization, it seems the old structures will accumulate
indefinitely. Could this be triggered by an untrusted USB device to
out-of-memory the host kernel?

[ ... ]
>  static void gip_shutdown(struct gip_device *gip)
>  {
>  	int i;
>  
>  	cancel_work_sync(&gip->receive_message);
>  
>  	for (i = 0; i < MAX_ATTACHMENTS; i++) {
>  		struct gip_attachment *attachment;
>  
>  		mutex_lock(&gip->attachment_lock);
>  		attachment = gip->attachments[i];
>  		mutex_unlock(&gip->attachment_lock);
>  
>  		if (!attachment)
>  			continue;
>  
>  		cancel_delayed_work_sync(&attachment->metadata_next);
>  		cancel_delayed_work_sync(&attachment->in_fragment_timeout);
>  		cancel_work_sync(&attachment->rumble_work);
>  
>  		guard(mutex)(&attachment->lock);
>  		gip_reset_attachment(attachment);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race condition
during driver unbind?

In gip_shutdown(), cancel_work_sync(&attachment->rumble_work) is called
before the input device is unregistered. If an active ff-memless background
timer fires right after this, it can invoke gip_play_effect() and re-schedule
the work.

gip_shutdown() then acquires the lock and calls gip_reset_attachment(), which
uses the non-synchronous cancel_work():

drivers/input/joystick/gip/gip-core.c:gip_reset_attachment() {
    ...
	cancel_work(&attachment->rumble_work);
    ...
}

If the re-scheduled work has already started executing, it will block on
attachment->lock (held by gip_shutdown). Once gip_shutdown() unregisters the
device and the attachment is freed by devres, the blocked work would resume
and access freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912030426.2997003-1-vi@endrift.com?part=2