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

--=-mx5cNm777Bf9oKHRXlOr
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 <[email protected]>
> > >=20
> > > 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.
> > >=20
> > > 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.
> > >=20
> > > Assisted-by: AISLE:Snapshot
> > > Cc: stable <[email protected]>
> > > Signed-off-by: Joshua Rogers <[email protected]>
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > > ---
> > >  drivers/tty/serdev/serdev-ttyport.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >=20
> > > 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 =3D port->client_data;
> > > -	struct serport *serport =3D serdev_controller_get_drvdata(ctrl);
> > > +	struct serport *serport;
> > >  	size_t ret;
> > > =20
> > > +	if (!ctrl)
> > > +		return 0;
> > > +
> > > +	serport =3D serdev_controller_get_drvdata(ctrl);
> > > +
> > >  	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
> > >  		return 0;
> > > =20
> > > @@ -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 =3D port->client_data;
> > > -	struct serport *serport =3D serdev_controller_get_drvdata(ctrl);
> > > +	struct serport *serport;
> > >  	struct tty_struct *tty;
> > > =20
> > > +	if (!ctrl)
> > > +		return;
> > > +
> > > +	serport =3D serdev_controller_get_drvdata(ctrl);
> > > +
> > >  	tty =3D tty_port_tty_get(port);
> > >  	if (!tty)
> > >  		return;
> > > @@ -312,6 +322,7 @@ int serdev_tty_port_unregister(struct tty_port *p=
ort)
> > >  		return -ENODEV;
> > > =20
> > >  	serdev_controller_remove(ctrl);
> > > +	cancel_work_sync(&port->buf.work);
> > >  	port->client_data =3D NULL;
> > >  	port->client_ops =3D &tty_port_default_client_ops;
> > >  	serdev_controller_put(ctrl);
> >=20
> > So why exactly does tty keep calling `receive_buf` and `write_wakeup`
> > with the tty port closed?
> >=20
> > After `serdev_controller_remove` is called, the tty port should already
> > be closed by the drivers.
>=20
> 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.)
>=20
> 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

>=20
> thanks,
>=20
> greg k-h

--=-mx5cNm777Bf9oKHRXlOr
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part

-----BEGIN PGP SIGNATURE-----

iQJPBAABCAA5FiEEgnQYxPSsWOdyMMRzNHYf+OetQ9IFAmpsmEobFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwyAAoJEDR2H/jnrUPSPnoP/0cy0RT9oHV7SXZFZtha
c1nq/y1RcBgmcbNulJ06C2nBg7uT21PzKOzFuhXEMQg8JeLen0LVNhTjad8+Ma5h
pMRVZLf48NqikqnbsMqhtM0b8SSFGsVtUL8HVvaDepyxP4r0FpGkggtm2aGts6ON
fqkAAiV1xKMcbjNNS9x1MfVNMvqgfI47Fau4yyVKu8nT3BQaaCSYPwaNbTUOVIkY
A8hU1E8VExVfkl/GiqXsbeF9HCwcE3twg3ZEBm+7P9398cGq3DuX5RYRrZK+PNl2
1RNarJRoK1Hkuz411gL492QHkqw97eLMj9qBWdQRt2xWUgRV7AM4YUUQ47otxc+0
3XZ5c+THK7saEzc5rhQ5Etss3YUnz4+xWmXlf2Yku7XJGujThfqct2hEl7mUQMtD
EfTmTTdpB3FsLrruxpTkAiq2hXLH0c9x5tzKM92g9/G+uR1udxVys6RU8QqCsAGo
vxiXCFZwVRMChEYOb/WXWVOKv1/bnbE2rN0U8AirT2nxvf5UCEqyUpVI46AGs/dR
GAot9jtfxX3tF1wM7E/lgAqBi54bv+Wrzy1ontwo0KHHQcgKo8H94scxE/8YFCxZ
ICX4u65gjDLehjTNk0eGQ8GhcO/R7sachZvg7UJTsKTw8xk16t1vuMwSJ9rjKDx1
ZquopxFs8kIG+bfzZsY1YNnA
=88FJ
-----END PGP SIGNATURE-----

--=-mx5cNm777Bf9oKHRXlOr--