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

[Jakub Kicinski <kuba@kernel.org>]

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()?

> to ser_release() where the network device is unregistered.
>
> Fix this by moving tty_kref_put() from ldisc_close() to ser_release(),
> after unregister_netdevice(). This ensures the tty reference is held
> as long as the network device exists, preventing the UAF.
>
> Note: We save ser->tty before unregister_netdevice() because ser is
> embedded in netdev's private data and will be freed along with netdev
> (needs_free_netdev = true).
>
> How to reproduce: Add mdelay(500) at the beginning of ldisc_close()
> to widen the race window, then run the reproducer program [1].
>
> 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?

[ ... ]

> 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")

> 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