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

[Shuangpeng Bai <shuangpeng.kernel@gmail.com>]

A reproducer exposes a KASAN use-after-free in caif_serial's TX path
(e.g., via tty_write_room() / tty->ops->write()) on top of commit
<308e7e4d0a84> ("serial: caif: fix use-after-free in caif_serial
ldisc_close()").

That commit moved tty_kref_put() to ser_release(). There is still a race
because the TX path may fetch ser->tty and use it while ser_release()
drops the last tty reference:

    CPU 0 (ser_release worker)         CPU 1 (xmit)
    -------------------------        ------------
                                      caif_xmit()
                                      handle_tx()
                                        tty = ser->tty

    ser_release()
      tty = ser->tty
      dev_close(ser->dev)
      unregister_netdevice(ser->dev)
      debugfs_deinit(ser)
      tty_kref_put(tty)        // may drop the last ref
                     <-- race window -->
                                        tty->ops->write(tty, ...)  // UAF

Fix it by serializing accesses to ser->tty with a dedicated lock. The TX
path grabs a tty kref under the lock and drops it after the TX attempt,
while ser_release() clears ser->tty under the same lock before putting the
old tty reference. This prevents the TX path from observing a freed tty
object via ser->tty.

With this change applied, the reproducer no longer triggers the UAF in my
test.

One concern is that handle_tx() can be a hot path. This fix adds a short
lock-held section plus an extra tty kref get/put per TX run. Feedback on
the performance impact, or suggestions for a lower-overhead approach, are
welcome.

Link: https://groups.google.com/g/syzkaller/c/usNe0oKtoXw/m/x8qUc3yUAQAJ
Fixes: 308e7e4d0a84 ("serial: caif: fix use-after-free in caif_serial ldisc_close()")
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
 drivers/net/caif/caif_serial.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
index b90890030751..ddd90d01cd40 100644
--- 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);
+
+	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) {
@@ -245,9 +255,11 @@ static int handle_tx(struct ser_device *ser)
 		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);
@@ -330,6 +345,7 @@ static int ldisc_open(struct tty_struct *tty)
 		return -ENOMEM;
 
 	ser = netdev_priv(dev);
+	spin_lock_init(&ser->tty_lock);
 	ser->tty = tty_kref_get(tty);
 	ser->dev = dev;
 	debugfs_init(ser, tty);
-- 
2.34.1