[PATCH] Input: wm831x-ts - drain pen-down work at teardown

[PATCH] Input: wm831x-ts - drain pen-down work at teardown

[Fan Wu]

The pen-down and data IRQ handlers queue pd_data_work to switch between the
two IRQs.  The worker obtains wm831x_ts with container_of() and calls
enable_irq().  free_irq() synchronizes an IRQ handler, but does not drain a
work item already queued by that handler.

wm831x_ts_remove() can therefore free the IRQ actions while pd_data_work is
pending or running.  The work may then dereference wm831x_ts after devres
frees it, or enable an IRQ after its action has been freed.

Disable both IRQs before cancelling the work, so neither handler can queue
another instance while cancel_work_sync() drains it.  Free the IRQ actions
only after the work has stopped.  Apply the same sequence to err_pd_irq:
both IRQ actions exist there when input_register_device() fails.

This issue was found by an in-house static analysis tool and confirmed by
manual code review.

Fixes: f5346668150c ("Input: wm831x-ts - fix races with IRQ management")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/input/touchscreen/wm831x-ts.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index 98f8ec408cad..705772025648 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -366,6 +366,10 @@ static int wm831x_ts_probe(struct platform_device *pdev)
 	return 0;
 
 err_pd_irq:
+	disable_irq(wm831x_ts->pd_irq);
+	disable_irq(wm831x_ts->data_irq);
+	cancel_work_sync(&wm831x_ts->pd_data_work);
+
 	free_irq(wm831x_ts->pd_irq, wm831x_ts);
 err_data_irq:
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
@@ -378,6 +382,10 @@ static void wm831x_ts_remove(struct platform_device *pdev)
 {
 	struct wm831x_ts *wm831x_ts = platform_get_drvdata(pdev);
 
+	disable_irq(wm831x_ts->pd_irq);
+	disable_irq(wm831x_ts->data_irq);
+	cancel_work_sync(&wm831x_ts->pd_data_work);
+
 	free_irq(wm831x_ts->pd_irq, wm831x_ts);
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
 }
-- 
2.34.1

Re: [PATCH] Input: wm831x-ts - drain pen-down work at teardown

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free / Lifecycle Bug due to devres and manual IRQ teardown mismatch.
--

--- Patch [1]: [PATCH] Input: wm831x-ts - drain pen-down work at teardown ---
commit 084d696661fe76ff6ec20cb061d22484763d2522
Author: Fan Wu <fanwu01@zju.edu.cn>

Input: wm831x-ts - drain pen-down work at teardown

This patch explicitly disables the pen-down and data IRQs and cancels the
pd_data_work before freeing the IRQs in the error path and during device
removal. This aims to fix races where the work might run or be pending
while the IRQs are being freed.

> diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
> index 98f8ec408cad6..7057720256487 100644
> --- a/drivers/input/touchscreen/wm831x-ts.c
> +++ b/drivers/input/touchscreen/wm831x-ts.c
[ ... ]
> @@ -378,6 +382,10 @@ static void wm831x_ts_remove(struct platform_device *pdev)
>  {
>  	struct wm831x_ts *wm831x_ts = platform_get_drvdata(pdev);
>  
> +	disable_irq(wm831x_ts->pd_irq);
> +	disable_irq(wm831x_ts->data_irq);
> +	cancel_work_sync(&wm831x_ts->pd_data_work);
> +
>  	free_irq(wm831x_ts->pd_irq, wm831x_ts);
>  	free_irq(wm831x_ts->data_irq, wm831x_ts);
>  }

[Severity: High]
This is a pre-existing issue, but does freeing the IRQs here lead to a
use-after-free when the input device is closed during devres cleanup?

Because the input device is managed via devres (allocated with
devm_input_allocate_device()), the input core will automatically unregister
it after wm831x_ts_remove() returns.

If the input device is held open by userspace while the driver is unbound,
the unregistration process will call the device's close callback. That callback
operates on the IRQs that have already been freed:

wm831x_ts_input_close() {
    ...
	if (wm831x_ts->pen_down) {
		disable_irq(wm831x_ts->data_irq);
		enable_irq(wm831x_ts->pd_irq);
		wm831x_ts->pen_down = false;
	}
    ...
}

Calling enable_irq() on an already freed IRQ can drop its depth to 0 and
unmask it without an action handler, leading to an IRQ shutdown and a loud
kernel 'irq X: nobody cared' warning. In a shared IRQ environment (like an MFD),
could this disrupt other devices sharing the same line?

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

[PATCH v2] Input: wm831x-ts - drain pen-down work at teardown

[Fan Wu]

The pen-down and data IRQ handlers queue pd_data_work to switch between the
two IRQs.  The worker obtains wm831x_ts with container_of() and calls
enable_irq().  free_irq() synchronizes an IRQ handler, but does not drain a
work item already queued by that handler.

wm831x_ts_remove() can therefore free the IRQ actions while pd_data_work is
pending or running.  The work may then dereference wm831x_ts after devres
frees it (use-after-free), or re-enable an IRQ whose action has been freed.

The input device is devm-allocated, so the input core unregisters it after
wm831x_ts_remove() returns.  If a userspace handle is still open, that
unregister drives the input close callback, which on pen-down calls
enable_irq(pd_irq) - after wm831x_ts_remove() has already freed the pd_irq
action.  Unregister the input device before freeing the IRQs, so the close
callback runs while the actions are still installed.  This must precede the
IRQ teardown: disabling and cancelling first would still let the close
callback re-enable pd_irq after its action is gone.

After unregistering, drain the work: disable both IRQs so neither handler
can queue another instance, cancel_work_sync() to make the drain final,
then free the actions.  Apply the same drain to err_pd_irq; the input
device is not registered there, so it is not unregistered.

This issue was found by an in-house static analysis tool and confirmed by
manual code review.

Fixes: f5346668150c ("Input: wm831x-ts - fix races with IRQ management")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
Changes since v1:
- Unregister the input device before freeing the IRQs. v1 drained the
  work in wm831x_ts_remove(), but the input device is devm-managed and is
  unregistered by the input core only after remove() returns; with a
  userspace handle open, that unregister drives the close callback, which
  on pen-down calls enable_irq(pd_irq) after its action has been freed.
 drivers/input/touchscreen/wm831x-ts.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index 98f8ec408cad..a465b1b96fd8 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -366,6 +366,10 @@ static int wm831x_ts_probe(struct platform_device *pdev)
 	return 0;
 
 err_pd_irq:
+	disable_irq(wm831x_ts->pd_irq);
+	disable_irq(wm831x_ts->data_irq);
+	cancel_work_sync(&wm831x_ts->pd_data_work);
+
 	free_irq(wm831x_ts->pd_irq, wm831x_ts);
 err_data_irq:
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
@@ -378,6 +382,12 @@ static void wm831x_ts_remove(struct platform_device *pdev)
 {
 	struct wm831x_ts *wm831x_ts = platform_get_drvdata(pdev);
 
+	input_unregister_device(wm831x_ts->input_dev);
+
+	disable_irq(wm831x_ts->pd_irq);
+	disable_irq(wm831x_ts->data_irq);
+	cancel_work_sync(&wm831x_ts->pd_data_work);
+
 	free_irq(wm831x_ts->pd_irq, wm831x_ts);
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
 }
-- 
2.34.1

Re: [PATCH v2] Input: wm831x-ts - drain pen-down work at teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Explicitly calling `input_unregister_device()` on a device allocated via `devm_input_allocate_device()` violates input subsystem guidelines.
--

--- Patch [1]: [PATCH v2] Input: wm831x-ts - drain pen-down work at teardown ---
commit f1080238ec479acff6be9e5861469736f7ad1c61
Author: Fan Wu <fanwu01@zju.edu.cn>

Input: wm831x-ts - drain pen-down work at teardown

This commit fixes a use-after-free race condition by draining the pending
pen-down work item during teardown. It explicitly unregisters the input
device before freeing the IRQs so that the close callback runs while actions
are still installed.

> diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
> --- a/drivers/input/touchscreen/wm831x-ts.c
> +++ b/drivers/input/touchscreen/wm831x-ts.c

[ ... ]

> @@ -378,6 +382,12 @@ static void wm831x_ts_remove(struct platform_device *pdev)
>  {
>  	struct wm831x_ts *wm831x_ts = platform_get_drvdata(pdev);
>  
> +	input_unregister_device(wm831x_ts->input_dev);

[Severity: Medium]
Is it safe to explicitly call input_unregister_device() here?

Since the input device is allocated using devm_input_allocate_device() during
wm831x_ts_probe(), the input subsystem already tracks it as a managed resource
and will automatically unregister it when the provider is unbound.

Explicitly calling input_unregister_device() on a devm-managed device mixes
managed and manual lifecycles, which can potentially lead to double-unregister
issues during teardown.

Could this race condition be addressed instead by fully migrating the driver to
managed resources? For example, using devm_request_threaded_irq() for the
IRQs and devm_add_action_or_reset() to handle the workqueue cancellation might
ensure the correct cleanup ordering without needing manual unregistration.

> +
> +	disable_irq(wm831x_ts->pd_irq);
> +	disable_irq(wm831x_ts->data_irq);
> +	cancel_work_sync(&wm831x_ts->pd_data_work);
> +
>  	free_irq(wm831x_ts->pd_irq, wm831x_ts);
>  	free_irq(wm831x_ts->data_irq, wm831x_ts);
>  }

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