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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probe error path fails to set `fd->removed = true`, leading to UAF and destroyed mutex usage via devres unregistration and input callbacks.
--

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

HID: flydigi: add support for vader 5 pro

This commit adds support for the Flydigi Vader 5 Pro gamepad. It introduces
a driver that creates two evdev nodes (gamepad and sensors) sharing the
same uniqid.

> diff --git a/drivers/hid/hid-flydigi.c b/drivers/hid/hid-flydigi.c
> new file mode 100644
> index 0000000000000..ee95ade7d127b
> --- /dev/null
> +++ b/drivers/hid/hid-flydigi.c
[ ... ]
> +static void flydigi_gamepad_close(struct input_dev *dev)
> +{
> +	struct flydigi_device *fd = input_get_drvdata(dev);
> +	bool removed;
> +
> +	scoped_guard(spinlock_irqsave, &fd->lock) {
> +		fd->gamepad_open = false;
> +		fd->strong = 0;
> +		fd->weak = 0;
> +	}
> +
> +	cancel_delayed_work_sync(&fd->acquire_work);
> +
> +	scoped_guard(spinlock_irqsave, &fd->lock)
> +		removed = fd->removed;
> +
> +	if (!removed)
> +		schedule_work(&fd->rumble_work);
> +}
[ ... ]
> +static int flydigi_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
[ ... ]
> +	ret = flydigi_register_gamepad(fd);
> +	if (ret)
> +		goto err_stop;
> +
> +	ret = flydigi_register_sensors(fd);
> +	if (ret)
> +		goto err_stop;
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hw open failed: %d\n", ret);
> +		goto err_stop;
> +	}
[ ... ]
> +err_stop:
> +	hid_hw_stop(hdev);
> +	cancel_delayed_work_sync(&fd->acquire_work);
> +	cancel_work_sync(&fd->rumble_work);
> +	mutex_destroy(&fd->output_mutex);
> +	return ret;

[Severity: High]
Can a probe failure after gamepad registration cause a use-after-free?

If flydigi_probe() fails after flydigi_register_gamepad() completes (for
example, if hid_hw_open() fails), the err_stop path is taken. However,
fd->removed is not set to true.

When probe returns an error, devres will unregister the input device. If
userspace had already opened the device, the unregistration will trigger
flydigi_gamepad_close(). Because fd->removed is still false, it will
schedule fd->rumble_work:

	if (!removed)
		schedule_work(&fd->rumble_work);

After devres frees the fd structure, the workqueue could execute
flydigi_rumble_worker(), resulting in a use-after-free and an attempt to
lock the destroyed output_mutex.

Should fd->removed be set to true in the err_stop path, similar to what is
done in flydigi_remove()?

> +}

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