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

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 3947 bytes --]

On Wed, 2026-09-30 at 17:43 +0000, sashiko-bot@kernel.org wrote:
> 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().
It was never intended to be called from inside 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).
True.

> 
> > +
> > +	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.
Already mentioned in a comment (true).

> 
> > +}
> > +
> > +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?
True.

> 
> > +
> > +	guard(mutex)(&serport->lock);
> > +
> > +	clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> > +
> > +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> > +		tty_buffer_restart_work(tty->port);
> > +}
> > +

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]