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

[Greg KH <gregkh@linuxfoundation.org>]

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