drivers/input/mouse/alps.c | 1 + drivers/input/mouse/psmouse-base.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
The flush_workqueue() in psmouse_disconnect() only blocks and waits for
work items that were already queued to the workqueue prior to its
invocation. Any work items submitted after flush_workqueue() is called
are not included in the set of tasks that the flush operation awaits.
This means that after flush_workqueue() has finished executing, the
resync_work and dev3_register_work could be rescheduled again, resulting
in the following two use-after-free scenarios:
1. The psmouse structure is deallocated in psmouse_disconnect(), while
resync_work remains active and attempts to dereference the already
freed psmouse in psmouse_resync().
CPU 0 | CPU 1
psmouse_disconnect() | psmouse_receive_byte()
| if(psmouse->state == ...)
psmouse_set_state() |
flush_workqueue() |
| psmouse_queue_work() //reschedule
kfree(psmouse); //FREE|
| psmouse_resync()
| psmouse = container_of(); //USE
| psmouse-> //USE
2. The alps_data structure is deallocated in alps_disconnect(), while
dev3_register_work remains active and attempts to dereference the
already freed alps_data inside alps_register_bare_ps2_mouse().
CPU 0 | CPU 1
psmouse_disconnect() | alps_process_byte()
flush_workqueue() | alps_report_bare_ps2_packet()
| psmouse_queue_work() //reschedule
alps_disconnect() |
| alps_register_bare_ps2_mouse()
kfree(priv); //FREE |
| priv = container_of(); //USE
| priv-> //USE
Replace flush_workqueue() with disable_delayed_work_sync(), and also
add disable_delayed_work_sync() in alps_disconnect(). This ensures
that both resync_work and dev3_register_work are properly canceled
and prevented from being rescheduled before the psmouse and alps_data
structures are deallocated.
These bugs are identified by static analysis.
Fixes: f0d5c6f419d3 ("Input: psmouse - attempt to re-synchronize mouse every 5 seconds")
Fixes: 04aae283ba6a ("Input: ALPS - do not mix trackstick and external PS/2 mouse data")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
drivers/input/mouse/alps.c | 1 +
drivers/input/mouse/psmouse-base.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index d0cb9fb9482..df8953a5196 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2975,6 +2975,7 @@ static void alps_disconnect(struct psmouse *psmouse)
psmouse_reset(psmouse);
timer_shutdown_sync(&priv->timer);
+ disable_delayed_work_sync(&priv->dev3_register_work);
if (priv->dev2)
input_unregister_device(priv->dev2);
if (!IS_ERR_OR_NULL(priv->dev3))
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index 77ea7da3b1c..eb41c553e80 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -1484,7 +1484,7 @@ static void psmouse_disconnect(struct serio *serio)
/* make sure we don't have a resync in progress */
mutex_unlock(&psmouse_mutex);
- flush_workqueue(kpsmoused_wq);
+ disable_delayed_work_sync(&psmouse->resync_work);
mutex_lock(&psmouse_mutex);
if (serio->parent && serio->id.type == SERIO_PS_PSTHRU) {
--
2.34.1Hi Duoming, On Sat, Nov 08, 2025 at 12:56:09PM +0800, Duoming Zhou wrote: > The flush_workqueue() in psmouse_disconnect() only blocks and waits for > work items that were already queued to the workqueue prior to its > invocation. Any work items submitted after flush_workqueue() is called > are not included in the set of tasks that the flush operation awaits. > This means that after flush_workqueue() has finished executing, the > resync_work and dev3_register_work could be rescheduled again, resulting > in the following two use-after-free scenarios: > > 1. The psmouse structure is deallocated in psmouse_disconnect(), while > resync_work remains active and attempts to dereference the already > freed psmouse in psmouse_resync(). > > CPU 0 | CPU 1 > psmouse_disconnect() | psmouse_receive_byte() > | if(psmouse->state == ...) > psmouse_set_state() | > flush_workqueue() | > | psmouse_queue_work() //reschedule > kfree(psmouse); //FREE| > | psmouse_resync() > | psmouse = container_of(); //USE > | psmouse-> //USE Before flushing the workqueue we set psmouse state to PSMOUSE_CMD_MODE, but psmouse_queue_work() is only called from psmouse_receive_byte() if the mouse is PSMOUSE_ACTIVE. Therefore there is no chance that work will be scheduled while psmouse instance is being freed. For ALPS, the work is a "single shot", so will not get rescheduled. However I think that the changes are improvement to the code. Please split in 2 (for psmouse-base and alps separately) and drop mentions of UAF in psmouse but rather mention that disable_delayed_work_sync() is more robust and efficient. I think if we switch to it we should be able to get rid of kpsmoused workqueue and use default system workqueue. Thanks. -- Dmitry
On Fri, 7 Nov 2025 21:43:37 -0800, Dmitry Torokhov wrote:
> > The flush_workqueue() in psmouse_disconnect() only blocks and waits for
> > work items that were already queued to the workqueue prior to its
> > invocation. Any work items submitted after flush_workqueue() is called
> > are not included in the set of tasks that the flush operation awaits.
> > This means that after flush_workqueue() has finished executing, the
> > resync_work and dev3_register_work could be rescheduled again, resulting
> > in the following two use-after-free scenarios:
> >
> > 1. The psmouse structure is deallocated in psmouse_disconnect(), while
> > resync_work remains active and attempts to dereference the already
> > freed psmouse in psmouse_resync().
> >
> > CPU 0 | CPU 1
> > psmouse_disconnect() | psmouse_receive_byte()
> > | if(psmouse->state == ...)
> > psmouse_set_state() |
> > flush_workqueue() |
> > | psmouse_queue_work() //reschedule
> > kfree(psmouse); //FREE|
> > | psmouse_resync()
> > | psmouse = container_of(); //USE
> > | psmouse-> //USE
>
> Before flushing the workqueue we set psmouse state to PSMOUSE_CMD_MODE,
> but psmouse_queue_work() is only called from psmouse_receive_byte() if
> the mouse is PSMOUSE_ACTIVE. Therefore there is no chance that work will
> be scheduled while psmouse instance is being freed.
I believe a potential race condition still exists between
psmouse_receive_byte() and psmouse_disconnect(). Specifically,
if the condition check 'if (psmouse->state == PSMOUSE_ACTIVATED)'
has already been passed, and then we set the psmouse state to
PSMOUSE_CMD_MODE and flush the workqueue, it is still possible
for psmouse_queue_work() to be scheduled after flush_workqueue().
Here is the problematic sequence:
CPU 0 | CPU 1
psmouse_disconnect() | psmouse_receive_byte()
| if(psmouse->state == PSMOUSE_ACTIVATED && ...)
psmouse_set_state() |
flush_workqueue() |
| psmouse_queue_work()
kfree(psmouse); //FREE|
| psmouse_resync()
| psmouse = container_of(); //USE
| psmouse-> //USE
> For ALPS, the work is a "single shot", so will not get rescheduled.
I agree with you, the 'rescheduled' is not accurate. But the race condition
is still possible. The dev3_register_work is initialized through alps_reconnect()
and scheduled when receiving the first bare PS/2 packet from an external
device connected to the ALPS touchpad's PS/2 port. When the device
is disconnecting, the original code does not cancel the dev3_register_work.
This leads to a use-after-free scenario where the alps_data is
freed in alps_disconnect(), but the dev3_register_work remains active
and may attempt to access the freed alps_data in alps_register_bare_ps2_mouse().
A typical race condition is illustrated below:
CPU 0 (cleanup) | CPU 1 (delayed work)
alps_disconnect() |
kfree(priv); |
| alps_register_bare_ps2_mouse()
| priv = container_of(...); //USE
| // access priv->... (USE)
Fix this by ensuring that the delayed work is canceled in alps_disconnect()
using disable_delayed_work_sync().
> However I think that the changes are improvement to the code. Please
> split in 2 (for psmouse-base and alps separately) and drop mentions of
> UAF in psmouse but rather mention that disable_delayed_work_sync() is
> more robust and efficient. I think if we switch to it we should be able
> to get rid of kpsmoused workqueue and use default system workqueue.
Thank you for your suggestions, I will split this into two patches:
one to fix the issue in psmouse-base, and another for the ALPS.
Best regards,
Duoming Zhou
On Sat, Nov 08, 2025 at 02:22:10PM +0800, duoming@zju.edu.cn wrote: > On Fri, 7 Nov 2025 21:43:37 -0800, Dmitry Torokhov wrote: > > > The flush_workqueue() in psmouse_disconnect() only blocks and waits for > > > work items that were already queued to the workqueue prior to its > > > invocation. Any work items submitted after flush_workqueue() is called > > > are not included in the set of tasks that the flush operation awaits. > > > This means that after flush_workqueue() has finished executing, the > > > resync_work and dev3_register_work could be rescheduled again, resulting > > > in the following two use-after-free scenarios: > > > > > > 1. The psmouse structure is deallocated in psmouse_disconnect(), while > > > resync_work remains active and attempts to dereference the already > > > freed psmouse in psmouse_resync(). > > > > > > CPU 0 | CPU 1 > > > psmouse_disconnect() | psmouse_receive_byte() > > > | if(psmouse->state == ...) > > > psmouse_set_state() | > > > flush_workqueue() | > > > | psmouse_queue_work() //reschedule > > > kfree(psmouse); //FREE| > > > | psmouse_resync() > > > | psmouse = container_of(); //USE > > > | psmouse-> //USE > > > > Before flushing the workqueue we set psmouse state to PSMOUSE_CMD_MODE, > > but psmouse_queue_work() is only called from psmouse_receive_byte() if > > the mouse is PSMOUSE_ACTIVE. Therefore there is no chance that work will > > be scheduled while psmouse instance is being freed. > > I believe a potential race condition still exists between > psmouse_receive_byte() and psmouse_disconnect(). Specifically, > if the condition check 'if (psmouse->state == PSMOUSE_ACTIVATED)' > has already been passed, and then we set the psmouse state to > PSMOUSE_CMD_MODE and flush the workqueue, it is still possible > for psmouse_queue_work() to be scheduled after flush_workqueue(). Setting psmouse state can not race with psmouse_receive_byte() because we take the serio->lock and disable interrupts. Thanks. -- Dmitry
© 2016 - 2025 Red Hat, Inc.