Re: [net,v1] serial: caif: fix use-after-free in caif_serial ldisc_close()

["Jiayuan Chen" <jiayuan.chen@linux.dev>]

February 6, 2026 at 10:25, "Jakub Kicinski" <kuba@kernel.org mailto:kuba@kernel.org?to=%22Jakub%20Kicinski%22%20%3Ckuba%40kernel.org%3E > wrote:


Thanks for the review.

> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> serial: caif: fix use-after-free in caif_serial ldisc_close()
> 
> This commit fixes a use-after-free bug in the CAIF serial driver by
> moving the tty_kref_put() call from ldisc_close() to ser_release(),
> ensuring the tty reference is held as long as the network device exists.
> 
> Closes: https://lore.kernel.org/all/000000000000a4a7550611e234f5@google.com/T/
> 
> > 
> > serial: caif: fix use-after-free in caif_serial ldisc_close()
> > 
> >  There is a use-after-free bug in caif_serial where handle_tx() may
> >  access ser->tty after the tty has been freed.
> > 
> >  The race condition occurs between ldisc_close() and packet transmission:
> > 
> >  CPU 0 (close) CPU 1 (xmit)
> >  ------------- ------------
> >  ldisc_close()
> >  tty_kref_put(ser->tty)
> >  [tty may be freed here]
> >  <-- race window -->
> >  caif_xmit()
> >  handle_tx()
> >  tty = ser->tty // dangling ptr
> >  tty->ops->write() // UAF!
> >  schedule_work()
> >  ser_release()
> >  unregister_netdevice()
> > 
> >  The root cause is that tty_kref_put() is called in ldisc_close() while
> >  the network device is still active and can receive packets.
> > 
> >  Since ser and tty have a 1:1 binding relationship with consistent
> >  lifecycles (ser is created/destroyed in ldisc_open/close, and each ser
> >  binds exactly one tty), we can safely defer the tty reference release
> > 
> The statement "ser is created/destroyed in ldisc_open/close" isn't quite
> accurate. While ser is created in ldisc_open() via alloc_netdev(), it's
> not actually destroyed in ldisc_close(). Instead, ldisc_close() only moves
> ser to the release list via list_move(&ser->node, &ser_release_list).
> The actual destruction happens in ser_release() when unregister_netdevice()
> is called, which eventually frees the netdev and embedded ser via
> netdev_run_todo() invoked by rtnl_unlock(). Would it be more accurate to
> say ser is destroyed in ser_release()?

The description of ser's lifecycle is imprecise - ser is indeed not destroyed
in ldisc_close() but rather in ser_release() via unregister_netdevice().

I'll update the commit message to clarify this.


[...]
> > 
> >  Note: There is a separate deadloop issue in handle_tx() when using
> >  PORT_UNKNOWN serial ports (e.g., /dev/ttyS3 in QEMU without proper
> >  serial backend). This deadloop exists even without this patch,
> >  and is caused by an independent bug: inconsistency between
> >  uart_write_room() and uart_write() in serial core. It has been
> >  addressed in a separate patch [2].
> > 
> The claim about the deadloop being caused by inconsistency between
> uart_write_room() and uart_write() couldn't be fully verified without
> examining the referenced patch [2]. While the claim is plausible (if
> uart_write_room() reports available space but uart_write() returns 0
> bytes written, the deadloop would occur in handle_tx()), this appears
> to be presented as fact rather than hypothesis. Could this be clarified
> or softened to indicate it's the suspected cause pending verification?


The deadloop is not a hypothesis - it's a confirmed, independently reproducible bug.
My reproducer can trigger the use-after-free; after applying this fix (or disabling KASAN), the same
reproducer hits the deadloop, which also reproduces without this patch at all.

The separate fix [2] has been partially confirmed by Greg Kroah-Hartman.

I don't think any change is needed here.

> [ ... ]
> 
> > 
> > Fixes: 9b27105b4a44 ("net-caif-driver: add CAIF serial driver (ldisc)")
> > 
> The Fixes: tag appears to reference the wrong commit. The bug was actually
> introduced by commit 56e0ef527b18 ("drivers/net: caif: fix wrong
> rtnl_is_locked() usage"). The original commit 9b27105b4a44 didn't have any
> tty_kref operations. Commit e31d5a05948e added tty_kref_get/put, but at
> that time tty_kref_put was called in ldisc_close() which directly
> unregistered the netdevice, so there was no race. Commit 56e0ef527b18
> moved dev_close/unregister_netdevice to a work queue (ser_release) but
> left tty_kref_put in ldisc_close, creating the race window. Should the
> Fixes: tag be updated to:
> 
> Fixes: 56e0ef527b18 ("drivers/net: caif: fix wrong rtnl_is_locked() usage")


You're right about the Fixes tag. I'll update the Fixes tag accordingly.

> > 
> > diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
> >  --- a/drivers/net/caif/caif_serial.c
> >  +++ b/drivers/net/caif/caif_serial.c
> >  @@ -284,6 +284,7 @@ static void ser_release(struct work_struct *work)
> >  {
> >  struct list_head list;
> >  struct ser_device *ser, *tmp;
> >  + struct tty_struct *tty;
> > 
> >  spin_lock(&ser_lock);
> >  list_replace_init(&ser_release_list, &list);
> >  @@ -292,9 +293,11 @@ static void ser_release(struct work_struct *work)
> >  if (!list_empty(&list)) {
> >  rtnl_lock();
> >  list_for_each_entry_safe(ser, tmp, &list, node) {
> >  + tty = ser->tty;
> >  dev_close(ser->dev);
> >  unregister_netdevice(ser->dev);
> >  debugfs_deinit(ser);
> >  + tty_kref_put(tty);
> >  }
> >  rtnl_unlock();
> >  }
> > 
> [ ... ]
> 
> > 
> > @@ -354,8 +357,6 @@ static int ldisc_open(struct tty_struct *tty)
> >  static void ldisc_close(struct tty_struct *tty)
> >  {
> >  struct ser_device *ser = tty->disc_data;
> >  -
> >  - tty_kref_put(ser->tty);
> > 
> >  spin_lock(&ser_lock);
> >  list_move(&ser->node, &ser_release_list);
> > 
> -- 
> pw-bot: cr
>