[PATCH net v4] serial: caif: hold tty->link reference in ldisc_open and ser_release

[PATCH net v4] serial: caif: hold tty->link reference in ldisc_open and ser_release

[Shuangpeng Bai]

A reproducer triggers a KASAN slab-use-after-free in pty_write_room()
when caif_serial's TX path calls tty_write_room(). The faulting access
is on tty->link->port.

Hold an extra kref on tty->link for the lifetime of the caif_serial line
discipline: get it in ldisc_open() and drop it in ser_release(), and
also drop it on the ldisc_open() error path.

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

This issue becomes reproducible on top of 308e7e4d0a84. Before that, the
reproducer typically hits another bug first, so this UAF is not
observable there.

Link: https://lore.kernel.org/all/20260228094741.1e248271@kernel.org/
Link: https://gist.github.com/shuangpengbai/c898debad6bdf170a84be7e6b3d8707f
Fixes: 308e7e4d0a84 ("serial: caif: fix use-after-free in caif_serial ldisc_close()")
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
Changes since v3:
  - No code changes; repost without cover letter and with updated Cc list.

 drivers/net/caif/caif_serial.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
index b90890030751..1873d8287bb9 100644
--- a/drivers/net/caif/caif_serial.c
+++ b/drivers/net/caif/caif_serial.c
@@ -297,6 +297,7 @@ static void ser_release(struct work_struct *work)
 			dev_close(ser->dev);
 			unregister_netdevice(ser->dev);
 			debugfs_deinit(ser);
+			tty_kref_put(tty->link);
 			tty_kref_put(tty);
 		}
 		rtnl_unlock();
@@ -331,6 +332,7 @@ static int ldisc_open(struct tty_struct *tty)
 
 	ser = netdev_priv(dev);
 	ser->tty = tty_kref_get(tty);
+	tty_kref_get(tty->link);
 	ser->dev = dev;
 	debugfs_init(ser, tty);
 	tty->receive_room = 4096;
@@ -339,6 +341,7 @@ static int ldisc_open(struct tty_struct *tty)
 	rtnl_lock();
 	result = register_netdevice(dev);
 	if (result) {
+		tty_kref_put(tty->link);
 		tty_kref_put(tty);
 		rtnl_unlock();
 		free_netdev(dev);
-- 
2.34.1

Re: [net,v4] serial: caif: hold tty->link reference in ldisc_open and ser_release

[Jakub Kicinski]

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: hold tty->link reference in ldisc_open and ser_release

This commit addresses a KASAN slab-use-after-free in pty_write_room()
triggered when caif_serial's TX path calls tty_write_room(). The fix
holds an extra reference on tty->link throughout the lifetime of the
caif_serial line discipline.

Link: https://lore.kernel.org/all/20260228094741.1e248271@kernel.org/
Link: https://gist.github.com/shuangpengbai/c898debad6bdf170a84be7e6b3d8707f

The Fixes tag points to commit 308e7e4d0a84 ("serial: caif: fix
use-after-free in caif_serial ldisc_close()"), but the regression was
actually introduced by commit e31d5a05948e ("caif: tty's are kref
objects so take a reference"). That commit added tty_kref_get(tty) to
hold a reference on the tty, but failed to also hold a reference on
tty->link, which is accessed via tty_write_room() in handle_tx().

Commit 308e7e4d0a84 merely exposed this existing regression by changing
the timing.

Should the Fixes tag be updated to:

    Fixes: e31d5a05948e ("caif: tty's are kref objects so take a reference")

> diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
> index b90890030751..1873d8287bb9 100644
> --- a/drivers/net/caif/caif_serial.c
> +++ b/drivers/net/caif/caif_serial.c
> @@ -297,6 +297,7 @@ static void ser_release(struct work_struct *work)
>  			dev_close(ser->dev);
>  			unregister_netdevice(ser->dev);
>  			debugfs_deinit(ser);
> +			tty_kref_put(tty->link);
                        ^^^^^^^^^^^^^^^^^^^^

Can this dereference NULL? The tty->link field is only set for PTY
(pseudo-terminal) pairs. For regular serial ports and other non-PTY
devices, tty->link is NULL.

The CAIF line discipline can be attached to any TTY device via the
TIOCSETD ioctl. When ser_release() is called to clean up a ser_device
that was created for a non-PTY TTY device, tty->link would be NULL.

Should this check for NULL before calling tty_kref_put()?

>  			tty_kref_put(tty);
>  		}
>  		rtnl_unlock();
> @@ -331,6 +332,7 @@ static int ldisc_open(struct tty_struct *tty)
>
>  	ser = netdev_priv(dev);
>  	ser->tty = tty_kref_get(tty);
> +	tty_kref_get(tty->link);
        ^^^^^^^^^^^^^^^^^^^^^^^^

Can this dereference NULL? When a user attaches the CAIF line discipline
to a non-PTY TTY device via TIOCSETD, ldisc_open() is called with a tty
where tty->link is NULL.

While ldisc_open() performs capability checks (CAP_SYS_ADMIN), there is
no code restricting this to PTY devices only.

Should this check for NULL before calling tty_kref_get()?

>  	ser->dev = dev;
>  	debugfs_init(ser, tty);
>  	tty->receive_room = 4096;
> @@ -339,6 +341,7 @@ static int ldisc_open(struct tty_struct *tty)
>  	rtnl_lock();
>  	result = register_netdevice(dev);
>  	if (result) {
> +		tty_kref_put(tty->link);
                ^^^^^^^^^^^^^^^^^^^^^^^^

Can this dereference NULL in the error path? If register_netdevice()
fails after the CAIF line discipline was attached to a non-PTY TTY
device, the error path would attempt to release a reference on
tty->link when it is NULL.

Should this check for NULL before calling tty_kref_put()?

>  		tty_kref_put(tty);
>  		rtnl_unlock();
>  		free_netdev(dev);

Simon Horman raised a concern in v2 about a NULL pointer dereference in
update_tty_status(). The v4 patch does not use the spinlock approach
from v2, and does not address this race condition.

After ser_release() sets ser->tty = NULL, a concurrent handle_tx() that
already passed the NULL check can reach update_tty_status(ser).

Looking at update_tty_status():

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);
}

It dereferences ser->tty directly (ser->tty->flow.stopped,
ser->tty->flow.tco_stopped, ser->tty->ctrl.packet).

Can this cause a NULL pointer dereference?

Simon also raised a concern about potential deadlock with spinlock usage
in v2. The v4 patch removes the spinlock approach entirely.

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

How does the current synchronization between handle_tx() and
ser_release() prevent races?
-- 
pw-bot: cr

Re: [net,v4] serial: caif: hold tty->link reference in ldisc_open and ser_release

[Shuangpeng]

> On Mar 3, 2026, at 20:26, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> 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: hold tty->link reference in ldisc_open and ser_release
> 
> This commit addresses a KASAN slab-use-after-free in pty_write_room()
> triggered when caif_serial's TX path calls tty_write_room(). The fix
> holds an extra reference on tty->link throughout the lifetime of the
> caif_serial line discipline.
> 
> Link: https://lore.kernel.org/all/20260228094741.1e248271@kernel.org/
> Link: https://gist.github.com/shuangpengbai/c898debad6bdf170a84be7e6b3d8707f
> 
> The Fixes tag points to commit 308e7e4d0a84 ("serial: caif: fix
> use-after-free in caif_serial ldisc_close()"), but the regression was
> actually introduced by commit e31d5a05948e ("caif: tty's are kref
> objects so take a reference"). That commit added tty_kref_get(tty) to
> hold a reference on the tty, but failed to also hold a reference on
> tty->link, which is accessed via tty_write_room() in handle_tx().
> 
> Commit 308e7e4d0a84 merely exposed this existing regression by changing
> the timing.
> 
> Should the Fixes tag be updated to:
> 
>    Fixes: e31d5a05948e ("caif: tty's are kref objects so take a reference")
> 

Thanks for confirming the correct Fixes commit. I will update the Fixes
tag in the next revision.

>> diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
>> index b90890030751..1873d8287bb9 100644
>> --- a/drivers/net/caif/caif_serial.c
>> +++ b/drivers/net/caif/caif_serial.c
>> @@ -297,6 +297,7 @@ static void ser_release(struct work_struct *work)
>> dev_close(ser->dev);
>> unregister_netdevice(ser->dev);
>> debugfs_deinit(ser);
>> + tty_kref_put(tty->link);
>                        ^^^^^^^^^^^^^^^^^^^^
> 
> Can this dereference NULL? The tty->link field is only set for PTY
> (pseudo-terminal) pairs. For regular serial ports and other non-PTY
> devices, tty->link is NULL.
> 
> The CAIF line discipline can be attached to any TTY device via the
> TIOCSETD ioctl. When ser_release() is called to clean up a ser_device
> that was created for a non-PTY TTY device, tty->link would be NULL.
> 
> Should this check for NULL before calling tty_kref_put()?
> 

tty_kref_get() / tty_kref_put() are NULL-safe, so calling them with
tty->link == NULL is not a functional issue.

I can add explicit if (tty->link) guards to improve readability if you prefer. 
Please let me know if you would like me to include those checks. 
This also applies to the other concerns in the patch where tty->link may
be NULL.


>> tty_kref_put(tty);
>> }
>> rtnl_unlock();
>> @@ -331,6 +332,7 @@ static int ldisc_open(struct tty_struct *tty)
>> 
>> ser = netdev_priv(dev);
>> ser->tty = tty_kref_get(tty);
>> + tty_kref_get(tty->link);
>        ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Can this dereference NULL? When a user attaches the CAIF line discipline
> to a non-PTY TTY device via TIOCSETD, ldisc_open() is called with a tty
> where tty->link is NULL.
> 
> While ldisc_open() performs capability checks (CAP_SYS_ADMIN), there is
> no code restricting this to PTY devices only.
> 
> Should this check for NULL before calling tty_kref_get()?
> 
>> ser->dev = dev;
>> debugfs_init(ser, tty);
>> tty->receive_room = 4096;
>> @@ -339,6 +341,7 @@ static int ldisc_open(struct tty_struct *tty)
>> rtnl_lock();
>> result = register_netdevice(dev);
>> if (result) {
>> + tty_kref_put(tty->link);
>                ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Can this dereference NULL in the error path? If register_netdevice()
> fails after the CAIF line discipline was attached to a non-PTY TTY
> device, the error path would attempt to release a reference on
> tty->link when it is NULL.
> 
> Should this check for NULL before calling tty_kref_put()?
> 
>> tty_kref_put(tty);
>> rtnl_unlock();
>> free_netdev(dev);
> 
> Simon Horman raised a concern in v2 about a NULL pointer dereference in
> update_tty_status(). The v4 patch does not use the spinlock approach
> from v2, and does not address this race condition.
> 
> After ser_release() sets ser->tty = NULL, a concurrent handle_tx() that
> already passed the NULL check can reach update_tty_status(ser).
> 
> Looking at update_tty_status():
> 
> 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);
> }
> 
> It dereferences ser->tty directly (ser->tty->flow.stopped,
> ser->tty->flow.tco_stopped, ser->tty->ctrl.packet).
> 
> Can this cause a NULL pointer dereference?
> 
> Simon also raised a concern about potential deadlock with spinlock usage
> in v2. The v4 patch removes the spinlock approach entirely.
> 
> 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).
> 
> How does the current synchronization between handle_tx() and
> ser_release() prevent races?

We keep an extra reference to tty->link for the lifetime of the CAIF
ser_device, until ser_release() unregisters the netdevice. We believe
this can cover all the usages in handle_tx(), but please feel free to let
us know if there are uncaught issues by us.



> -- 
> pw-bot: cr

Re: [net,v4] serial: caif: hold tty->link reference in ldisc_open and ser_release

[Jakub Kicinski]

On Wed, 4 Mar 2026 20:23:20 -0500 Shuangpeng wrote:
> > Can this dereference NULL? The tty->link field is only set for PTY
> > (pseudo-terminal) pairs. For regular serial ports and other non-PTY
> > devices, tty->link is NULL.
> > 
> > The CAIF line discipline can be attached to any TTY device via the
> > TIOCSETD ioctl. When ser_release() is called to clean up a ser_device
> > that was created for a non-PTY TTY device, tty->link would be NULL.
> > 
> > Should this check for NULL before calling tty_kref_put()?
> >   
> 
> tty_kref_get() / tty_kref_put() are NULL-safe, so calling them with
> tty->link == NULL is not a functional issue.
> 
> I can add explicit if (tty->link) guards to improve readability if you prefer. 

Sorry, I should have checked, I'm surprised the AI agent got something
this basic wrong. Just the Fixes tag, then..

Re: [net,v4] serial: caif: hold tty->link reference in ldisc_open and ser_release

[Shuangpeng]

> On Mar 4, 2026, at 20:39, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Wed, 4 Mar 2026 20:23:20 -0500 Shuangpeng wrote:
>>> Can this dereference NULL? The tty->link field is only set for PTY
>>> (pseudo-terminal) pairs. For regular serial ports and other non-PTY
>>> devices, tty->link is NULL.
>>> 
>>> The CAIF line discipline can be attached to any TTY device via the
>>> TIOCSETD ioctl. When ser_release() is called to clean up a ser_device
>>> that was created for a non-PTY TTY device, tty->link would be NULL.
>>> 
>>> Should this check for NULL before calling tty_kref_put()?
>>> 
>> 
>> tty_kref_get() / tty_kref_put() are NULL-safe, so calling them with
>> tty->link == NULL is not a functional issue.
>> 
>> I can add explicit if (tty->link) guards to improve readability if you prefer.
> 
> Sorry, I should have checked, I'm surprised the AI agent got something
> this basic wrong. Just the Fixes tag, then..

Thanks for confirming. If there are no other concerns, I'll update the Fixes 
tag and send the final version.