Re: [PATCH v6 04/12] Input: xbox_gip - Add HID relaying

[sashiko-bot@kernel.org]

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

New issues:
- [High] gip_handle_command_hid_report() uses hid_input_report() instead of hid_safe_input_report(), bypassing buffer size checks.
- [Medium] gip_create_hdev() contains a redundant synchronize_rcu() call after publishing the newly allocated hdev pointer.
- [Medium] The hid_device's uniq field is not initialized before hid_add_device() is called.
- [Low] gip_free_devices() leaks the RCU-protected hdev pointer outside the read-side critical section, violating the RCU API contract.

Pre-existing issues:
- [Critical] gip_disconnect() cancels the receive workqueue before killing the active URB, introducing a race condition that causes Use-After-Free of the gip_device.
- [Critical] The LED classdev is embedded in the attachment but registered using the devres of the ephemeral input_dev, leading to list corruption upon device reset.
- [High] Resetting the attachment leaks input_dev structures because devm_input_allocate_device() is used but the memory is never explicitly freed before reallocation.
--

commit 4a45548f46c49c18ea22bd85490b3cf588327135
Author: Vicki Pfau <vi@endrift.com>

Input: xbox_gip - Add HID relaying
    
GIP allows tunneling of HID packets, with the HID descriptor embedded in
the GIP metadata exchanged during the initial handshake. This patch creates
a hid_device for this HID descriptor if found, as well as relaying the HID
packets.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index a5ed5f3bf3fed..dc174560ee49f 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

> @@ -1310,6 +1358,50 @@ static int gip_init_input_device(struct gip_attachment *attachment)
>  	return rc;
>  }
>  
> +static int gip_create_hdev(struct gip_attachment *attachment)
> +{
> +	struct hid_device *hdev;
> +	int rc;
> +

[ ... ]

> +	hdev->dev.parent = to_gip_device(attachment);
> +	hdev->driver_data = attachment;
> +	if (attachment->name)
> +		strscpy(hdev->name, attachment->name);
> +	else
> +		strscpy(hdev->name, "Xbox Chatpad");
> +	strscpy(hdev->phys, attachment->phys);

[Severity: Medium]
Does this code forget to initialize hdev->uniq?

The HID subsystem documentation explicitly mandates that transport drivers
fully initialize fields including uniq before calling hid_add_device().
While the input_dev correctly populates this, it appears to be omitted here,
which leaves the device missing standard sysfs attributes.

> +	rc = hid_add_device(hdev);
> +	if (rc) {
> +		gip_err(attachment, "HID device add failed: %d\n", rc);
> +		hid_destroy_device(hdev);
> +	} else {
> +		rcu_assign_pointer(attachment->hdev, hdev);
> +		synchronize_rcu();

[Severity: Medium]
Is it necessary to block the thread for an RCU grace period here?

Since rcu_assign_pointer() publishes the newly allocated hdev pointer
(replacing NULL or a synchronized teardown state), and no memory is being
reclaimed at this point, this synchronize_rcu() call appears completely
redundant and needlessly delays device initialization.

> +	}
> +
> +	return rc;
> +}

[ ... ]

> @@ -1393,19 +1489,25 @@ static void gip_fragment_timeout(struct work_struct *work)
>  static void gip_free_devices(struct gip_attachment *attachment)
>  {
>  	struct input_dev *input;
> +	struct hid_device *hdev;
>  
>  	if (attachment->driver && attachment->driver->remove)
>  		attachment->driver->remove(attachment);
>  
>  	rcu_read_lock();
>  	input = rcu_dereference(attachment->input);
> +	hdev = rcu_dereference(attachment->hdev);
>  	rcu_read_unlock();
>  
>  	rcu_assign_pointer(attachment->input, NULL);
> +	rcu_assign_pointer(attachment->hdev, NULL);
>  	synchronize_rcu();
>  
>  	if (input)
>  		input_unregister_device(input);
> +
> +	if (hdev)
> +		hid_destroy_device(hdev);

[Severity: Low]
Can this leak the RCU-protected hdev pointer outside the read-side
critical section?

The hdev pointer is extracted using rcu_dereference() under rcu_read_lock(),
but it is utilized outside the critical section to destroy it. While this
might be functionally safe here because the thread is the exclusive updater
and synchronizes before destruction, it violates RCU API rules. Should this
use rcu_access_pointer() or rcu_dereference_protected() instead?

>  }
>  
>  static void gip_reset_metadata(struct gip_attachment *attachment)

[ ... ]

> @@ -1770,9 +1872,16 @@ static int gip_handle_command_firmware(struct gip_attachment *attachment,
>  static int gip_handle_command_hid_report(struct gip_attachment *attachment,
>  	const struct gip_header *header, uint8_t *bytes, int num_bytes)
>  {
> -	gip_warn(attachment, "Unimplemented HID report message\n");
> +	struct hid_device *hdev;
>  
> -	return -EOPNOTSUPP;
> +	guard(rcu)();
> +	hdev = rcu_dereference(attachment->hdev);
> +	if (hdev)
> +		return hid_input_report(hdev, HID_INPUT_REPORT, bytes, num_bytes, true);

[Severity: High]
Could this cause an out-of-bounds read in the HID core?

The raw byte array from untrusted hardware input is passed directly to
hid_input_report() without providing the allocated buffer size. The HID
subsystem explicitly directs transport drivers to use hid_safe_input_report()
with the allocated buffer size to ensure proper boundary checks are performed
against the report descriptor.

> +
> +	gip_warn(attachment, "Got HID report with no HID descriptor\n");
> +
> +	return -EPROTO;
>  }

While reviewing this file, I also noticed a few pre-existing regressions
elsewhere in the codebase:

> 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_urb(gip->data.urb_in);

[Severity: Critical]
This is a pre-existing issue, but does this destruction order cause a
use-after-free of the gip_device?

The cleanup function gip_shutdown() cancels the receive_message workqueue
before usb_kill_urb() is called. If the urb_in completes concurrently after
the workqueue is canceled but before the URB is killed, it will reschedule
the receive_message work. This work then executes concurrently with the rest
of gip_disconnect(), which frees the gip_device.

> 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 isn't a bug introduced by this patch, but are input_dev structures
leaked when the attachment is reset?

The input_dev is allocated using devm_input_allocate_device(). During a
protocol reset, input_unregister_device() is explicitly called, but this
does not free the devres-managed memory until the parent USB interface
unbinds. Re-allocating the device on subsequent resets appears to accumulate
leaked structures in memory.

> static int gip_guide_led_probe(struct gip_attachment *attachment, struct device *dev)
> {
> 	int rc = 0;
> 
> 	if (!gip_supports_system_message(attachment, GIP_CMD_LED, false))
> 		return 0;
> 
> 	if (attachment->features & GIP_FEATURE_GUIDE_COLOR) {
> 		struct mc_subled *mc_led_info;
> 		struct led_classdev_mc *mc_cdev = &attachment->guide_led.color;
> 		struct led_classdev *cdev = &mc_cdev->led_cdev;
> 
> 		mc_led_info = devm_kcalloc(dev, 4,
[ ... ]
> 		if (!rc)
> 			rc = devm_led_classdev_multicolor_register(dev,
> 				mc_cdev);

[Severity: Critical]
This is a pre-existing issue, but does tying the embedded LED classdev
registration to the ephemeral input_dev lead to list corruption?

The led_classdev is embedded in the attachment structure, but it is
registered using devm_led_classdev_register() tied to the input_dev's
lifecycle. On reset, the input_dev is unregistered, but its devres cleanup
is deferred until the last userspace reference is closed. If the driver
reallocates the input_dev and re-registers the exact same embedded
led_classdev structure before cleanup completes, it corrupts the LED
subsystem's linked lists.

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