[RFC] tty: n_gsm: fix use-after-free in gsmtty_install

[RFC] tty: n_gsm: fix use-after-free in gsmtty_install

[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

[PATCH] tty: n_gsm: fix use-after-free in gsmtty_install

[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

Re: [PATCH] tty: n_gsm: fix use-after-free in gsmtty_install

[Greg KH]

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