Re: [PATCH] Input: byd - synchronize timer deletion before freeing private data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free or NULL pointer dereference in byd_process_byte() due to missing RX synchronization during disconnect.
- [High] Improper timer teardown API allows the timer to be re-armed after sync, causing a delayed Use-After-Free.
--

--- Patch [1]: [PATCH] Input: byd - synchronize timer deletion before freeing private data ---
commit eb1fe8255d54e5e900b504d5aed98e3fff1721fb
Author: Linmao Li <lilinmao@kylinos.cn>

Input: byd - synchronize timer deletion before freeing private data
    
byd_disconnect() uses timer_delete() before freeing the driver's private
data.  This does not wait for a running byd_clear_touch() callback, which
dereferences the private data and its psmouse pointer.  A callback racing
with disconnect can therefore access the private data after it has been
freed.
    
Use timer_delete_sync() to wait for any running callback before releasing
the private data.
    
Fixes: 2d5f5611dd0d ("Input: byd - enable absolute mode")
Cc: stable@vger.kernel.org
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>

> diff --git a/drivers/input/mouse/byd.c b/drivers/input/mouse/byd.c
> index f5770a3af2f12..7a69724c1ac6b 100644
> --- a/drivers/input/mouse/byd.c
> +++ b/drivers/input/mouse/byd.c
> @@ -423,7 +423,7 @@ static void byd_disconnect(struct psmouse *psmouse)
>  	struct byd_data *priv = psmouse->private;
>  
>  	if (priv) {
> -		timer_delete(&priv->timer);
> +		timer_delete_sync(&priv->timer);

[Severity: High]
This isn't a bug introduced by this patch, but does timer_delete_sync()
prevent the timer from being re-armed concurrently?

Because device interrupts are not disabled during byd_disconnect(),
byd_process_byte() can execute immediately after timer_delete_sync()
returns but before psmouse->private is freed.

If byd_process_byte() processes a touch event, it can call mod_timer()
and successfully re-arm the timer on the priv structure:

drivers/input/mouse/byd.c:byd_process_byte() {
	...
	mod_timer(&priv->timer, jiffies + BYD_TOUCH_TIMEOUT);
	...
}

When byd_disconnect() subsequently frees priv, won't the timer callback
execute on the freed memory?

Should this use timer_shutdown_sync() instead to permanently prevent the timer
from being re-armed during teardown?

>  		kfree(psmouse->private);
>  		psmouse->private = NULL;

[Severity: High]
This is a pre-existing issue, but is it possible for an interrupt to trigger
byd_process_byte() concurrently with or immediately after freeing this memory?

The core psmouse_disconnect() function invokes the driver's disconnect callback
without first stopping device interrupts (such as by using serio_pause_rx()).

If the hardware sends a byte during device teardown, ps2_interrupt() can
route the data to the active protocol handler byd_process_byte(). This
unconditionally dereferences the freed or NULL psmouse->private pointer:

drivers/input/mouse/byd.c:byd_process_byte() {
	struct byd_data *priv = psmouse->private;
	...
}

Does the receive path need to be paused or synchronized before freeing the
private data to prevent a use-after-free or NULL pointer dereference?

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720033030.1580891-1-lilinmao@kylinos.cn?part=1