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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] gip_reset_metadata() frees hid_descriptor before destroying the HID device, allowing a concurrent UAF during HID probe.
- [High] Fragmented HID reports are processed with a payload size of 0, causing the entire assembled report to be truncated and silently dropped.
- [Medium] Useless synchronize_rcu() call delays device initialization.
- [Medium] The hid_device fields `version`, `country`, and `uniq` are not initialized before calling `hid_add_device()`.

Pre-existing issues:
- [Critical] Use-after-free in receive worker due to incorrect shutdown ordering.
--

commit e6352eb8b59489fd2119b2f3d053cfef65ac2c92
Author: Vicki Pfau <vi@endrift.com>
Subject: Input: xbox_gip - Add HID relaying

This patch allows tunneling of HID packets by creating a hid_device for
the HID descriptor embedded in the GIP metadata, and relays the packets.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 23ab0768580f5..3b033fe65dc40 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
> @@ -560,6 +560,54 @@ int gip_send_vendor_message(struct gip_attachment *attachment,
>  		bytes, num_bytes);
>  }
>  
> +static int gip_hid_ll_parse(struct hid_device *hdev)
> +{
> +	struct gip_attachment *attachment = hdev->driver_data;
> +
> +	return hid_parse_report(hdev,
> +		attachment->metadata.device.hid_descriptor,
> +		attachment->metadata.device.hid_descriptor_size);
> +}

[Severity: Critical]
Can this lead to a use-after-free during a concurrent HID probe?

If a device reset or hotplug occurs, gip_reset_metadata() frees the
metadata before destroying the child devices:

gip_reset_metadata()
    gip_metadata_free(to_gip_device(attachment), &attachment->metadata);
    gip_free_devices(attachment);

Since gip_metadata_free() synchronously frees hid_descriptor, if the HID
core concurrently probes the device and calls gip_hid_ll_parse() before
gip_free_devices() unregisters the HID device, it would dereference the
freed descriptor memory.

[ ... ]

> @@ -1303,6 +1351,48 @@ 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;
> +
> +	if (!attachment->metadata.device.hid_descriptor)
> +		return 0;
> +
> +	rcu_read_lock();
> +	hdev = rcu_dereference(attachment->hdev);
> +	rcu_read_unlock();
> +	if (hdev)
> +		return 0;
> +
> +	hdev = hid_allocate_device();
> +
> +	if (IS_ERR(hdev))
> +		return PTR_ERR(hdev);
> +
> +	hdev->ll_driver = &gip_hid_ll_driver;
> +	hdev->bus = BUS_GIP;
> +	hdev->vendor = attachment->vendor_id;
> +	hdev->product = attachment->product_id;
> +	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);
> +	rc = hid_add_device(hdev);

[Severity: Medium]
Are we missing the initialization of some standard hid_device fields here?

It appears that the version, country, and uniq fields are not populated
before calling hid_add_device(). This might break userspace expectations
that rely on these standard HID properties for device matching or
configuration.

> +	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 this synchronize_rcu() call necessary on the successful allocation path?

Since we are publishing a new pointer where the previous value was NULL,
there are no existing readers holding an old pointer that needs to be
reclaimed. This seems to delay device initialization by a full RCU grace
period unnecessarily.

[ ... ]

> @@ -1768,9 +1868,17 @@ 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_safe_input_report(hdev, HID_INPUT_REPORT, bytes,
> +			num_bytes, umin(header->length, num_bytes), true);

[Severity: High]
Could this cause large fragmented HID reports to be silently dropped?

When gip_receive_fragment() reassembles a fragmented message, it completes
upon receiving a terminator fragment where header->length is 0. It then
calls gip_handle_message() using this final header. 

If umin(header->length, num_bytes) is evaluated with header->length == 0,
the payload size passed to hid_safe_input_report() will be 0, causing the
entire reassembled HID report to be truncated.

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

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free risk in the
receive worker due to incorrect shutdown ordering in gip_disconnect()?

Looking at gip_disconnect():

	gip_shutdown(gip);
	...
	usb_kill_urb(gip->data.urb_in);

The gip_shutdown() function synchronously cancels the receive worker.
However, this happens before usb_kill_urb() is called. If an incoming URB
completes right after cancel_work_sync() returns, gip_urb_in() could
re-queue the worker.

When gip_disconnect() then proceeds to free the gip structure, the
re-queued worker will execute gip_receive_work() and access the freed
memory. Should the URBs be killed before cancelling the workers?

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