[PATCH 1/1] tty: n_gsm: fix missing modem controls after DLCI open in convergence layer type 2

[PATCH 1/1] tty: n_gsm: fix missing modem controls after DLCI open in convergence layer type 2

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

The current implementation only updates the virtual V.24 control signals
after DLCI open in basic option mode. In advanced option mode with
convergence layer type 2 an empty data frame needs to be transmitted to
inform the peer about the initial virtual V.24 control signals. Not doing
so has two unwanted side effects:
1. hardware flow control status is unclear (initial CTS line is unknown)
2. DLCI user presence is not transmitted (initial DTR line is unknown)

Applications waiting on these lines can not start after a DLCI has been
opened until the first data frame has been transmitted.

Fix this by using gsm_modem_update() thoroughly which handles basic and
advanced option mode correctly. Add a flag in gsm_modem_update() and the
delegated gsm_modem_upd_via_msc() to control whether waiting for the MSC
command response is possible. Remove obsolete gsm_modem_send_initial_msc().

Fixes: 3cf0b3c243e5 ("tty: n_gsm: Don't block input queue by waiting MSC")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 47 ++++++++++++++-------------------------------
 1 file changed, 14 insertions(+), 33 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c13e050de83b..e55779ff0af2 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -454,14 +454,13 @@ static const u8 gsm_fcs8[256] = {
 
 static void gsm_dlci_close(struct gsm_dlci *dlci);
 static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
-static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
+static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk, bool wait);
 static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
 								u8 ctrl);
 static int gsm_send_packet(struct gsm_mux *gsm, struct gsm_msg *msg);
 static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr);
 static void gsmld_write_trigger(struct gsm_mux *gsm);
 static void gsmld_write_task(struct work_struct *work);
-static int gsm_modem_send_initial_msc(struct gsm_dlci *dlci);
 
 /**
  *	gsm_fcs_add	-	update FCS
@@ -2174,7 +2173,7 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
 		pr_debug("DLCI %d goes open.\n", dlci->addr);
 	/* Send current modem state */
 	if (dlci->addr) {
-		gsm_modem_send_initial_msc(dlci);
+		gsm_modem_update(dlci, 0, false);
 	} else {
 		/* Start keep-alive control */
 		gsm->ka_num = 0;
@@ -4136,9 +4135,10 @@ static void gsm_modem_upd_via_data(struct gsm_dlci *dlci, u8 brk)
  *	gsm_modem_upd_via_msc	-	send modem bits via control frame
  *	@dlci: channel
  *	@brk: break signal
+ *	@wait: wait for the MSC command
  */
 
-static int gsm_modem_upd_via_msc(struct gsm_dlci *dlci, u8 brk)
+static int gsm_modem_upd_via_msc(struct gsm_dlci *dlci, u8 brk, bool wait)
 {
 	u8 modembits[3];
 	struct gsm_control *ctrl;
@@ -4155,41 +4155,22 @@ static int gsm_modem_upd_via_msc(struct gsm_dlci *dlci, u8 brk)
 		modembits[2] = (brk << 4) | 2 | EA; /* Length, Break, EA */
 		len++;
 	}
+	if (!wait)
+		return gsm_control_command(dlci->gsm, CMD_MSC, modembits, len);
 	ctrl = gsm_control_send(dlci->gsm, CMD_MSC, modembits, len);
 	if (ctrl == NULL)
 		return -ENOMEM;
 	return gsm_control_wait(dlci->gsm, ctrl);
 }
 
-/**
- * gsm_modem_send_initial_msc - Send initial modem status message
- *
- * @dlci: channel
- *
- * Send an initial MSC message after DLCI open to set the initial
- * modem status lines. This is only done for basic mode.
- * Does not wait for a response as we cannot block the input queue
- * processing.
- */
-static int gsm_modem_send_initial_msc(struct gsm_dlci *dlci)
-{
-	u8 modembits[2];
-
-	if (dlci->adaption != 1 || dlci->gsm->encoding != GSM_BASIC_OPT)
-		return 0;
-
-	modembits[0] = (dlci->addr << 2) | 2 | EA; /* DLCI, Valid, EA */
-	modembits[1] = (gsm_encode_modem(dlci) << 1) | EA;
-	return gsm_control_command(dlci->gsm, CMD_MSC, (const u8 *)&modembits, 2);
-}
-
 /**
  *	gsm_modem_update	-	send modem status line state
  *	@dlci: channel
  *	@brk: break signal
+ *	@wait: wait for the MSC command
  */
 
-static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
+static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk, bool wait)
 {
 	if (dlci->gsm->dead)
 		return -EL2HLT;
@@ -4199,7 +4180,7 @@ static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
 		return 0;
 	} else if (dlci->gsm->encoding == GSM_BASIC_OPT) {
 		/* Send as MSC control message. */
-		return gsm_modem_upd_via_msc(dlci, brk);
+		return gsm_modem_upd_via_msc(dlci, brk, wait);
 	}
 
 	/* Modem status lines are not supported. */
@@ -4265,7 +4246,7 @@ static void gsm_dtr_rts(struct tty_port *port, bool active)
 		modem_tx &= ~(TIOCM_DTR | TIOCM_RTS);
 	if (modem_tx != dlci->modem_tx) {
 		dlci->modem_tx = modem_tx;
-		gsm_modem_update(dlci, 0);
+		gsm_modem_update(dlci, 0, true);
 	}
 }
 
@@ -4471,7 +4452,7 @@ static int gsmtty_tiocmset(struct tty_struct *tty,
 
 	if (modem_tx != dlci->modem_tx) {
 		dlci->modem_tx = modem_tx;
-		return gsm_modem_update(dlci, 0);
+		return gsm_modem_update(dlci, 0, true);
 	}
 	return 0;
 }
@@ -4553,7 +4534,7 @@ static void gsmtty_throttle(struct tty_struct *tty)
 		dlci->modem_tx &= ~TIOCM_RTS;
 	dlci->throttled = true;
 	/* Send an MSC with RTS cleared */
-	gsm_modem_update(dlci, 0);
+	gsm_modem_update(dlci, 0, true);
 }
 
 static void gsmtty_unthrottle(struct tty_struct *tty)
@@ -4565,7 +4546,7 @@ static void gsmtty_unthrottle(struct tty_struct *tty)
 		dlci->modem_tx |= TIOCM_RTS;
 	dlci->throttled = false;
 	/* Send an MSC with RTS set */
-	gsm_modem_update(dlci, 0);
+	gsm_modem_update(dlci, 0, true);
 }
 
 static int gsmtty_break_ctl(struct tty_struct *tty, int state)
@@ -4583,7 +4564,7 @@ static int gsmtty_break_ctl(struct tty_struct *tty, int state)
 		if (encode > 0x0F)
 			encode = 0x0F;	/* Best effort */
 	}
-	return gsm_modem_update(dlci, encode);
+	return gsm_modem_update(dlci, encode, true);
 }
 
 static void gsmtty_cleanup(struct tty_struct *tty)
-- 
2.43.0

Re: [PATCH 1/1] tty: n_gsm: fix missing modem controls after DLCI open in convergence layer type 2

[sashiko-bot]

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

Pre-existing issues:
- [High] Threads waiting in `gsm_control_wait` may hang forever in `TASK_UNINTERRUPTIBLE` if the GSM mux is concurrently shut down, also leading to a memory leak of the control object.
- [High] Lockless read-modify-write of `dlci->modem_tx` leading to lost virtual V.24 control signals and broken hardware flow control.
--

commit 94af07ea4775d3698d9b5b43641a19696873abc0
Author: Daniel Starke <daniel.starke@siemens.com>

tty: n_gsm: fix missing modem controls after DLCI open in convergence layer type 2

This commit updates virtual V.24 control signals by transmitting an empty data
frame in advanced option mode with convergence layer type 2. It introduces a
wait flag to gsm_modem_update() and gsm_modem_upd_via_msc() to control waiting
for the MSC command response. The obsolete gsm_modem_send_initial_msc() is
also removed.

> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c

[ ... ]

> @@ -4155,41 +4155,22 @@ static int gsm_modem_upd_via_msc(struct gsm_dlci *dlci, u8 brk)
>  		modembits[2] = (brk << 4) | 2 | EA; /* Length, Break, EA */
>  		len++;
>  	}
> +	if (!wait)
> +		return gsm_control_command(dlci->gsm, CMD_MSC, modembits, len);
>  	ctrl = gsm_control_send(dlci->gsm, CMD_MSC, modembits, len);
>  	if (ctrl == NULL)
>  		return -ENOMEM;
>  	return gsm_control_wait(dlci->gsm, ctrl);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can threads waiting in
gsm_control_wait() hang forever?

If a thread queues a control command here and blocks in gsm_control_wait(),
and the physical serial port is closed concurrently, gsmld_close() will call
gsm_cleanup_mux().

gsm_cleanup_mux() calls timer_delete_sync(&gsm->t2_timer), which cancels the
retransmit timer without aborting the pending command. Does this mean the wake
condition for wait_event(gsm->event, control->done == 1) will never be met,
leading to an unkillable TASK_UNINTERRUPTIBLE task and leaking the control
object?

[ ... ]

> @@ -4265,7 +4246,7 @@ static void gsm_dtr_rts(struct tty_port *port, bool active)
>  		modem_tx &= ~(TIOCM_DTR | TIOCM_RTS);
>  	if (modem_tx != dlci->modem_tx) {
>  		dlci->modem_tx = modem_tx;
> -		gsm_modem_update(dlci, 0);
> +		gsm_modem_update(dlci, 0, true);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does this lockless read-modify-write of
dlci->modem_tx risk losing virtual V.24 control signals?

If gsmtty_throttle() is called (often from softirq context when receive buffers
fill up) to clear TIOCM_RTS, it updates dlci->modem_tx locklessly.

Concurrently, if a userspace ioctl calls gsmtty_tiocmset(), it also performs
a lockless read-modify-write on dlci->modem_tx.

If these updates overlap, could the throttle state change be overwritten and
lost? This might cause the local system to believe it has throttled the peer,
while the RTS bit remains set, leading to receive buffer overflows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826063026.2472-1-daniel.starke@siemens.com?part=1