Re: [PATCH 1/2] serdev: fix race between tty-port unregister and in-flight callbacks

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

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

On Fri, 2026-07-31 at 14:01 +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 31, 2026 at 11:30:53AM +0000, Markus Probst wrote:
> > On Fri, 2026-07-31 at 10:06 +0200, Greg Kroah-Hartman wrote:
> > > From: Joshua Rogers <linux@joshua.hu>
> > > 
> > > serdev_tty_port_unregister() clears port->client_data and frees the
> > > controller without synchronizing with in-flight flip buffer work.
> > > This can cause NULL pointer dereferences or use-after-free if
> > > ttyport_receive_buf() or ttyport_write_wakeup() runs concurrently.
> > > 
> > > Add cancel_work_sync() to drain pending buffer work before clearing
> > > state, and add NULL checks for client_data in both callbacks as
> > > secondary hardening.
> > > 
> > > Assisted-by: AISLE:Snapshot
> > > Cc: stable <stable@kernel.org>
> > > Signed-off-by: Joshua Rogers <linux@joshua.hu>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  drivers/tty/serdev/serdev-ttyport.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> > > index bab1b143b8a6..48ce5b3f8308 100644
> > > --- a/drivers/tty/serdev/serdev-ttyport.c
> > > +++ b/drivers/tty/serdev/serdev-ttyport.c
> > > @@ -26,9 +26,14 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
> > >  				  const u8 *fp, size_t count)
> > >  {
> > >  	struct serdev_controller *ctrl = port->client_data;
> > > -	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> > > +	struct serport *serport;
> > >  	size_t ret;
> > >  
> > > +	if (!ctrl)
> > > +		return 0;
> > > +
> > > +	serport = serdev_controller_get_drvdata(ctrl);
> > > +
> > >  	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> > >  		return 0;
> > >  
> > > @@ -46,9 +51,14 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
> > >  static void ttyport_write_wakeup(struct tty_port *port)
> > >  {
> > >  	struct serdev_controller *ctrl = port->client_data;
> > > -	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> > > +	struct serport *serport;
> > >  	struct tty_struct *tty;
> > >  
> > > +	if (!ctrl)
> > > +		return;
> > > +
> > > +	serport = serdev_controller_get_drvdata(ctrl);
> > > +
> > >  	tty = tty_port_tty_get(port);
> > >  	if (!tty)
> > >  		return;
> > > @@ -312,6 +322,7 @@ int serdev_tty_port_unregister(struct tty_port *port)
> > >  		return -ENODEV;
> > >  
> > >  	serdev_controller_remove(ctrl);
> > > +	cancel_work_sync(&port->buf.work);
> > >  	port->client_data = NULL;
> > >  	port->client_ops = &tty_port_default_client_ops;
> > >  	serdev_controller_put(ctrl);
> > 
> > So why exactly does tty keep calling `receive_buf` and `write_wakeup`
> > with the tty port closed?
> > 
> > After `serdev_controller_remove` is called, the tty port should already
> > be closed by the drivers.
> 
> Are you sure?  remove can be called by a device removal, while close
> might be coming from a different code path (like it is in userspace.)
> 
> If this is impossible, as close will ALWAYS happen before remove, then
> it's not an issue, but that is probably not the case, right?

The driver *should* always close the serdev device on remove if
previously opened. Thus the existence of `devm_serdev_device_open`.

The serdev subsystem itself does not guarantee that it has been called.

I just took a look at every driver using `serdev_device_open` (non
devm), and every one seems to call `serdev_device_close` on remove.

Assuming `cancel_work_sync(&port->buf.work);` is used correctly here,
we could still take the patch for hardening.

Thanks
- Markus Probst

> 
> thanks,
> 
> greg k-h

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