* [RFC] tty: n_gsm: fix use-after-free in gsmtty_install
@ 2026-04-15 2:48 Kito Xu (veritas501)
2026-04-15 3:17 ` [PATCH] " Kito Xu (veritas501)
0 siblings, 1 reply; 3+ messages in thread
From: Kito Xu (veritas501) @ 2026-04-15 2:48 UTC (permalink / raw)
To: gregkh, jirislaby; +Cc: linux-serial, linux-kernel, Kito Xu (veritas501)
gsmtty_install() reads gsm_mux[] without holding gsm_mux_lock and
defers mux_get() about 40 lines later. A concurrent gsmld_close() can
free the mux through gsm_cleanup_mux() -> mux_put() -> kfree(gsm) in
that window, leading to a use-after-free when gsmtty_install() later
dereferences the stale pointer.
race condition:
cpu 0 | cpu 1
gsmtty_install() | gsmld_close()
gsm = gsm_mux[mux] // no lock |
// CFS preempts here | gsm_cleanup_mux(gsm)
| gsm->dead = true
| mux_put(gsm)
| -> kfree(gsm)
gsm->dead // UAF! |
mutex_lock(&gsm->mutex) // UAF! |
Acquire gsm_mux_lock before reading gsm_mux[] and take the refcount
via kref_get() while still holding the lock, so the mux cannot be
freed between lookup and refcount increment.
Fixes: 86176ed90545 ("TTY: n_gsm, use tty_port_install")
Signed-off-by: Kito Xu (veritas501) <hxzene@gmail.com>
---
drivers/tty/n_gsm.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c13e050de83b..6519f1c92fc5 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -4288,14 +4288,20 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
if (mux >= MAX_MUX)
return -ENXIO;
- /* FIXME: we need to lock gsm_mux for lifetimes of ttys eventually */
- if (gsm_mux[mux] == NULL)
- return -EUNATCH;
if (line == 0 || line > 61) /* 62/63 reserved */
return -ECHRNG;
+
+ /* Acquire gsm_mux_lock to prevent concurrent gsmld_close() from
+ * freeing the mux between reading gsm_mux[] and taking a refcount.
+ */
+ spin_lock(&gsm_mux_lock);
gsm = gsm_mux[mux];
- if (gsm->dead)
- return -EL2HLT;
+ if (!gsm || gsm->dead) {
+ spin_unlock(&gsm_mux_lock);
+ return gsm ? -EL2HLT : -EUNATCH;
+ }
+ kref_get(&gsm->ref);
+ spin_unlock(&gsm_mux_lock);
/* If DLCI 0 is not yet fully open return an error.
This is ok from a locking
perspective as we don't have to worry about this
@@ -4309,8 +4315,10 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
if (dlci0->state == DLCI_OPENING)
wait_event(gsm->event, dlci0->state != DLCI_OPENING);
- if (dlci0->state != DLCI_OPEN)
+ if (dlci0->state != DLCI_OPEN) {
+ mux_put(gsm);
return -EL2NSYNC;
+ }
mutex_lock(&gsm->mutex);
}
@@ -4322,6 +4330,7 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
}
if (dlci == NULL) {
mutex_unlock(&gsm->mutex);
+ mux_put(gsm);
return -ENOMEM;
}
ret = tty_port_install(&dlci->port, driver, tty);
@@ -4329,12 +4338,12 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
if (alloc)
dlci_put(dlci);
mutex_unlock(&gsm->mutex);
+ mux_put(gsm);
return ret;
}
dlci_get(dlci);
dlci_get(gsm->dlci[0]);
- mux_get(gsm);
tty->driver_data = dlci;
mutex_unlock(&gsm->mutex);
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH] tty: n_gsm: fix use-after-free in gsmtty_install
2026-04-15 2:48 [RFC] tty: n_gsm: fix use-after-free in gsmtty_install Kito Xu (veritas501)
@ 2026-04-15 3:17 ` Kito Xu (veritas501)
2026-04-20 9:26 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Kito Xu (veritas501) @ 2026-04-15 3:17 UTC (permalink / raw)
To: gregkh, jirislaby; +Cc: linux-serial, linux-kernel, Kito Xu (veritas501)
gsmtty_install() reads gsm_mux[] without holding gsm_mux_lock and
defers mux_get() about 40 lines later. A concurrent gsmld_close() can
free the mux through gsm_cleanup_mux() -> mux_put() -> kfree(gsm) in
that window, leading to a use-after-free when gsmtty_install() later
dereferences the stale pointer.
race condition:
cpu 0 | cpu 1
gsmtty_install() | gsmld_close()
gsm = gsm_mux[mux] // no lock |
// CFS preempts here | gsm_cleanup_mux(gsm)
| gsm->dead = true
| mux_put(gsm)
| -> kfree(gsm)
gsm->dead // UAF! |
mutex_lock(&gsm->mutex) // UAF! |
KASAN report:
BUG: KASAN: slab-use-after-free in gsmtty_install+0x6cf/0x830
Read of size 1 at addr ffff88800fd440ac by task poc/170
CPU: 0 UID: 0 PID: 170 Comm: poc Not tainted 7.0.0-rc7-next-20260410+ #20
Call Trace:
<TASK>
dump_stack_lvl+0x64/0x80
print_report+0xd0/0x5e0
kasan_report+0xce/0x100
gsmtty_install+0x6cf/0x830
tty_init_dev.part.0+0x92/0x4a0
tty_open+0x8ab/0x1050
chrdev_open+0x1ec/0x5e0
do_dentry_open+0x419/0x1260
vfs_open+0x79/0x350
path_openat+0x212c/0x3a70
do_file_open+0x1d2/0x400
do_sys_openat2+0xdc/0x170
__x64_sys_openat+0x122/0x1e0
do_syscall_64+0x64/0x680
entry_SYSCALL_64_after_hwframe+0x76/0x7e
</TASK>
Allocated by task 169 on cpu 0 at 3.824684s:
Freed by task 169 on cpu 0 at 3.892012s:
The buggy address belongs to the object at ffff88800fd44000
which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 172 bytes inside of
freed 1024-byte region [ffff88800fd44000, ffff88800fd44400)
Acquire gsm_mux_lock before reading gsm_mux[] and take the refcount
via kref_get() while still holding the lock, so the mux cannot be
freed between lookup and refcount increment.
Fixes: 86176ed90545 ("TTY: n_gsm, use tty_port_install")
Signed-off-by: Kito Xu (veritas501) <hxzene@gmail.com>
---
drivers/tty/n_gsm.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c13e050de83b..6519f1c92fc5 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -4288,14 +4288,20 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
if (mux >= MAX_MUX)
return -ENXIO;
- /* FIXME: we need to lock gsm_mux for lifetimes of ttys eventually */
- if (gsm_mux[mux] == NULL)
- return -EUNATCH;
if (line == 0 || line > 61) /* 62/63 reserved */
return -ECHRNG;
+
+ /* Acquire gsm_mux_lock to prevent concurrent gsmld_close() from
+ * freeing the mux between reading gsm_mux[] and taking a refcount.
+ */
+ spin_lock(&gsm_mux_lock);
gsm = gsm_mux[mux];
- if (gsm->dead)
- return -EL2HLT;
+ if (!gsm || gsm->dead) {
+ spin_unlock(&gsm_mux_lock);
+ return gsm ? -EL2HLT : -EUNATCH;
+ }
+ kref_get(&gsm->ref);
+ spin_unlock(&gsm_mux_lock);
/* If DLCI 0 is not yet fully open return an error.
This is ok from a locking
perspective as we don't have to worry about this
@@ -4309,8 +4315,10 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
if (dlci0->state == DLCI_OPENING)
wait_event(gsm->event, dlci0->state != DLCI_OPENING);
- if (dlci0->state != DLCI_OPEN)
+ if (dlci0->state != DLCI_OPEN) {
+ mux_put(gsm);
return -EL2NSYNC;
+ }
mutex_lock(&gsm->mutex);
}
@@ -4322,6 +4330,7 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
}
if (dlci == NULL) {
mutex_unlock(&gsm->mutex);
+ mux_put(gsm);
return -ENOMEM;
}
ret = tty_port_install(&dlci->port, driver, tty);
@@ -4329,12 +4338,12 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
if (alloc)
dlci_put(dlci);
mutex_unlock(&gsm->mutex);
+ mux_put(gsm);
return ret;
}
dlci_get(dlci);
dlci_get(gsm->dlci[0]);
- mux_get(gsm);
tty->driver_data = dlci;
mutex_unlock(&gsm->mutex);
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] tty: n_gsm: fix use-after-free in gsmtty_install
2026-04-15 3:17 ` [PATCH] " Kito Xu (veritas501)
@ 2026-04-20 9:26 ` Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2026-04-20 9:26 UTC (permalink / raw)
To: Kito Xu (veritas501); +Cc: jirislaby, linux-serial, linux-kernel
On Wed, Apr 15, 2026 at 11:17:31AM +0800, Kito Xu (veritas501) wrote:
> gsmtty_install() reads gsm_mux[] without holding gsm_mux_lock and
> defers mux_get() about 40 lines later. A concurrent gsmld_close() can
> free the mux through gsm_cleanup_mux() -> mux_put() -> kfree(gsm) in
> that window, leading to a use-after-free when gsmtty_install() later
> dereferences the stale pointer.
>
> race condition:
>
> cpu 0 | cpu 1
> gsmtty_install() | gsmld_close()
> gsm = gsm_mux[mux] // no lock |
> // CFS preempts here | gsm_cleanup_mux(gsm)
> | gsm->dead = true
> | mux_put(gsm)
> | -> kfree(gsm)
> gsm->dead // UAF! |
> mutex_lock(&gsm->mutex) // UAF! |
>
> KASAN report:
>
> BUG: KASAN: slab-use-after-free in gsmtty_install+0x6cf/0x830
> Read of size 1 at addr ffff88800fd440ac by task poc/170
Do you have this hardware to test with, or was this done in a virtual
machine?
>
> CPU: 0 UID: 0 PID: 170 Comm: poc Not tainted 7.0.0-rc7-next-20260410+ #20
> Call Trace:
> <TASK>
> dump_stack_lvl+0x64/0x80
> print_report+0xd0/0x5e0
> kasan_report+0xce/0x100
> gsmtty_install+0x6cf/0x830
> tty_init_dev.part.0+0x92/0x4a0
> tty_open+0x8ab/0x1050
> chrdev_open+0x1ec/0x5e0
> do_dentry_open+0x419/0x1260
> vfs_open+0x79/0x350
> path_openat+0x212c/0x3a70
> do_file_open+0x1d2/0x400
> do_sys_openat2+0xdc/0x170
> __x64_sys_openat+0x122/0x1e0
> do_syscall_64+0x64/0x680
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> </TASK>
>
> Allocated by task 169 on cpu 0 at 3.824684s:
> Freed by task 169 on cpu 0 at 3.892012s:
>
> The buggy address belongs to the object at ffff88800fd44000
> which belongs to the cache kmalloc-1k of size 1024
> The buggy address is located 172 bytes inside of
> freed 1024-byte region [ffff88800fd44000, ffff88800fd44400)
>
> Acquire gsm_mux_lock before reading gsm_mux[] and take the refcount
> via kref_get() while still holding the lock, so the mux cannot be
> freed between lookup and refcount increment.
>
> Fixes: 86176ed90545 ("TTY: n_gsm, use tty_port_install")
> Signed-off-by: Kito Xu (veritas501) <hxzene@gmail.com>
Did you test this change out?
Why was one version sent as "RFC" folllowed by this?
> ---
> drivers/tty/n_gsm.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c13e050de83b..6519f1c92fc5 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -4288,14 +4288,20 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
>
> if (mux >= MAX_MUX)
> return -ENXIO;
> - /* FIXME: we need to lock gsm_mux for lifetimes of ttys eventually */
> - if (gsm_mux[mux] == NULL)
> - return -EUNATCH;
> if (line == 0 || line > 61) /* 62/63 reserved */
> return -ECHRNG;
> +
> + /* Acquire gsm_mux_lock to prevent concurrent gsmld_close() from
> + * freeing the mux between reading gsm_mux[] and taking a refcount.
> + */
> + spin_lock(&gsm_mux_lock);
Wrong comment style :(
Also, how was this tested, last time this was attempted, lockups
happened, hence the FIXME line.
And why isn't this the irqsave version of the lock? Why is that not
needed here? Have you searched the email archives for when this was
attempted in the past? What has now changed?
> gsm = gsm_mux[mux];
> - if (gsm->dead)
> - return -EL2HLT;
> + if (!gsm || gsm->dead) {
> + spin_unlock(&gsm_mux_lock);
> + return gsm ? -EL2HLT : -EUNATCH;
Please spell out inline if statements like this.
> + }
> + kref_get(&gsm->ref);
Shouldn't this be mux_get() somehow? Open-coding it like this is
confusing.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-20 9:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-15 2:48 [RFC] tty: n_gsm: fix use-after-free in gsmtty_install Kito Xu (veritas501)
2026-04-15 3:17 ` [PATCH] " Kito Xu (veritas501)
2026-04-20 9:26 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox