Re: [PATCH 1/2] HID: flydigi: add support for vader 5 pro

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Unconditional scheduling of `rumble_work` in `flydigi_gamepad_close()` leads to a Use-After-Free during `devm` teardown.
- [High] Early HID reports cause a NULL pointer dereference because `fd->gamepad` is accessed before it is initialized.
- [High] The HID `raw_event` handler attempts to acquire a mutex and perform synchronous USB control transfers from an interrupt context, triggering a system panic.
- [High] The `output_buf` array is embedded in `struct flydigi_device` without cache-line alignment, violating DMA API requirements.
- [High] Incoming HID raw events can reschedule `acquire_work` after it has been explicitly canceled in `flydigi_remove()`, leading to a Use-After-Free.
- [Low] Canceling `acquire_work` before setting `gamepad_open` to false leaks an armed timer.
- [Low] Negating a signed 16-bit sensor axis value can produce an integer that exceeds the configured `ABS` maximum.
--

--- Patch [1]: [PATCH 1/2] HID: flydigi: add support for vader 5 pro ---
commit 01b1533a1ac56f03f32a4b26cb0c7795d18213a6
Author: Denis Benato <[email protected]>

HID: flydigi: add support for vader 5 pro

The Flydigi Vader Pro 5 is a gamepad with additional buttons
and a gyroscope that is connected wireless to a PC via its
own dongle so it's always a USB device: add support for it
by following the enstablished pattern of spawning two evdevs
that share the same uniqid so that SDL can match the two and
expose them as a single controller.

> diff --git a/drivers/hid/hid-flydigi.c b/drivers/hid/hid-flydigi.c
> new file mode 100644
> index 0000000000000..48129f429f3c2
> --- /dev/null
> +++ b/drivers/hid/hid-flydigi.c

[ ... ]

> +struct flydigi_device {
> +	struct hid_device *hdev;
> +	struct input_dev *gamepad;
> +	struct input_dev *sensors;
> +
> +	struct mutex output_mutex;
> +	spinlock_t lock;
> +	u8 output_buf[FLYDIGI_REPORT_SIZE];
> +	u8 output_report_id;

[Severity: High]
Is this embedded DMA buffer cache-line aligned?

On architectures that are not hardware cache-coherent, passing an unaligned
embedded buffer to the USB subsystem for DMA can cause memory corruption on
adjacent fields (like output_report_id or lock).

[ ... ]

> +static void flydigi_gamepad_close(struct input_dev *dev)
> +{
> +	struct flydigi_device *fd = input_get_drvdata(dev);
> +
> +	cancel_delayed_work_sync(&fd->acquire_work);

[Severity: Low]
Is it possible for the worker to re-arm itself here?

If the worker is running when cancel_delayed_work_sync() is called, it waits
for the worker to finish. However, because gamepad_open is still true at this
point, the worker could evaluate the flag as true and call
schedule_delayed_work(), leaking an armed timer after the device is closed.

> +
> +	{
> +		guard(spinlock_irqsave)(&fd->lock);
> +
> +		fd->gamepad_open = false;
> +		fd->strong = 0;
> +		fd->weak = 0;
> +	}
> +
> +	schedule_work(&fd->rumble_work);
> +}

[Severity: High]
Does this unconditionally schedule the work item during teardown?

Because the input device is allocated with devm_input_allocate_device(), the
device is automatically unregistered by devres after flydigi_remove() returns.
The input core will call this close() callback during that unregistration if
the device was open.

Since this schedules the work right as devres is freeing the fd structure,
can this lead to a Use-After-Free when the worker thread executes?

[ ... ]

> +static void flydigi_report_gamepad(struct flydigi_device *fd, const u8 *data)
> +{
> +	struct input_dev *gamepad = fd->gamepad;
> +	s16 axis;
> +	int hat_x;
> +	int hat_y;
> +
> +	flydigi_map_hat(data[FLYDIGI_OFF_DPAD_ABXY], &hat_x, &hat_y);
> +	input_report_abs(gamepad, ABS_HAT0X, hat_x);

[Severity: High]
Can gamepad be NULL here?

In flydigi_probe(), hid_hw_open() is called before flydigi_register_gamepad().
Calling hid_hw_open() starts the hardware polling, allowing an early
FLYDIGI_CMD_INPUT event to trigger flydigi_raw_event() and subsequently
flydigi_report_gamepad().

Since fd->gamepad is assigned only at the end of flydigi_register_gamepad(),
will this early hardware polling cause a NULL pointer dereference here?

[ ... ]

> +static void flydigi_report_sensors(struct flydigi_device *fd, const u8 *data)
> +{
> +	struct input_dev *sensors = fd->sensors;
> +
> +	if (!sensors)
> +		return;
> +
> +	input_event(sensors, EV_MSC, MSC_TIMESTAMP,
> +		    ktime_to_us(ktime_get_boottime()));
> +
> +	/* Accelerometer */
> +	input_report_abs(sensors, ABS_X,
> +			 (s16)get_unaligned_le16(&data[FLYDIGI_OFF_ACCEL_X]));
> +	input_report_abs(sensors, ABS_Y,
> +			 (s16)get_unaligned_le16(&data[FLYDIGI_OFF_ACCEL_Y]));
> +	input_report_abs(sensors, ABS_Z,
> +			 -(s16)get_unaligned_le16(&data[FLYDIGI_OFF_ACCEL_Z]));

[Severity: Low]
Can this negation produce an out-of-bounds value?

If the raw 16-bit sensor value is exactly -32768, casting it to s16 and
negating it will yield 32768 due to C integer promotion rules, which exceeds
the declared ABS maximum of 32767.

[ ... ]

> +static int flydigi_raw_event(struct hid_device *hdev, struct hid_report *report,
> +			     u8 *data, int size)
> +{
> +	struct flydigi_device *fd = hid_get_drvdata(hdev);
> +	const u8 *payload = data;

[ ... ]

> +	switch (payload[2]) {
> +	case FLYDIGI_CMD_INPUT:
> +		flydigi_report_gamepad(fd, payload);
> +		flydigi_report_sensors(fd, payload);
> +		break;
> +	case FLYDIGI_CMD_INFO:
> +	case FLYDIGI_CMD_STATUS:
> +	case FLYDIGI_CMD_ACQUIRE:
> +	case FLYDIGI_CMD_HAPTIC:
> +		/* Expected replies/acks to commands sent by this driver. */
> +		break;
> +	case FLYDIGI_CMD_STATUS_UPDATE:
> +		flydigi_send_status_request(fd);

[Severity: High]
Can this execute a sleeping USB control transfer in an atomic context?

This function is called from an URB completion handler, but
flydigi_send_status_request() eventually calls flydigi_send_output() which
acquires a blocking mutex and calls hid_hw_raw_request(). Does this cause
a scheduling while atomic panic?

> +		schedule_delayed_work(&fd->acquire_work, 0);

[Severity: High]
Does this re-arm acquire_work during device removal?

In flydigi_remove(), cancel_delayed_work_sync(&fd->acquire_work) is called
before hid_hw_stop(). If the hardware sends a FLYDIGI_CMD_STATUS_UPDATE
packet in this small window, this will unconditionally re-schedule the work.

If the gamepad was not open, can this leave the work queued after fd is
freed, resulting in a Use-After-Free?

> +		break;
> +	default:
> +		hid_dbg(hdev, "unhandled command: cmd=%02x sz=%d\n",
> +			payload[2], len);
> +		break;
> +	}
> +
> +	return 0;
> +}

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