Re: [net,v2,1/1] serial: caif: fix remaining ser->tty UAF in TX path

[Simon Horman]

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 remaining ser->tty UAF in TX path

This patch addresses a KASAN use-after-free in the caif_serial TX path
by adding a spinlock to serialize access to ser->tty. The TX path now
takes a tty kref under the lock, and ser_release() clears ser->tty under
the same lock before dropping the reference.

Link: https://groups.google.com/g/syzkaller/c/usNe0oKtoXw/m/x8qUc3yUAQAJ

> 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
> @@ -68,6 +68,7 @@ struct ser_device {
>  	struct net_device *dev;
>  	struct sk_buff_head head;
>  	struct tty_struct *tty;
> +	spinlock_t tty_lock; /* protects ser->tty */
>  	bool tx_started;
>  	unsigned long state;
>  #ifdef CONFIG_DEBUG_FS
> @@ -197,12 +198,21 @@ static int handle_tx(struct ser_device *ser)
>  	struct sk_buff *skb;
>  	int tty_wr, len, room;
>
> +	spin_lock(&ser->tty_lock);
>  	tty = ser->tty;
> +	tty_kref_get(tty);
> +	spin_unlock(&ser->tty_lock);

handle_tx() is called from caif_xmit(), which is the ndo_start_xmit
handler. This can run in softirq context (NET_TX softirq). ser_release()
runs in process context (workqueue). If ser_release() holds tty_lock in
process context and a softirq fires on the same CPU, calling caif_xmit()
-> handle_tx() -> spin_lock(&ser->tty_lock), won't this deadlock?

Should this use spin_lock_bh()/spin_unlock_bh() instead?

> +
> +	if (!tty)
> +		return 0;
> +
>  	ser->tx_started = true;
>
>  	/* Enter critical section */
> -	if (test_and_set_bit(CAIF_SENDING, &ser->state))
> +	if (test_and_set_bit(CAIF_SENDING, &ser->state)) {
> +		tty_kref_put(tty);
>  		return 0;
> +	}
>
>  	/* skb_peek is safe because handle_tx is called after skb_queue_tail */
>  	while ((skb = skb_peek(&ser->head)) != NULL) {

[ ... ]

>  		if (!ser_loop) {
>  			tty_wr = tty->ops->write(tty, skb->data, len);
>  			update_tty_status(ser);
>  		} else {

After ser_release() sets ser->tty = NULL (the new code from this patch),
a concurrent handle_tx() that already passed the NULL check above can
reach update_tty_status(ser) here. Looking at update_tty_status(), it
dereferences ser->tty directly:

drivers/net/caif/caif_serial.c:update_tty_status() {
    inline_decode(ser->common.flowctrl,
        ser->tty->flow.stopped << 5 |
        ser->tty->flow.tco_stopped << 3 |
        ser->tty->ctrl.packet << 2);
}

Won't this cause a NULL pointer dereference? Should update_tty_status()
take the local tty pointer as a parameter instead of reading ser->tty?

> @@ -245,9 +255,11 @@
>  		ser->common.flowctrl != NULL)
>  				ser->common.flowctrl(ser->dev, ON);
>  	clear_bit(CAIF_SENDING, &ser->state);
> +	tty_kref_put(tty);
>  	return 0;
>  error:
>  	clear_bit(CAIF_SENDING, &ser->state);
> +	tty_kref_put(tty);
>  	return tty_wr;
>  }
>
> @@ -293,7 +305,10 @@ static void ser_release(struct work_struct *work)
>  	if (!list_empty(&list)) {
>  		rtnl_lock();
>  		list_for_each_entry_safe(ser, tmp, &list, node) {
> +			spin_lock(&ser->tty_lock);
>  			tty = ser->tty;
> +			ser->tty = NULL;
> +			spin_unlock(&ser->tty_lock);
>  			dev_close(ser->dev);
>  			unregister_netdevice(ser->dev);
>  			debugfs_deinit(ser);

[ ... ]

Jakub Kicinski's review of v1 (commit 308e7e4d0a84) pointed out that
moving tty_kref_put() to ser_release() doesn't fully prevent the UAF
because handle_tx() can still fetch ser->tty while ser_release() is
dropping the last tty reference. The current patch addresses this by
adding the tty_lock synchronization. While the v1/v2 review thread
didn't explicitly mark this as blocking, it represents a legitimate
race window that this patch fixes.

Reference: https://lore.kernel.org/netdev/20260206022526.3191971-1-kuba@xxxxxxxxxx/
-- 
pw-bot: changes-requested