[PATCH] tty: n_gsm: bound the EA scan in gsm_control_rls()

[PATCH] tty: n_gsm: bound the EA scan in gsm_control_rls()

[Michael Bommarito]

gsm_control_rls() walks the control message looking for the end of the
EA-encoded address, but it dereferences each byte before checking the
remaining length:

	int len = clen;
	const u8 *dp = data;

	while (gsm_read_ea(&addr, *dp++) == 0) {
		len--;
		if (len == 0)
			return;
	}

When a malformed RLS control message decodes to a control length of
zero, len starts at 0, the loop reads *dp before testing len, len
underflows to -1, and the "if (len == 0)" test never fires. The scan
then keeps reading forward through gsm->buf (kmalloc(MAX_MRU + 1)) until
it happens upon a byte whose EA bit is set, reading past the end of the
allocation.

A two byte DLCI 0 UIH control frame carrying CMD_RLS with an encoded
length of zero reaches this from the receive path. KASAN reports a
slab-out-of-bounds read in gsm_control_rls() (inlined into
gsm_dlci_command()) when the bytes that follow the message in gsm->buf
do not terminate the scan before the end of the buffer.

The sibling control handlers gsm_control_modem() and gsm_dlci_data()
already reject a too short message before walking it; gsm_control_rls()
was missed. Bound the EA scan by the remaining length so a zero length
message returns without dereferencing past the data.

Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/tty/n_gsm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c13e050de83b1..4f8f2ce038bc8 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1806,12 +1806,12 @@ static void gsm_control_rls(struct gsm_mux *gsm, const u8 *data, int clen)
 	int len = clen;
 	const u8 *dp = data;
 
-	while (gsm_read_ea(&addr, *dp++) == 0) {
+	while (len > 0) {
+		if (gsm_read_ea(&addr, *dp++))
+			break;
 		len--;
-		if (len == 0)
-			return;
 	}
-	/* Must be at least one byte following ea */
+	/* Must be at least one byte following the EA */
 	len--;
 	if (len <= 0)
 		return;

base-commit: 5200f5f493f79f14bbdc349e402a40dfb32f23c8
-- 
2.53.0

Re: [PATCH] tty: n_gsm: bound the EA scan in gsm_control_rls()

[Greg Kroah-Hartman]

On Sat, Jun 06, 2026 at 01:03:23PM -0400, Michael Bommarito wrote:
> gsm_control_rls() walks the control message looking for the end of the
> EA-encoded address, but it dereferences each byte before checking the
> remaining length:
> 
> 	int len = clen;
> 	const u8 *dp = data;
> 
> 	while (gsm_read_ea(&addr, *dp++) == 0) {
> 		len--;
> 		if (len == 0)
> 			return;
> 	}
> 
> When a malformed RLS control message decodes to a control length of
> zero, len starts at 0, the loop reads *dp before testing len, len
> underflows to -1, and the "if (len == 0)" test never fires. The scan
> then keeps reading forward through gsm->buf (kmalloc(MAX_MRU + 1)) until
> it happens upon a byte whose EA bit is set, reading past the end of the
> allocation.
> 
> A two byte DLCI 0 UIH control frame carrying CMD_RLS with an encoded
> length of zero reaches this from the receive path. KASAN reports a
> slab-out-of-bounds read in gsm_control_rls() (inlined into
> gsm_dlci_command()) when the bytes that follow the message in gsm->buf
> do not terminate the scan before the end of the buffer.
> 
> The sibling control handlers gsm_control_modem() and gsm_dlci_data()
> already reject a too short message before walking it; gsm_control_rls()
> was missed. Bound the EA scan by the remaining length so a zero length
> message returns without dereferencing past the data.
> 
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>  drivers/tty/n_gsm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c13e050de83b1..4f8f2ce038bc8 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1806,12 +1806,12 @@ static void gsm_control_rls(struct gsm_mux *gsm, const u8 *data, int clen)
>  	int len = clen;
>  	const u8 *dp = data;
>  
> -	while (gsm_read_ea(&addr, *dp++) == 0) {
> +	while (len > 0) {
> +		if (gsm_read_ea(&addr, *dp++))
> +			break;
>  		len--;
> -		if (len == 0)
> -			return;
>  	}
> -	/* Must be at least one byte following ea */
> +	/* Must be at least one byte following the EA */
>  	len--;
>  	if (len <= 0)
>  		return;
> 
> base-commit: 5200f5f493f79f14bbdc349e402a40dfb32f23c8

While it's fun to throw LLMs at this codebase, how was this actually
tested?  there are loads of documented "problems" with this file, but in
the real world, with real devices, it works fine so without testing of
the code with those devices, we have to be careful with changing
anything here.

thanks,

greg k-h

Re: [PATCH] tty: n_gsm: bound the EA scan in gsm_control_rls()

[Michael Bommarito]

On Sun, Jun 7, 2026 at 2:38 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> While it's fun to throw LLMs at this codebase, how was this actually
> tested?  there are loads of documented "problems" with this file, but in
> the real world, with real devices, it works fine so without testing of
> the code with those devices, we have to be careful with changing
> anything here.

Understood.  A few different notes for you to consider on this one:

1. This is almost certainly the continuation / completion of
669609cea1d29, where two other sites were similarly addressed.  I
think Daniel could answer if there was a reason why he didn't include
this spot, but there's no evidence in the commit log or code comments
otherwise.


2. The backport situation on 67c37756898a5 (the CAP_NET_ADMIN gate)
looks complicated because it wasn't originally stable tagged and
didn't get actioned by vendors for awhile, so I think reachability for
unprivileged chaining in attacks is still high.


3. I'm normally running tests on UML or qemu for every patch I submit.
Sometimes, the AI assistance tag is more about the testing side of the
patch than anything else.

For example, for this patch, here's what Claude logged for the
regression testing:

<CLAUDE>
  repro/loopback.c stands up two real N_GSM0710 line disciplines in
one UML kernel, wired back to back through a userspace byte relay — so
the kernel runs both the
  initiator and responder TS 27.010 state machines (not a
userspace-faked peer like trigger.c). Phases:

  - A — DLCI0 + DLCI1 SABM/UA handshake (kernel both ends)
  - B — bidirectional gsmtty data, byte-exact (PING/PONG)
  - C — drop/raise DTR/RTS → real MSC frames → peer modem lines track
them (before=0x0 low=0x40 high=0x166)
  - D — inject a valid CMD_RLS (clen=2) on DLCI0 → accepted by the
patched gsm_control_rls, mux keeps carrying data
  - E — clean teardown
</CLAUDE>


4.  Relatedly, I think it would be helpful for someone (Willy again?)
to improve the guidance for AI assisted testing and establish some
standards for documentation.  Because most of the kernel doesn't have
established KUnit setups and dumping all this info is frowned upon in
commit logs or notes, it seems like there is a tension between
"showing your work" and sticking to general convention.


P.S. I know it's somewhat contentious, but maybe one of the best use
cases for AI would be helping maintainers add or improve KUnit or
out-of-kernel test frameworks.  I have dozens of orphaned tests or
harnesses across the kernel now and would be happy to contribute them
or use my tokens for the cause.

Thanks,
Mike

Re: [PATCH] tty: n_gsm: bound the EA scan in gsm_control_rls()

[Greg Kroah-Hartman]

On Sun, Jun 07, 2026 at 07:09:06AM -0400, Michael Bommarito wrote:
> On Sun, Jun 7, 2026 at 2:38 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > While it's fun to throw LLMs at this codebase, how was this actually
> > tested?  there are loads of documented "problems" with this file, but in
> > the real world, with real devices, it works fine so without testing of
> > the code with those devices, we have to be careful with changing
> > anything here.
> 
> Understood.  A few different notes for you to consider on this one:
> 
> 1. This is almost certainly the continuation / completion of
> 669609cea1d29, where two other sites were similarly addressed.  I
> think Daniel could answer if there was a reason why he didn't include
> this spot, but there's no evidence in the commit log or code comments
> otherwise.

It was perhaps missed, or not actually needed?  Again, how was this
tested?

> 2. The backport situation on 67c37756898a5 (the CAP_NET_ADMIN gate)
> looks complicated because it wasn't originally stable tagged and
> didn't get actioned by vendors for awhile, so I think reachability for
> unprivileged chaining in attacks is still high.

Distros/vendors know all about this line dicipline and they enable it at
their own risk.  There should not be any that has it enabled for normal
users, and I would not enabling it at all except for systems that
actually has this hardware.

> 3. I'm normally running tests on UML or qemu for every patch I submit.
> Sometimes, the AI assistance tag is more about the testing side of the
> patch than anything else.

That's fine, but where is the test?

> For example, for this patch, here's what Claude logged for the
> regression testing:
> 
> <CLAUDE>
>   repro/loopback.c stands up two real N_GSM0710 line disciplines in
> one UML kernel, wired back to back through a userspace byte relay — so
> the kernel runs both the
>   initiator and responder TS 27.010 state machines (not a
> userspace-faked peer like trigger.c). Phases:
> 
>   - A — DLCI0 + DLCI1 SABM/UA handshake (kernel both ends)
>   - B — bidirectional gsmtty data, byte-exact (PING/PONG)
>   - C — drop/raise DTR/RTS → real MSC frames → peer modem lines track
> them (before=0x0 low=0x40 high=0x166)
>   - D — inject a valid CMD_RLS (clen=2) on DLCI0 → accepted by the
> patched gsm_control_rls, mux keeps carrying data
>   - E — clean teardown
> </CLAUDE>

Adding tests like this to the tree would be great.

> 4.  Relatedly, I think it would be helpful for someone (Willy again?)
> to improve the guidance for AI assisted testing and establish some
> standards for documentation.

Why not submit it yourself?

> Because most of the kernel doesn't have
> established KUnit setups and dumping all this info is frowned upon in
> commit logs or notes, it seems like there is a tension between
> "showing your work" and sticking to general convention.

Add the test!  We'll gladly take them, that would make it even easier to
prove changes work, AND prevent regressions in the future.

thanks,

greg k-h