Re: [PATCH] HID: playstation: Fix DualShock4 dongle hotplug work use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hid: redundant synchronization due to false-positive race
--

commit 13619e48822a1be542eef83182d65e856c6ee40b
Author: Fan Wu <fanwu01@zju.edu.cn>

HID: playstation: Fix DualShock4 dongle hotplug work use-after-free
    
This commit attempts to fix a use-after-free issue by adding synchronization
state (dongle_hotplug_worker_initialized) and clearing it during error and
teardown paths. It claims this prevents the raw_event handler from scheduling
work concurrently during probe() and remove().

> HID: playstation: Fix DualShock4 dongle hotplug work use-after-free
>
> dualshock4_remove() cancels ds4->dongle_hotplug_worker while input
> reports can still arrive: hid_hw_close() and hid_hw_stop() only run
> later in ps_remove(), so a dongle connect report in that window makes
> dualshock4_dongle_parse_report() schedule the just-cancelled work
> again. ds4 is devm-managed and freed once ps_remove() returns, so
> dualshock4_dongle_calibration_work() then runs on freed memory.
>
> The dualshock4_create() error path has the same exposure. Reports are
> already flowing when it runs, because ps_probe() starts and opens the
> device before creating it, so the connect branch of the dongle report
> handler can also queue ds4->output_worker through its lightbar update.

Is this race actually possible?

Looking at the HID core, __hid_input_report() uses down_trylock() on
hdev->driver_input_lock and drops incoming reports when the lock is held.

During initialization, hid_device_probe() acquires this exact lock before
calling the driver's probe function (ps_probe()). The driver never calls
hid_device_io_start(), meaning this lock is held for the entire duration
of ps_probe(), including all of dualshock4_create().

Similarly, during teardown, hid_device_remove() acquires this lock before
calling the driver's remove function (ps_remove()), meaning it is held
for the entire duration of dualshock4_remove().

Since dualshock4_dongle_parse_report() is called from the raw_event
handler, which is fed by __hid_input_report(), it cannot execute concurrently
with either dualshock4_create() or dualshock4_remove().

> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index f9dc9378cf777..1b7a18fbeeb4b 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
[ ... ]
> @@ -2618,10 +2619,12 @@ static int dualshock4_dongle_parse_report(struct ps_device *ps_dev, struct hid_r
>  
>  			dualshock4_set_default_lightbar_colors(ds4);
>  
> -			scoped_guard(spinlock_irqsave, &ps_dev->lock)
> +			scoped_guard(spinlock_irqsave, &ps_dev->lock) {
>  				ds4->dongle_state = DONGLE_CALIBRATING;
>  
> -			schedule_work(&ds4->dongle_hotplug_worker);
> +				if (ds4->dongle_hotplug_worker_initialized)
> +					schedule_work(&ds4->dongle_hotplug_worker);
> +			}

[Severity: Medium]
Does this flag check actually do anything in dualshock4_dongle_parse_report(),
given that this function cannot run concurrently with the teardown sequence?

>  			/* Don't process the report since we don't have
>  			 * calibration data, but let hidraw have it anyway.
[ ... ]
> @@ -2677,8 +2680,12 @@ static void dualshock4_remove(struct ps_device *ps_dev)
>  
>  	cancel_work_sync(&ds4->output_worker);
>  
> -	if (ps_dev->hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE)
> +	if (ps_dev->hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE) {
> +		scoped_guard(spinlock_irqsave, &ds4->base.lock)
> +			ds4->dongle_hotplug_worker_initialized = false;
> +
>  		cancel_work_sync(&ds4->dongle_hotplug_worker);
> +	}
>  }

[Severity: Medium]
Does clearing this flag in dualshock4_remove() prevent a race, or is the
incoming report path already physically blocked by hdev->driver_input_lock?

[ ... ]
> @@ -2858,6 +2868,18 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
>  
>  err:
>  	ps_devices_list_remove(ps_dev);
> +err_cancel:
> +	scoped_guard(spinlock_irqsave, &ps_dev->lock)
> +		ds4->output_worker_initialized = false;
> +
> +	cancel_work_sync(&ds4->output_worker);
> +
> +	if (ds4->dongle_hotplug_worker_initialized) {
> +		scoped_guard(spinlock_irqsave, &ps_dev->lock)
> +			ds4->dongle_hotplug_worker_initialized = false;
> +
> +		cancel_work_sync(&ds4->dongle_hotplug_worker);
> +	}
>  	return ERR_PTR(ret);
>  }

[Severity: Medium]
Since hdev->driver_input_lock is held throughout dualshock4_create(), are
these flag checks and locks on the error path protecting against a race that
is impossible to hit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923032634.417433-1-fanwu01@zju.edu.cn?part=1