[PATCH] tty: n_gsm: keep DLCI 0 object across mux restarts

[PATCH] tty: n_gsm: keep DLCI 0 object across mux restarts

[henrymei]

From: Aohan Mei <henrymei@tencent.com>

gsmtty_install() takes a reference on the object occupying the
gsm->dlci[0] slot, and gsmtty_cleanup() drops a reference on whatever
object occupies the slot at cleanup time. The two are expected to
balance on the same object, but gsm_activate_mux() breaks that: on a
mux restart (GSMIOC_SETCONF with a need_restart change) it
unconditionally allocates a fresh DLCI 0 into the slot, orphaning the
old object which still has install-time references outstanding.

When the last of those old gsmttys is closed, gsmtty_cleanup() then
drops its slot reference on the *new* DLCI 0, driving its base kref
from 1 to 0: gsm_dlci_free() frees it and NULLs the slot while the
mux stays alive (dead == false), and the old object is leaked. The
next gsmtty open passes the dead check and dereferences the NULL slot
in gsmtty_install() (dlci_get(gsm->dlci[0])), crashing the kernel.

Keep the DLCI 0 object in the slot on reactivation when one is still
present: it can only still be there because open gsmttys hold
references on it, and gsm_dlci_free() is the only code that clears the
slot and only runs at kref 0, so the slot identity can no longer
change while install-time references are outstanding. Re-take the base
reference that gsm_cleanup_mux() dropped via gsm_dlci_release(), clear
the dead flag it set, and refresh the per-object config fields exactly
like a fresh gsm_dlci_alloc() would, since the restart may carry a
config change.

This also restores the invariant that dead == false implies
gsm->dlci[0] != NULL, making the NULL dereference structurally
impossible.

Fixes: 6ab8fba7fcb0 ("tty: n_gsm: Added refcount usage to gsm_mux and gsm_dlci structs")
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Cc: stable@vger.kernel.org
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
---
 drivers/tty/n_gsm.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c13e050de83b..2ca7e18addc5 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3186,7 +3186,29 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
 	struct gsm_dlci *dlci;
 	int ret;
 
-	dlci = gsm_dlci_alloc(gsm, 0);
+	dlci = gsm->dlci[0];
+	if (dlci) {
+		/*
+		 * gsmtty_install() takes a reference on the object that
+		 * occupies the dlci[0] slot and gsmtty_cleanup() drops a
+		 * reference on the object occupying the slot at cleanup
+		 * time. Replacing the object here while such references
+		 * are still outstanding makes the cleanup reference drop
+		 * on the wrong (new) object, freeing it prematurely, and
+		 * leaks the old one. Keep the still-referenced object in
+		 * the slot and re-take the base reference that
+		 * gsm_cleanup_mux() dropped so the install and cleanup
+		 * references always balance on the same object.
+		 */
+		dlci_get(dlci);
+		dlci->dead = false;
+		dlci->adaption = gsm->adaption;
+		dlci->mtu = gsm->mtu;
+		dlci->ftype = gsm->ftype;
+		dlci->k = gsm->k;
+	} else {
+		dlci = gsm_dlci_alloc(gsm, 0);
+	}
 	if (dlci == NULL)
 		return -ENOMEM;
 
-- 
2.43.7

Re: [PATCH] tty: n_gsm: keep DLCI 0 object across mux restarts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] A Use-After-Free vulnerability in gsm_activate_mux() due to a Time-of-Check to Time-of-Use (TOCTOU) race with virtual TTY destruction when accessing gsm->dlci[0] locklessly.
- [High] Reference leak of DLCI 0 in gsm_activate_mux() under multiple conditions, such as when the base reference was never initially dropped or on early error paths.

Pre-existing issues:
- [Critical] gsm_cleanup_mux() unconditionally drops references on data DLCIs (dlci[1] to dlci[63]) without clearing their pointers, leading to a double-drop and Use-After-Free across multiple mux restarts.
--

commit d9235f406403e25f640b7a316cd1963f9911d61e
Author: Aohan Mei <henrymei@tencent.com>

    tty: n_gsm: keep DLCI 0 object across mux restarts
    
    gsmtty_install() takes a reference on the object occupying the
    gsm->dlci[0] slot, and gsmtty_cleanup() drops a reference on whatever
    object occupies the slot at cleanup time. The two are expected to
    balance on the same object, but gsm_activate_mux() breaks that: on a
    mux restart (GSMIOC_SETCONF with a need_restart change) it
    unconditionally allocates a fresh DLCI 0 into the slot, orphaning the
    old object which still has install-time references outstanding.

> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c13e050de83b1..2ca7e18addc52 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c

[Severity: Critical]
This is a pre-existing issue, but does gsm_cleanup_mux() contain a double-drop
regression?

Looking at drivers/tty/n_gsm.c, gsm_cleanup_mux() iterates over all dlci
objects and unconditionally drops their references:

drivers/tty/n_gsm.c:gsm_cleanup_mux() {
	...
	for (i = NUM_DLCI - 1; i >= 0; i--)
		if (gsm->dlci[i])
			gsm_dlci_release(gsm->dlci[i]);
	...
}

If a virtual tty is open, its refcount drops by 1 but it is not freed, and
gsm->dlci[i] is not cleared. If a user simply reconfigures the mux twice,
calling GSMIOC_SETCONF a second time, gsm_cleanup_mux() is called again and
drops the reference a second time.

Could this cause the refcount to hit 0 and free the dlci, despite the user
still holding the tty file descriptor? Subsequent operations on the open tty
would then trigger a use-after-free.

> @@ -3186,7 +3186,29 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
>  	struct gsm_dlci *dlci;
>  	int ret;
>  
> -	dlci = gsm_dlci_alloc(gsm, 0);
> +	dlci = gsm->dlci[0];
> +	if (dlci) {

[Severity: Critical]
Does this un-synchronized access to gsm->dlci[0] introduce a time-of-check to
time-of-use (TOCTOU) race?

If gsm_activate_mux() is called via GSMIOC_SETCONF without holding gsm->mutex,
and a virtual tty is concurrently closed, gsmtty_cleanup() drops the
outstanding reference on dlci 0 via dlci_put(gsm->dlci[0]).

If that concurrent dlci_put() drops the reference to 0, the dlci object is
freed. Could gsm_activate_mux() then access the freed memory via dlci_get()
below?

> +		/*
> +		 * gsmtty_install() takes a reference on the object that
> +		 * occupies the dlci[0] slot and gsmtty_cleanup() drops a
> +		 * reference on the object occupying the slot at cleanup
> +		 * time. Replacing the object here while such references
> +		 * are still outstanding makes the cleanup reference drop
> +		 * on the wrong (new) object, freeing it prematurely, and
> +		 * leaks the old one. Keep the still-referenced object in
> +		 * the slot and re-take the base reference that
> +		 * gsm_cleanup_mux() dropped so the install and cleanup
> +		 * references always balance on the same object.
> +		 */
> +		dlci_get(dlci);

[Severity: High]
Can unconditionally acquiring a duplicate base reference here lead to a
resource leak?

If the mux is closed via a modem command (CMD_CLD), gsm->dead is set to true
without calling gsm_cleanup_mux(). A subsequent GSMIOC_SETCONF with identical
parameters skips gsm_cleanup_mux() but calls gsm_activate_mux().

In this scenario, the initial base reference was never dropped, and this
dlci_get() takes a duplicate reference.

Additionally, what happens if gsm_register_devices() fails shortly after this?

drivers/tty/n_gsm.c:gsm_activate_mux() {
	...
	ret = gsm_register_devices(gsm_tty_driver, gsm->num);
	if (ret)
		return ret;
	...
}

The early error path returns without dropping the reference acquired here,
which seems to permanently leak the newly acquired reference.

> +		dlci->dead = false;
> +		dlci->adaption = gsm->adaption;
> +		dlci->mtu = gsm->mtu;
> +		dlci->ftype = gsm->ftype;
> +		dlci->k = gsm->k;
> +	} else {
> +		dlci = gsm_dlci_alloc(gsm, 0);
> +	}
>  	if (dlci == NULL)
>  		return -ENOMEM;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911065826.3515170-1-henrymei@tencent.com?part=1

Re: [PATCH] tty: n_gsm: keep DLCI 0 object across mux restarts

[Greg KH]

On Fri, Sep 11, 2026 at 02:58:26PM +0800, henrymei wrote:
> From: Aohan Mei <henrymei@tencent.com>
> 
> gsmtty_install() takes a reference on the object occupying the
> gsm->dlci[0] slot, and gsmtty_cleanup() drops a reference on whatever
> object occupies the slot at cleanup time. The two are expected to
> balance on the same object, but gsm_activate_mux() breaks that: on a
> mux restart (GSMIOC_SETCONF with a need_restart change) it
> unconditionally allocates a fresh DLCI 0 into the slot, orphaning the
> old object which still has install-time references outstanding.
> 
> When the last of those old gsmttys is closed, gsmtty_cleanup() then
> drops its slot reference on the *new* DLCI 0, driving its base kref
> from 1 to 0: gsm_dlci_free() frees it and NULLs the slot while the
> mux stays alive (dead == false), and the old object is leaked. The
> next gsmtty open passes the dead check and dereferences the NULL slot
> in gsmtty_install() (dlci_get(gsm->dlci[0])), crashing the kernel.
> 
> Keep the DLCI 0 object in the slot on reactivation when one is still
> present: it can only still be there because open gsmttys hold
> references on it, and gsm_dlci_free() is the only code that clears the
> slot and only runs at kref 0, so the slot identity can no longer
> change while install-time references are outstanding. Re-take the base
> reference that gsm_cleanup_mux() dropped via gsm_dlci_release(), clear
> the dead flag it set, and refresh the per-object config fields exactly
> like a fresh gsm_dlci_alloc() would, since the restart may carry a
> config change.
> 
> This also restores the invariant that dead == false implies
> gsm->dlci[0] != NULL, making the NULL dereference structurally
> impossible.
> 
> Fixes: 6ab8fba7fcb0 ("tty: n_gsm: Added refcount usage to gsm_mux and gsm_dlci structs")
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>

As sashiko just told you, this isn't going to work.

ONLY touch this file if you can actually test this code, AND you have a
system that uses it.  Otherwise it's not needed to fix up anything that
a LLM might "find" in this file, see the mailing list archives for the
full details.

thanks,

greg k-h