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

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

New issues:
- [High] The tunneled `hid_device` (`attachment->hdev`) is permanently leaked when the controller's metadata is reset, which can lead to a future Use-After-Free.
- [High] The HID device is configured with `BUS_USB` but its parent is set to a `usb_device` rather than a `usb_interface`, causing downstream HID drivers to crash.
- [Medium] The transport driver uses `hid_input_report()` instead of the required `hid_safe_input_report()`, circumventing HID core bounds checking.
- [Low] Unnecessary `synchronize_rcu()` after `rcu_assign_pointer()` when publishing `hdev`.
- [Low] Not all required `hid_device` fields are initialized before `hid_add_device()`.

Pre-existing issues:
- [High] The error path in `gip_setup_input_device()` leaves a dangling pointer in `attachment->input`, which causes a Use-After-Free during shutdown.
- [High] Missing NULL check after allocating memory with `devm_kmalloc` in `gip_send_raw_message()`, leading to a NULL pointer dereference.
--

--- Patch [4]: [PATCH v4 04/12] Input: xbox_gip - Add HID relaying ---
commit a021e64e3205f2caaac1426796abb2e1c1a1a8e6
Author: Vicki Pfau <[email protected]>

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 15f8ed2107110..c833b10259ab8 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

> @@ -1361,7 +1409,34 @@ static int gip_send_init_sequence(struct gip_attachment *attachment)
>  	if (rc)
>  		return rc;
>  
> -	return 0;
> +	if (attachment->metadata.device.hid_descriptor) {
> +		struct hid_device *hdev = hid_allocate_device();
> +
> +		if (IS_ERR(hdev))
> +			return PTR_ERR(hdev);

[Severity: High]
Does this leak the previous hdev if called multiple times, or does
gip_reset_metadata() need to be updated to clean up this new device?

When the metadata is reset, gip_reset_metadata() tears down the input
device:

	if (input) {
		rcu_assign_pointer(attachment->input, NULL);
		synchronize_rcu();
		input_unregister_device(input);
	}

But the newly added attachment->hdev is not destroyed there, which could
leave a stray hid_device and lead to a use-after-free via hidraw.

> +
> +		hdev->ll_driver = &gip_hid_ll_driver;
> +		hdev->bus = BUS_USB;
> +		hdev->vendor = attachment->vendor_id;
> +		hdev->product = attachment->product_id;

[Severity: Low]
Are there missing hid_device initializations here?

The HID subsystem guidelines recommend fully initializing the device fields
(such as uniq, version, country, etc.) before calling hid_add_device().

> +		hdev->dev.parent = to_gip_device(attachment);

[Severity: High]
Could this parent assignment cause a crash in downstream HID drivers?

The device is configured as a USB bus device, but the parent is being set
to a gip_device (which contains a usb_device) rather than a usb_interface.
Many USB HID drivers assume the parent is a usb_interface and call
to_usb_interface() on it. If they do, they will perform an invalid cast and
crash.

> +		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);
> +		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]
Is this synchronize_rcu() call necessary?

Since rcu_assign_pointer() already provides the needed memory barriers to
publish a new pointer, and we aren't waiting for readers of an old pointer
to finish (as there isn't one being freed here), calling synchronize_rcu()
might just unnecessarily block and degrade performance.

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

[ ... ]

> @@ -1769,9 +1844,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: Medium]
Does this bypass the HID core's bounds checking?

Using hid_input_report() instead of hid_safe_input_report() for incoming
data violates the HID subsystem transport guidelines, as it prevents the
core from validating the size of the buffer.

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

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the input
device lifecycle, I noticed a potential use-after-free during shutdown.

In gip_setup_input_device(), if input_register_device() fails, the pointer
is left dangling:

	err_free_device:
		input_free_device(input);
		return rc;

When gip_shutdown() later runs, it sees the remaining attachment->input
pointer and calls input_unregister_device() on an already freed device.

[Severity: High]
This is a pre-existing issue, but does this allocation in
gip_send_raw_message() need a null check?

	fragment->data = devm_kmalloc(to_gip_device(attachment),
		fragment->total_length, GFP_ATOMIC);
	fragment->fragment_offset = 0;
	memcpy(fragment->data, bytes, num_bytes);

If the atomic allocation fails and returns a null pointer, the immediate
memcpy() will cause a kernel panic.

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