Re: [PATCH v7 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] Use of hid_input_report instead of hid_safe_input_report can lead to out-of-bounds reads.
- [High] gip_hid_ll_raw_request returns 0 for an unimplemented feature, causing an infinite loop in userspace.
- [High] Calling strscpy() with a potentially NULL source pointer will cause a kernel panic.
- [High] The patch removes ratelimiting from a warning triggered by incoming USB packets, which can lead to kernel log spam.
- [Medium] Hardware HID reporting is enabled before the HID device is fully created and published, leading to dropped early reports.
- [Low] Unnecessary synchronize_rcu() after publishing a new RCU pointer.

Pre-existing issues:
- [Critical] Potential use-after-free of the gip structure during device disconnect because the receive_message workqueue can be rescheduled after it is cancelled.
--

commit a00262371994fa8e8d98c8d8041e5850d744198e
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 9371d860a59b8..411b67399e018 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,
> +static int gip_hid_ll_raw_request(struct hid_device *hdev,
> +	unsigned char reportnum, uint8_t *buf, size_t count,
> +	unsigned char report_type, int reqtype)
> +{
> +	/*
> +	 * TODO: Based on the metadata, output reports appear to be possible,
> +	 * but the chatpad doesn't have the LEDs it claims to support, so
> +	 * it's not clear how to test we're sending them properly.
> +	 */
> +	return 0;
> +}

[Severity: High]
Returning 0 indicates success with 0 bytes transferred. When a userspace
application calls write() on the hidraw device node, it will eventually call
this raw_request callback. Won't returning 0 here cause standard userspace
write loops to continuously retry indefinitely, resulting in a 100% CPU
infinite loop? Should this return a negative error code like -EOPNOTSUPP
instead?

[ ... ]
> @@ -1322,6 +1370,51 @@ static int gip_init_input_device(struct gip_attachment *attachment)
[ ... ]
> +	if (attachment->name)
> +		strscpy(hdev->name, attachment->name);
> +	else
> +		strscpy(hdev->name, "Xbox Chatpad");
> +	strscpy(hdev->phys, attachment->phys);
> +	strscpy(hdev->uniq, attachment->uniq);

[Severity: High]
If attachment->uniq remains NULL (for example, if the earlier dynamic
allocation via devm_kasprintf() failed under memory pressure), strscpy()
will be called with a NULL source pointer. Since strscpy() doesn't
gracefully handle NULL source pointers, won't this trigger a kernel panic
during device hotplug?

> +	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: Low]
Because there are no pre-existing readers that could be accessing the newly
published hdev pointer, is synchronize_rcu() needed here? Does this just add
an unnecessary latency penalty during device initialization?

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

[ ... ]
> @@ -1387,7 +1480,11 @@ static int gip_send_init_sequence(struct gip_attachment *attachment)
>  	if (rc)
>  		return rc;
>  
> -	return 0;
> +	rc = gip_create_hdev(attachment);
> +	if (rc)
> +		return rc;
> +
> +	return rc;
>  }

[Severity: Medium]
Earlier in gip_send_init_sequence(), the driver sends a GIP_STATE_START
command to the device, instructing it to begin reporting state. Because the
HID device is created and published here after that hardware command, won't
any early input reports arriving before gip_create_hdev() completes be
dropped and trigger spurious kernel warnings?

[ ... ]
> @@ -1783,9 +1886,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_ratelimited(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]
If a malicious or malfunctioning USB device crafts a report exceeding bounds
because the descriptor defines more fields than the actual payload size,
could using hid_input_report() here cause an out-of-bounds read in the HID
core? The HID subsystem guidelines require transport drivers to use
hid_safe_input_report() so the core can perform boundary checks against the
allocated buffer size.

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

[Severity: High]
By changing gip_warn_ratelimited() to a standard gip_warn(), a
malfunctioning or malicious unprivileged USB device could send continuous
HID report packets before a descriptor is configured. Could this flood the
kernel log and lead to a denial of service?

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but I noticed a potential
use-after-free of the gip structure during device disconnect in the base
driver.

Looking at gip_disconnect() in gip-core.c:

gip_disconnect()
   gip_shutdown(gip); // synchronously cancels gip->receive_message
   
   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); // kills IN URB here

Because the USB IN URB is not killed until gip_deinit_input(), if the IN URB
completes between gip_shutdown() and gip_deinit_input(), gip_urb_in() will
execute and reschedule gip->receive_message.

When gip_disconnect() subsequently frees the gip structure, won't the
rescheduled work execute on freed memory?

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