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

[sashiko-bot@kernel.org]

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