[PATCH] tty: n_gsm: release the control channel if device registration fails

[PATCH] tty: n_gsm: release the control channel if device registration fails

[Wentao Liang]

gsm_activate_mux() allocates the DLCI 0 control channel before
registering the mux tty devices. If gsm_register_devices() fails, the
function returns without dropping the reference held by gsm->dlci[0].

The mux is left in the dead state, so a later reconfiguration allocates
a new control channel and overwrites gsm->dlci[0], leaking the previous
one together with its tty port and fifo. Release the control channel on
the error path.

Fixes: 734966043860 ("tty: n_gsm: fix resource allocation order in gsm_activate_mux()")
Cc: stable@vger.kernel.org
Assisted-by: DeepSeek-V4.1-Flash and OpenAI Codex
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/tty/n_gsm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e55779ff0af2..8ef2c711b2aa 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3195,8 +3195,10 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
 		gsm->receive = gsm1_receive;
 
 	ret = gsm_register_devices(gsm_tty_driver, gsm->num);
-	if (ret)
+	if (ret) {
+		dlci_put(dlci);
 		return ret;
+	}
 
 	gsm->has_devices = true;
 	gsm->dead = false;		/* Tty opens are now permissible */
-- 
2.34.1

Re: [PATCH] tty: n_gsm: release the control channel if device registration fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: n_gsm: NULL deref and DLCI 0 leak in gsm_activate_mux() on re-activation
- [High] tty: n_gsm: race between dlci_put() in gsm_activate_mux() and gsm_queue()
- [High] tty: n_gsm: concurrent gsm_activate_mux() clears active gsm->dlci[0] to NULL
--

commit 75222737c883df9342677bd8689ca9cd6f7dffbf
Author: Wentao Liang <vulab@iscas.ac.cn>

tty: n_gsm: release the control channel if device registration fails

This commit releases the control channel DLCI 0 via dlci_put() if
gsm_register_devices() fails in gsm_activate_mux().

> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index e55779ff0af2b..8ef2c711b2aa0 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -3195,8 +3195,10 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
>  		gsm->receive = gsm1_receive;
>  
>  	ret = gsm_register_devices(gsm_tty_driver, gsm->num);
> -	if (ret)
> +	if (ret) {
> +		dlci_put(dlci);
>  		return ret;
> +	}

[Severity: High]
Can this cause a NULL pointer dereference in gsmtty_cleanup() and leak the
previous gsm->dlci[0] when re-activating a mux after DLCI 0 closes?

When an active mux has open virtual TTYs (where gsmtty_install() took a
reference via dlci_get(gsm->dlci[0])) and control channel DLCI 0 closes via
an incoming DISC or DM frame, CMD_CLD, T1 retry exhaustion, or keep-alive
timeout, gsm_dlci_close() marks the mux dead without unregistering the tty
devices or releasing gsm->dlci[0]:

drivers/tty/n_gsm.c:gsm_dlci_close() {
	if (dlci->addr != 0) {
		tty_port_tty_hangup(&dlci->port, false);
		...
	} else {
		timer_delete(&dlci->gsm->ka_timer);
		dlci->gsm->dead = true;
	}
}

If userspace then calls GSMIOC_SETCONF with unchanged parameters (so
need_close and need_restart are 0 in gsm_config()) or GSMIOC_SETCONF_EXT
without GSM_FL_RESTART (so need_restart is false in gsm_config_ext()),
gsm_cleanup_mux() is skipped and gsm_activate_mux() is called because
gsm->dead is true:

drivers/tty/n_gsm.c:gsm_config_ext() {
	if (need_restart)
		gsm_cleanup_mux(gsm, true);
	...
	if (gsm->dead) {
		int ret = gsm_activate_mux(gsm);
		...
	}
}

In gsm_activate_mux(), gsm_dlci_alloc(gsm, 0) unconditionally overwrites
gsm->dlci[0] with a new struct gsm_dlci (leaking the old gsm->dlci[0] along
with its tty_port and kfifo), and gsm_register_devices() then fails with
-EEXIST because gsm->has_devices is still true and the minor devices are
still registered.

Calling dlci_put(dlci) on that error path invokes gsm_dlci_free(), which
sets dlci->gsm->dlci[0] = NULL while the existing virtual TTYs remain open:

drivers/tty/n_gsm.c:gsm_dlci_free() {
	timer_shutdown_sync(&dlci->t1);
	dlci->gsm->dlci[dlci->addr] = NULL;
	kfifo_free(&dlci->fifo);
	...
	kfree(dlci);
}

When one of those open virtual TTYs is subsequently closed, gsmtty_cleanup()
dereferences gsm->dlci[0] while it is NULL:

drivers/tty/n_gsm.c:gsmtty_cleanup() {
	dlci_put(dlci);
	dlci_put(gsm->dlci[0]);
	mux_put(gsm);
}

[Severity: High]
What prevents a use-after-free in gsm_queue() if data arrives on the
underlying serial TTY while gsm_register_devices() is running and then fails?

In gsmld_open(), gsm->receive is initialized when the line discipline is
attached, and gsm_activate_mux() also sets gsm->receive before calling
gsm_register_devices(). Meanwhile, gsm_dlci_alloc(gsm, 0) publishes the new
channel to gsm->dlci[0] before returning:

drivers/tty/n_gsm.c:gsm_dlci_alloc() {
	...
	gsm->dlci[addr] = dlci;
	return dlci;
}

While gsm_register_devices() is running, incoming bytes on the underlying
TTY invoke gsmld_receive_buf() -> gsm->receive() -> gsm_queue() on another
CPU without any lock serializing against gsm_activate_mux() and without
checking gsm->dead:

drivers/tty/n_gsm.c:gsm_queue() {
	...
	dlci = gsm->dlci[address];
	...
}

If gsm_register_devices() fails, dlci_put(dlci) -> gsm_dlci_free() clears
gsm->dlci[0] = NULL and frees dlci via kfree(dlci) while gsm_queue() may be
concurrently dereferencing dlci (for example in gsm_dlci_open(),
gsm_dlci_close(), or gsm_dlci_command()). In addition, if an incoming SABM
frame opened DLCI 0 during that window, gsm_dlci_open() may have armed
gsm->ka_timer and scheduled gsm->tx_work, which gsm_dlci_free() does not
cancel, and any subsequent control frame reaching gsm_control_reply() will
dereference gsm->dlci[0] after it has been set to NULL.

[Severity: High]
Could two concurrent GSMIOC_SETCONF or GSMIOC_SETCONF_EXT ioctls race in
gsm_activate_mux() and leave an active mux with gsm->dlci[0] set to NULL?

Neither gsmld_ioctl() nor gsm_config() / gsm_config_ext() holds gsm->mutex
when checking gsm->dead and calling gsm_activate_mux():

drivers/tty/n_gsm.c:gsm_config() {
	...
	if (gsm->dead) {
		int ret = gsm_activate_mux(gsm);
		if (ret)
			return ret;
		if (gsm->initiator)
			gsm_dlci_begin_open(gsm->dlci[0]);
	}
	return 0;
}

If two threads concurrently issue GSMIOC_SETCONF or GSMIOC_SETCONF_EXT on a
dead mux:

Thread 1 enters gsm_activate_mux(), allocates dlci1 (setting
gsm->dlci[0] = dlci1), and sleeps inside gsm_register_devices().

Thread 2 still sees gsm->dead == true, enters gsm_activate_mux(), allocates
dlci2 (overwriting gsm->dlci[0] = dlci2 and leaking dlci1), and calls
gsm_register_devices(), which fails with -EEXIST because Thread 1 already
registered the minor numbers for gsm->num.

Thread 2 then calls dlci_put(dlci2) -> gsm_dlci_free(), which sets
gsm->dlci[0] = NULL, while Thread 1 finishes gsm_register_devices() and sets
gsm->has_devices = true and gsm->dead = false. When a virtual TTY is later
opened on the active mux, gsmtty_install() calls dlci_get(gsm->dlci[0]) with
gsm->dlci[0] == NULL:

drivers/tty/n_gsm.c:gsmtty_install() {
	...
	dlci_get(dlci);
	dlci_get(gsm->dlci[0]);
	mux_get(gsm);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104021509.1468558-1-vulab@iscas.ac.cn?part=1

Re: [PATCH] tty: n_gsm: release the control channel if device registration fails

[Krzysztof Kozlowski]

On Wed, 04 Nov 2026 02:15:09 +0000, Wentao Liang wrote:
> gsm_activate_mux() allocates the DLCI 0 control channel before
> registering the mux tty devices. If gsm_register_devices() fails, the
> function returns without dropping the reference held by gsm->dlci[0].
> 
> The mux is left in the dead state, so a later reconfiguration allocates
> a new control channel and overwrites gsm->dlci[0], leaking the previous
> one together with its tty port and fifo. Release the control channel on
> the error path.
> 
> Fixes: 734966043860 ("tty: n_gsm: fix resource allocation order in gsm_activate_mux()")
> Cc: stable@vger.kernel.org
> Assisted-by: DeepSeek-V4.1-Flash and OpenAI Codex
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/tty/n_gsm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Multiple things here:
1. Your team ignored completely previous feedback. Multiple times.

2. You use multiple identities with this email address, thus I doubt that we
   speak with an actual person.

3. Your bot does not even bother running checkpatch, right?

4. Nothing of this was reproduced nor tested.

5. Finally, the same feedback:
You sent multiple independent patches, to multiple independent
subsystems. The amount of these patches clearly suggest this was
AI generated and most likely not tested.

More importantly, you sent all this work without properly organizing
relevant patches into patchsets. This makes reviewing difficult
and might cause multiple reviewers to address the same issue.
Replying to the entire set is impossible and requires handling each
patch independently, instead of applying or discarding the set.
Maintainers also won't see the bigger picture of your work. Quite
worrying.

This is on the verge of hostile patch: bomb us with so many
contributions, we won't be able to handle them in efficient manner,
like responding ONCE to ask you to slow down.  Considering all this
is untested and LLM generated, I have even more doubts whether this
should be considered for review.

Please read kernel documentation BEFORE posting more work. It will
explain you how to identify subsystems, how to organize your work per
subsystem (so a patchset grouping multiple patches with a short cover
letter), how to document the issue discovery method and what was
or was not tested.

Best regards,
Krzysztof