[PATCH] tty: n_gsm: pin DLCI during configuration ioctls

[PATCH] tty: n_gsm: pin DLCI during configuration ioctls

[Qing Ming]

The N_GSM line discipline keeps per-channel state in the mux table
gsm->dlci[].  GSMIOC_GETCONF_DLCI and GSMIOC_SETCONF_DLCI fetch a raw
pointer from that table and then read or update the DLCI
configuration, allocating the entry on the spot when the slot is
empty.

The table lookup is done without holding gsm->mutex and without
taking a reference on the DLCI.  A concurrent GSMIOC_SETCONF that
changes the multiplexer configuration calls gsm_cleanup_mux(), which
releases every table entry under gsm->mutex; the final tty_port put
frees the DLCI.  The first ioctl then keeps using the freed object:
the GET path reads it in gsm_dlci_copy_config_values() and the SET
path writes it in gsm_dlci_config().

The issue was found by static analysis of the returned-pointer
lifetime.  A reproducer on a PTY-backed passive mux racing 16
concurrent GSMIOC_GETCONF_DLCI readers against repeated mux restarts
via GSMIOC_SETCONF, with no fault injection or artificial delays,
produced the same KASAN report in 2 of 2 unpatched boots: the first
report landed within half a minute of starting the race, and each
120-second race window averaged 2-3 reports per restart (486 reports
across 172 restarts, then 357 across 162; the copy helper is inlined,
so KASAN names the caller):

  BUG: KASAN: slab-use-after-free in gsmld_ioctl+0x156b/0x17e0
  Read of size 4 at addr ffff88800b84d008 by task n_gsm_dlci_conf/226

  Call Trace:
   gsmld_ioctl
   tty_ioctl
   __x64_sys_ioctl

  Allocated by task 218:
   gsm_dlci_alloc
   gsmld_ioctl

  Freed by task 235:
   kfree
   gsm_cleanup_mux
   gsmld_ioctl

The reported address is 8 bytes inside the freed 2048-byte object
(cache kmalloc-2k), i.e. the read of dlci->addr.

A control run with this patch applied completed 19291547 GET
iterations and 226 restarts with no KASAN report.

Serialize the lookup/allocation against mux cleanup with gsm->mutex
and acquire a tty_port reference before dropping it.  A concurrent
gsm_cleanup_mux() then either completes before the lookup, in which
case the slot is NULL and a fresh DLCI is allocated, or runs after
the reference is taken, in which case its put is not final and the
object survives until the ioctl releases it.

Fixes: afe3154ba87e ("tty: n_gsm: add ioctl for DLC config via ldisc handle")
Cc: stable@vger.kernel.org
Signed-off-by: Qing Ming <a0yami@mailbox.org>
Assisted-by: Claude:kimi-k3
---
 drivers/tty/n_gsm.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c13e050de83b..605549d742e6 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2711,6 +2711,22 @@ static inline void dlci_put(struct gsm_dlci *dlci)
 	tty_port_put(&dlci->port);
 }
 
+static struct gsm_dlci *gsm_dlci_get_or_alloc(struct gsm_mux *gsm,
+					      unsigned int addr)
+{
+	struct gsm_dlci *dlci;
+
+	mutex_lock(&gsm->mutex);
+	dlci = gsm->dlci[addr];
+	if (!dlci)
+		dlci = gsm_dlci_alloc(gsm, addr);
+	if (dlci)
+		dlci_get(dlci);
+	mutex_unlock(&gsm->mutex);
+
+	return dlci;
+}
+
 static void gsm_destroy_network(struct gsm_dlci *dlci);
 
 /**
@@ -3862,29 +3878,29 @@ static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
 		if (dc.channel == 0 || dc.channel >= NUM_DLCI)
 			return -EINVAL;
 		addr = array_index_nospec(dc.channel, NUM_DLCI);
-		dlci = gsm->dlci[addr];
-		if (!dlci) {
-			dlci = gsm_dlci_alloc(gsm, addr);
-			if (!dlci)
-				return -ENOMEM;
-		}
+		dlci = gsm_dlci_get_or_alloc(gsm, addr);
+		if (!dlci)
+			return -ENOMEM;
 		gsm_dlci_copy_config_values(dlci, &dc);
+		dlci_put(dlci);
 		if (copy_to_user((void __user *)arg, &dc, sizeof(dc)))
 			return -EFAULT;
 		return 0;
-	case GSMIOC_SETCONF_DLCI:
+	case GSMIOC_SETCONF_DLCI: {
+		int ret;
+
 		if (copy_from_user(&dc, (void __user *)arg, sizeof(dc)))
 			return -EFAULT;
 		if (dc.channel == 0 || dc.channel >= NUM_DLCI)
 			return -EINVAL;
 		addr = array_index_nospec(dc.channel, NUM_DLCI);
-		dlci = gsm->dlci[addr];
-		if (!dlci) {
-			dlci = gsm_dlci_alloc(gsm, addr);
-			if (!dlci)
-				return -ENOMEM;
-		}
-		return gsm_dlci_config(dlci, &dc, 0);
+		dlci = gsm_dlci_get_or_alloc(gsm, addr);
+		if (!dlci)
+			return -ENOMEM;
+		ret = gsm_dlci_config(dlci, &dc, 0);
+		dlci_put(dlci);
+		return ret;
+	}
 	default:
 		return n_tty_ioctl_helper(tty, cmd, arg);
 	}

base-commit: 45c13f3f9e3bb15fd89ff2864c6f627a3b4b4229
-- 
2.53.0

Re: [PATCH] tty: n_gsm: pin DLCI during configuration ioctls

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The patch introduces a Use-After-Free and refcount underflow because dlci_put() is called outside of gsm->mutex, and gsm_dlci_get_or_alloc() fails to check if the acquired reference is valid.
--

commit 705962eabdbe5c697544b69418bcb84720186a19
Author: Qing Ming <a0yami@mailbox.org>

tty: n_gsm: pin DLCI during configuration ioctls

This patch addresses a lifetime issue in the N_GSM line discipline by
attempting to acquire a DLCI reference during configuration ioctls. It adds
gsm_dlci_get_or_alloc() to retrieve or allocate a DLCI and pin it using
dlci_get() before releasing gsm->mutex, then unpins it with dlci_put() after
use.

> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c13e050de83b1..605549d742e6a 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2711,6 +2711,22 @@ static inline void dlci_put(struct gsm_dlci *dlci)
>  	tty_port_put(&dlci->port);
>  }
>  
> +static struct gsm_dlci *gsm_dlci_get_or_alloc(struct gsm_mux *gsm,
> +					      unsigned int addr)
> +{
> +	struct gsm_dlci *dlci;
> +
> +	mutex_lock(&gsm->mutex);
> +	dlci = gsm->dlci[addr];
> +	if (!dlci)
> +		dlci = gsm_dlci_alloc(gsm, addr);
> +	if (dlci)
> +		dlci_get(dlci);

[Severity: Critical]
Does this code silently ignore reference acquisition failures?

When looking at dlci_get(), it wraps tty_port_get() but discards its return
value:

static inline void dlci_get(struct gsm_dlci *dlci)
{
	tty_port_get(&dlci->port);
}

Since tty_port_get() uses kref_get_unless_zero(), it can fail to increment
the refcount if it is already zero.

Because dlci_get() returns void, gsm_dlci_get_or_alloc() cannot check if
the underlying tty_port_get() failed, which might result in returning a
dying object to the caller.

> +	mutex_unlock(&gsm->mutex);
> +
> +	return dlci;
> +}
> +
[ ... ]
> @@ -3862,29 +3878,29 @@ static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
>  		if (dc.channel == 0 || dc.channel >= NUM_DLCI)
>  			return -EINVAL;
>  		addr = array_index_nospec(dc.channel, NUM_DLCI);
> -		dlci = gsm->dlci[addr];
> -		if (!dlci) {
> -			dlci = gsm_dlci_alloc(gsm, addr);
> -			if (!dlci)
> -				return -ENOMEM;
> -		}
> +		dlci = gsm_dlci_get_or_alloc(gsm, addr);
> +		if (!dlci)
> +			return -ENOMEM;
>  		gsm_dlci_copy_config_values(dlci, &dc);
> +		dlci_put(dlci);

[Severity: Critical]
Is it safe to drop this transient reference outside of gsm->mutex?

If a concurrent GSMIOC_SETCONF or gsm_cleanup_mux() drops the mux's base
reference, this dlci_put() could drop the final reference and trigger the
destructor gsm_dlci_free().

The destructor gsm_dlci_free() runs unprotected by gsm->mutex and eventually
clears the array pointer without synchronization:

static void gsm_dlci_free(struct tty_port *port)
{
	...
	dlci->gsm->dlci[dlci->addr] = NULL;
	...
}

Concurrently, a new ioctl could call gsm_dlci_get_or_alloc(), acquire
gsm->mutex, and observe the non-NULL stale pointer before it is cleared.

It would then call dlci_get(), which fails to increment the zero refcount,
returning a dying DLCI to the caller. The caller would then use the freed
object and subsequently call dlci_put(), triggering a kref underflow.

>  		if (copy_to_user((void __user *)arg, &dc, sizeof(dc)))
>  			return -EFAULT;
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828031949.18580-1-a0yami@mailbox.org?part=1

Re: [PATCH] tty: n_gsm: pin DLCI during configuration ioctls

[Greg Kroah-Hartman]

On Fri, Aug 28, 2026 at 11:19:49AM +0800, Qing Ming wrote:
> Fixes: afe3154ba87e ("tty: n_gsm: add ioctl for DLC config via ldisc handle")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qing Ming <a0yami@mailbox.org>
> Assisted-by: Claude:kimi-k3

Please have your llm read the email archives for why you shouldn't be
touching this file at all, UNLESS you have the real hardware and system
to test it with.

thanks,

greg k-h

Re: [PATCH] tty: n_gsm: pin DLCI during configuration ioctls

[Ming Qing]

> On 08/28/2026 1:50 PM CST Greg Kroah-Hartman wrote:
> Please have your llm read the email archives for why you shouldn't be
> touching this file at all, UNLESS you have the real hardware and system
> to test it with.
> 
> thanks,
> 
> greg k-h

Thanks for the reminder. 

I'll be more careful with fixes like this in the future.

Best regards,
Qing Ming