Re: [PATCH v4 2/4] tty: serdev: Export functions to pause receive_buf callback calls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: NULL pointer dereference / UAF in ttyport_pause_rx and ttyport_resume_rx due to TOCTOU
--

commit 7862c81055f41fa0e8cc37f91200a9bcccf35960
Author: Markus Probst <markus.probst@posteo.de>

tty: serdev: Export functions to pause receive_buf callback calls

These functions will be used to simply the serdev rust abstraction. It
also contributes to the fixing of 2 race conditions in the serdev rust
abstraction.

> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index 125a17a0bea50..e164ee7edc121 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c

[ ... ]

> @@ -185,6 +190,37 @@ static bool ttyport_close(struct serdev_controller *ctrl)
>  	return true;
>  }
>  
> +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> +{
> +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +
> +	guard(mutex)(&serport->lock);

[Severity: High]
Does this code have a race condition since the local pointer tty is assigned
from serport->tty before the serport->lock is acquired?

This lock protects the lifecycle of the tty structure and the SERPORT_ACTIVE
flag, which are modified concurrently by ttyport_open() and ttyport_close().

If a concurrent thread calls ttyport_open() or if the port transitions states
asynchronously, it will allocate a new tty_struct, set serport->tty, and set
SERPORT_ACTIVE to true.

> +
> +	scoped_guard(mutex, &serport->receive_lock)
> +		set_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		tty_buffer_cancel_work(tty->port);

Because tty was captured outside the lock, could it remain NULL if previously
closed or point to a freed structure?

If the ttyport_pause_rx() thread then acquires the lock and sees that
SERPORT_ACTIVE is true, will this result in an immediate NULL pointer
dereference or use-after-free when accessing tty->port?

Should the assignment of the tty pointer be moved inside the critical section?

> +}
> +
> +static void ttyport_resume_rx(struct serdev_controller *ctrl)
> +{
> +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +
> +	guard(mutex)(&serport->lock);

[Severity: High]
Does the identical vulnerability exist in the resume path where tty is
captured outside the lock?

> +
> +	if (!test_bit(SERPORT_PAUSE_RX, &serport->flags))
> +		return;
> +
> +	scoped_guard(mutex, &serport->receive_lock)
> +		clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		tty_buffer_restart_work(tty->port);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-rust_serdev_probe_refactor-v4-0-46832e034757@posteo.de?part=2