Re: [PATCH v7 01/12] Input: xbox_gip - Add new driver for Xbox GIP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing and misordered work cancellations lead to Use-After-Free during device teardown and probe failure
- [High] Memory leak of input device due to dynamic devm allocation during logical device reset
- [High] Input devices are improperly destroyed during system suspend
- [High] Incorrect metadata version check logic causes parsing failures on older controllers
- [Low] Race condition when checking `!urb->anchor` to determine if an URB is idle
--

commit 16c368dc90338176b4ca692834653e9db715d0b4
Author: Vicki Pfau <vi@endrift.com>
Subject: Input: xbox_gip - Add new driver for Xbox GIP

This commit introduces a new driver for the Xbox One/Series controller
protocol, also known as the Gaming Input Protocol. It adds a scalable
framework for specialized Xbox devices by parsing dynamic capabilities
through device metadata.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 0000000000000..b662cd7bddfa2
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> +static int gip_send_raw_message(struct gip_attachment *attachment,
> +	uint8_t message_type, uint8_t flags, uint8_t seq, const uint8_t *bytes,
> +	int num_bytes)
> +{
[ ... ]
> +	for (i = 0; i < MAX_OUT_MESSAGES && !urb; i++) {
> +		if (!intf->out_queue[i].urb)
> +			continue;
> +		if (!intf->out_queue[i].urb->anchor)
> +			urb = &intf->out_queue[i];
> +	}

[Severity: Low]
Is this check for a free URB safe against concurrent completion handlers?

The USB core unanchors the URB via usb_unanchor_urb() just before executing
the completion handler. Checking !urb->anchor allows picking and modifying
this URB while its previous completion handler is still running, which can
lead to data races.

[ ... ]
> +static int gip_parse_device_metadata(struct device *dev,
> +	struct gip_metadata *metadata, const uint8_t *bytes, int num_bytes,
> +	int *offset)
> +{
[ ... ]
> +	bytes = &bytes[*offset];
> +	num_bytes -= *offset;
> +	if (num_bytes < sizeof(*header))
> +		return -EINVAL;

[Severity: High]
Does this unconditionally require the buffer to be large enough for the full
v1.1 header structure?

Older devices running v0.1 metadata protocols would send a naturally shorter
header, which would cause initialization to fail entirely here.

[ ... ]
> +	if (metadata->version_major > 1 || metadata->version_minor >= 1) {
> +		/* HID descriptor support added in metadata version 1.1 */

[Severity: High]
Will this logic incorrectly pass for older metadata versions like 0.1 and 0.2?

Because the condition uses an OR operator, a version with version_major == 0
and version_minor >= 1 evaluates to true. This would cause the driver to
attempt parsing v1.1 fields (hid_descriptor_offset) out of bounds on a
shorter v0.x header.

Could this be checked as:
version_major > 1 || (version_major == 1 && version_minor >= 1)?

[ ... ]
> +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]
Does this dynamically allocate devm resources that are leaked on logical
resets?

This allocates the input device using devres tied to the parent usb_interface.
When the controller is logically reset (e.g. handling a new
GIP_CMD_HELLO_DEVICE), gip_reset_attachment() explicitly unregisters this
device but does not destroy the devm allocation action.

[ ... ]
> +static void gip_free_devices(struct gip_attachment *attachment)
> +{
> +	struct input_dev *input;
[ ... ]
> +	if (input)
> +		input_unregister_device(input);
> +}

[Severity: High]
Could this leave the devres action tracking the input device intact on the
parent?

Successive re-enumerations will allocate additional input_dev instances
without freeing the old ones, leaking memory linearly over time until the
physical USB device is unbound.

[ ... ]
> +static int gip_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +{
[ ... ]
> +	/* Pre-create the first attachment, as it should always exist */
> +	attachment = gip_ensure_attachment(gip, 0);
> +	if (IS_ERR(attachment)) {
> +		gip_deinit_input(&gip->data);
> +		gip_deinit_output(&gip->data);
> +		if (gip->audio.intf) {
> +			gip_deinit_input(&gip->audio);
> +			gip_deinit_output(&gip->audio);
> +		}
> +		rc = PTR_ERR(attachment);
> +		goto err_cleanup;
> +	}
> +
> +	/* Don't init audio interface -- we aren't using it yet */
> +
> +	usb_set_intfdata(intf, gip);
> +	return 0;
> +
> +err_cleanup:
> +	devm_kfree(to_gip_device(gip), gip);
> +
> +	return rc;
> +}

[Severity: High]
Does this error path fail to cancel the gip->receive_message work?

If gip_ensure_attachment() fails, gip_deinit_input() kills the URB, but the
scheduled work is not canceled before freeing the gip structure via
devm_kfree(), leading to a use-after-free.

[ ... ]
> +static void gip_disconnect(struct usb_interface *intf)
> +{
> +	struct gip_device *gip = usb_get_intfdata(intf);
> +	int i;
> +
> +	if (!gip)
> +		return;
> +
> +	gip_shutdown(gip);
> +
> +	usb_kill_anchored_urbs(&gip->data.out_anchor);
> +	if (gip->audio.intf)
> +		usb_kill_anchored_urbs(&gip->audio.out_anchor);
> +
> +	gip_deinit_input(&gip->data);

[Severity: High]
Can a late URB completion handler re-queue work after gip_shutdown() runs?

gip_shutdown() synchronously cancels gip->receive_message and
attachment->rumble_work, but the input URB is not killed until later in
gip_deinit_input().

If the URB completes in this window, gip_urb_in() will re-queue the
receive_message work. usb_kill_urb() waits for the completion handler but
doesn't cancel newly queued work. gip_disconnect() then frees the memory,
causing the pending work to execute on a freed gip structure.

Additionally, gip_shutdown() cancels attachment->rumble_work before
input_unregister_device() runs. A concurrent userspace ioctl could
trigger gip_play_effect() to re-queue the work right before the attachment
is freed.

[ ... ]
> +static int gip_suspend(struct usb_interface *intf, pm_message_t message)
> +{
[ ... ]
> +	gip_shutdown(gip);
> +
> +	return 0;
> +}
> +
> +static int gip_resume(struct usb_interface *intf)
> +{
> +	struct gip_device *gip = usb_get_intfdata(intf);
> +
> +	if (!gip)
> +		return 0;
> +
> +	if (usb_submit_urb(gip->data.urb_in, GFP_KERNEL))
> +		return -EIO;
> +
> +	return 0;
> +}

[Severity: High]
Are input devices destroyed during suspend and never properly restored?

gip_suspend() calls gip_shutdown(), which unregisters and destroys all input
devices. Upon gip_resume(), the driver only resubmits the input URB but fails
to recreate the input devices.

If the controller doesn't automatically send a GIP_CMD_HELLO_DEVICE, the input
device is permanently lost. Even if it does, it will create a brand new evdev
node, breaking the state of hotplug-unaware userspace applications across
system suspend/resume cycles.

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