Re: [PATCH v4 09/12] Input: xbox_gip - Add support for PDP guitar controllers

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

New issues:
- [High] Dynamic protocol detection relies on payload contents, causing face buttons and the thumbstick click on the RiffMaster to trigger ghost fret inputs when no frets are held.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a dangling RCU pointer and synchronous free of `input_dev` on registration failure.
--

--- Patch [9]: [PATCH v4 09/12] Input: xbox_gip - Add support for PDP guitar controllers ---
commit 6b8e97ecc5fa40007c8c5bf1a4aa892abddb06cf
Author: Vicki Pfau <[email protected]>

Input: xbox_gip - Add support for PDP guitar controllers

This patch adds support for the PDP guitar controllers, namely the Fender
Jaguar and RiffMaster models. These use mostly the same protocol, but the
RiffMaster appears t use a slightly newer variant.

> diff --git a/drivers/input/joystick/gip/gip-pdp-jaguar.c b/drivers/input/joystick/gip/gip-pdp-jaguar.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-pdp-jaguar.c

[ ... ]

> +static int gip_handle_pdp_jaguar_report(struct gip_attachment *attachment,
> +	struct input_dev *input, const uint8_t *bytes, int num_bytes)
> +{
> +	bool lower;
> +
> +	if (num_bytes < 4) {
> +		gip_dbg(attachment, "Discarding too-short input report\n");
> +		return -EINVAL;
> +	}
> +
> +	input_report_key(input, BTN_START, bytes[0] & BIT(2));
> +	input_report_key(input, BTN_SELECT, bytes[0] & BIT(3));
> +
> +	if (num_bytes >= 7 && (bytes[5] || bytes[6])) {

[Severity: High]
Does this condition cause ghost fret inputs on the RiffMaster when
no frets are held?

If face buttons or the thumbstick click are pressed on the RiffMaster
without pressing any frets, bytes[5] and bytes[6] would be zero. This
causes the check to fall through to the legacy report logic, which seems
like it would misinterpret the standard face button bits as legacy fret
bits.

> +		/* Newer report version on the RiffMaster */
> +		input_report_key(input, BTN_TRIGGER_HAPPY1, bytes[5] & BIT(0));

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a dangling RCU pointer
and synchronous free if input_register_device() fails?

Looking at gip_setup_input_device() in drivers/input/joystick/gip/gip-core.c:

	rcu_assign_pointer(attachment->input, input);
	rc = input_register_device(input);
	if (rc)
		goto err_free_device;
...
err_free_device:
	input_free_device(input);

The input device pointer is published to RCU readers and subsequently freed
synchronously on the error path without clearing the pointer or waiting for
an RCU grace period. Can concurrent incoming USB packets process and
dereference the dangling attachment->input pointer?

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