Re: [PATCH v3 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 3 potential issue(s) to consider:
- [High] tty: serdev: AB-BA deadlock between ttyport_pause_rx() and ttyport_receive_buf()
- [High] tty: serdev: Self-deadlock calling pause_rx or resume_rx from receive_buf
- [High] tty: serdev: TOCTOU race leading to NULL pointer dereference in ttyport_close/pause/resume
--

commit f462dbe8ac06c8a92e0fb79ce15c1e6444a27110
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 c11908f5e1ce3..2bed0e54202a8 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c

[ ... ]

> @@ -35,6 +37,9 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
>  	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
>  		return 0;
>  
> +	if (test_bit(SERPORT_PAUSE_RX, &serport->flags))
> +		return 0;
> +
>  	ret = serdev_controller_receive_buf(ctrl, cp, count);

[Severity: High]
Can this cause a self-deadlock if the driver attempts to pause or resume
reception during data reception?

If the downstream driver's receive callback calls serdev_device_pause_rx()
or serdev_device_resume_rx(), they will attempt to acquire the non-recursive
serport->lock which is already held by ttyport_receive_buf().

[ ... ]

> @@ -177,6 +182,32 @@ 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;

[Severity: High]
Is it possible for this unprotected read of serport->tty to introduce a TOCTOU
race leading to a NULL pointer dereference?

If ttyport_pause_rx() races with ttyport_open(), the local tty pointer could
be captured as NULL before acquiring the lock. After acquiring the lock, the
code might observe SERPORT_ACTIVE as true (set by the concurrent open) and
proceed to dereference the stale, NULL tty pointer.

(Note: ttyport_close() also appears to have this pre-existing issue).

> +
> +	guard(mutex)(&serport->lock);
> +
> +	set_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		tty_buffer_flush_work(tty->port);

[Severity: High]
Could this tty_buffer_flush_work() cause an AB-BA deadlock with
ttyport_receive_buf()?

ttyport_pause_rx() acquires serport->lock and then calls
tty_buffer_flush_work(), which blocks waiting for the currently executing work
to finish. If the current work is flush_to_ldisc(), it calls
ttyport_receive_buf(), which attempts to acquire serport->lock, resulting in
a deadlock.

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

[Severity: High]
Can this also cause a NULL pointer dereference if it races with ttyport_open()
and captures a NULL tty pointer before acquiring the lock?

> +
> +	guard(mutex)(&serport->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/20260930-rust_serdev_probe_refactor-v3-0-d1b6e77135e0@posteo.de?part=2