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

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

[Fan Wu]

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.
Neither work is cancelled when creation fails, and ds4 is freed once
the failed probe unwinds its devm allocations.

Fix this by clearing a new dongle_hotplug_worker_initialized flag under
ps_dev->lock before cancelling the worker and checking it under the same
lock before scheduling, mirroring how this driver already guards
ds4->output_worker. Also drain both workers on every error path of
dualshock4_create(): cancel dongle_hotplug_worker on the early MAC-read
and device-list failures that currently return directly, and clear
output_worker_initialized and cancel ds4->output_worker as
dualshock4_remove() already does. The output-report buffer allocation
failure returns through the same cleanup.

The locked check closes the schedule-vs-clear race, and neither work
re-schedules itself, so the cancels leave nothing pending.

This issue was found by an in-house static analysis tool.

Fixes: c64ed0cd9324 ("HID: playstation: add DualShock4 dongle support.")
Cc: stable@vger.kernel.org
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/hid/hid-playstation.c | 36 ++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index f9dc937..1b7a18f 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -421,6 +421,7 @@ struct dualshock4 {
 	enum dualshock4_dongle_state dongle_state;
 	/* Used during calibration. */
 	struct work_struct dongle_hotplug_worker;
+	bool dongle_hotplug_worker_initialized;
 
 	/* Timestamp for sensor data */
 	bool sensor_timestamp_initialized;
@@ -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);
+			}
 
 			/* 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);
+	}
 }
 
 static inline void dualshock4_schedule_work(struct dualshock4 *ds4)
@@ -2770,12 +2777,15 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
 
 	max_output_report_size = sizeof(struct dualshock4_output_report_bt);
 	ds4->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
-	if (!ds4->output_report_dmabuf)
-		return ERR_PTR(-ENOMEM);
+	if (!ds4->output_report_dmabuf) {
+		ret = -ENOMEM;
+		goto err_cancel;
+	}
 
 	if (hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE) {
 		ds4->dongle_state = DONGLE_DISCONNECTED;
 		INIT_WORK(&ds4->dongle_hotplug_worker, dualshock4_dongle_calibration_work);
+		ds4->dongle_hotplug_worker_initialized = true;
 
 		/* Override parse report for dongle specific hotplug handling. */
 		ps_dev->parse_report = dualshock4_dongle_parse_report;
@@ -2784,7 +2794,7 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
 	ret = dualshock4_get_mac_address(ds4);
 	if (ret) {
 		hid_err(hdev, "Failed to get MAC address from DualShock4\n");
-		return ERR_PTR(ret);
+		goto err_cancel;
 	}
 	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds4->base.mac_address);
 
@@ -2796,7 +2806,7 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
 
 	ret = ps_devices_list_add(ps_dev);
 	if (ret)
-		return ERR_PTR(ret);
+		goto err_cancel;
 
 	ret = dualshock4_get_calibration_data(ds4);
 	if (ret) {
@@ -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);
 }
 

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

[sashiko-bot]

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

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

[Fan Wu]

Please drop this patch.

__hid_input_report() returns -EBUSY when it cannot take
hdev->driver_input_lock, and hid_device_probe() and hid_device_remove()
hold that lock across the driver's ->probe() and ->remove() callbacks.
hid-playstation never calls hid_device_io_start(), so input reports
cannot reach dualshock4_dongle_parse_report() while
dualshock4_create() or dualshock4_remove() run. The window this patch
tried to guard does not exist, and the cancel_work_sync() calls that
were already there are sufficient.

The issue came from our static analysis tooling, which missed that
the HID core serializes the report path against probe and remove.

Sorry for the noise.

Fan Wu