Re: [PATCH v4 08/12] Input: xbox_gip - Add arcade stick support

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

New issues:
- [Medium] Memory leak of `gip_arcade_stick_info` on device disconnect because the core framework never invokes the `.remove()` callback.
- [Medium] Missing event synchronization when an input report is truncated (2 to 5 bytes), leaving spurious events stuck in the input subsystem buffer.

Pre-existing issues:
- [High] Use-After-Free of `attachment->input` if `input_register_device` fails.
- [High] Use-After-Free and DMA corruption in the teardown path due to failing to stop outbound URBs and the `send_fragment` work.
--

--- Patch [8]: [PATCH v4 08/12] Input: xbox_gip - Add arcade stick support ---
commit 0a0bb97c59622a30366ca74123a1a5db969d555d
Author: Vicki Pfau <[email protected]>

Input: xbox_gip - Add arcade stick support
    
This adds support for the arcade stick device type. Currently I'm only
aware of one Xbox controller that uses this device type, the Razer Atrox,
so testing coverage is limited.

> diff --git a/drivers/input/joystick/gip/gip-arcade-stick.c b/drivers/input/joystick/gip/gip-arcade-stick.c
> new file mode 100644
> index 0000000000000..11ededfab8555
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-arcade-stick.c
> @@ -0,0 +1,173 @@

[ ... ]

> +static int gip_arcade_stick_probe(struct gip_attachment *attachment)
> +{
> +	struct gip_arcade_stick_info *info = kzalloc(sizeof(*info), GFP_KERNEL);
> +
> +	if (!info)
> +		return -ENOMEM;
> +
> +	attachment->quirks |= GIP_QUIRK_SWAP_LB_RB;
> +	attachment->driver_data = info;
> +
> +	return 0;
> +}
> +
> +static void gip_arcade_stick_remove(struct gip_attachment *attachment)
> +{
> +	kfree(attachment->driver_data);
> +	attachment->driver_data = NULL;
> +}

[Severity: Medium]
Will this leak gip_arcade_stick_info on device disconnect? Looking at the
core framework in gip-core.c, it doesn't appear to invoke the .remove()
callback of any attachment driver during device teardown (gip_disconnect),
so the memory allocated here might never get freed.

[ ... ]

> +static int gip_handle_arcade_stick_report(struct gip_attachment *attachment,
> +	struct input_dev *input, const uint8_t *bytes, int num_bytes)
> +{
> +	struct gip_arcade_stick_info *info = attachment->driver_data;
> +	int rc;
> +
> +	if (!info)
> +		return -ENODEV;
> +
> +	rc = gip_driver_navigation.handle_input_report(attachment, input, bytes, num_bytes);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (num_bytes < 6) {
> +		gip_dbg(attachment, "Discarding too-short input report\n");
> +		return -EINVAL;
> +	}

[Severity: Medium]
Can this leave unsynchronized events in the input buffer if an input report is
truncated (between 2 and 5 bytes)? 

The base handler gip_driver_navigation.handle_input_report() may successfully
process the first 2 bytes, report multiple EV_KEY events, and return 0. If
num_bytes is then evaluated as < 6, returning -EINVAL here causes the
core caller to skip input_sync().

[ ... ]

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 765d70033f205..da22545662e21 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

> @@ -1554,7 +1556,7 @@ static int gip_handle_command_raw_report(struct gip_attachment *attachment,
>  	return 0;
>  }
>  
> -static int gip_setup_input_device(struct gip_attachment *attachment)
> +int gip_setup_input_device(struct gip_attachment *attachment)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but can attachment->input become a
dangling pointer if input_register_device() fails? 

In gip_setup_input_device(), attachment->input is assigned the newly
allocated input device:

	rcu_assign_pointer(attachment->input, input);
	rc = input_register_device(input);
	if (rc)
		goto err_free_device;
    ...
err_free_device:
	input_free_device(input);
	return rc;

If registration fails, the error path frees the input device but doesn't set
attachment->input back to NULL. Later disconnects or input packets could
dereference the freed pointer.

>  	struct input_dev *input;
>  	int rc;

[Severity: High]
This is a pre-existing issue, but does the teardown path in gip_disconnect()
cause a use-after-free or DMA corruption?

Looking at gip_disconnect() in drivers/input/joystick/gip/gip-core.c:

static void gip_disconnect(struct usb_interface *intf)
{
    ...
	usb_kill_urb(gip->data.urb_in);
	if (gip->audio.intf)
		usb_kill_urb(gip->audio.urb_in);

	gip_shutdown(gip);

	spin_lock_irqsave(&gip->message_lock, flags);
	gip_deinit_input(&gip->data);
	gip_deinit_output(&gip->data);

It calls usb_kill_urb() for inbound URBs, but misses outbound URBs
(intf->out_queue[i].urb). It then calls gip_deinit_output(), which
unconditionally frees the coherent DMA buffers and the URB structures. If an
outbound URB is in flight, the USB host controller might perform DMA reads from
the freed memory.

Additionally, gip_disconnect() doesn't seem to cancel the
&intf->send_fragment work, meaning if an outbound URB completes, it could
schedule the work to access the freed gip device structure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8