linux-kernel - RE: [PATCH 1/1] tty: n_gsm: Fix SW flow control encoding/handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM4PR1001MB137808D63F6C99698FA02DFFE0519@AM4PR1001MB1378.EURPRD10.PROD.OUTLOOK.COM>
Date:   Tue, 11 Jan 2022 11:54:01 +0000
From:   "Starke, Daniel" <daniel.starke@...mens.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1] tty: n_gsm: Fix SW flow control encoding/handling

> > According to 3GPP 27.010 chapter 5.2.7.3 DC1 and DC3 (SW flow control)
> 
> What is all of that?  Do you have a link to the document that this is and where it says this?

n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.2.7.3 states that DC1 and DC3 data bytes
shall be quoted to ensure transparent transmission of these bytes without
setting software flow control. This is partly already the case in the
current n_gsm implementation. This chapter refers also to ISO/IEC 646
regarding the encoding of DC1 and DC3.

> > are to
> > be treated according to ISO/IEC 646.
> 
> What is "ISO/IEC 646"?

ISO/IEC 646 refers to the set of ISO standards described as the ISO 7-bit
coded character set for information interchange. Its final version is also
known as ITU T.50. See https://www.itu.int/rec/T-REC-T.50-199209-I/en

> > That means the MSB shall be ignored.
> 
> "MSB"?  Please spell it out, you have plenty of room here.

MSB stands for "most significant bit" in this context.

> > This patch applies the needed changes to handle this correctly.
> 
> What changes are needed?  Please talk about what you are doing, as the documentation asks you to so do.

To abide the standard it is needed to quote DC1 and DC3 correctly if these
are seen as data bytes and not as control characters. The current code
already tries to enforces this but fails to catch all defined cases.
3GPP 27.010 chapter 5.2.7.3 clearly states that the most significant bit
shall be ignored for DC1 and DC3 handling. The current implementation
handles only the case with the most significant bit was set 0. Cases in
which DC1 and DC3 have the most significant bit set 1 are left unhandled.
This patch fixes this by masking the data bytes with ASCII_MASK (only the
7 least significant bits set 1) before comparing them with XON (a.k.a. DC1)
and XOFF (a.k.a. DC3).

> > 
> > Signed-off-by: Daniel Starke <daniel.starke@...mens.com>
> > ---
> >  drivers/tty/n_gsm.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 
> > 0b96b14bbfe1..9ee0643fc9e2 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -322,6 +322,7 @@ static int addr_cnt;
> >  #define GSM1_ESCAPE_BITS	0x20
> >  #define XON			0x11
> >  #define XOFF			0x13
> > +#define ASCII_MASK		0x7F
> 
> Where did "ASCII" come from?  You didn't say anything about that in the changelog.

The original version (ISO 646 IRV) differed from ASCII only in the code
point for the currency symbol. Therefore, I used ASCII_MASK here to define
the mask for the significant bits. I believe that this is easier to
understand than ISO_IEC_646_MASK for example.

> >  static const struct tty_port_operations gsm_port_ops;
> >  
> > @@ -521,7 +522,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
> >   *	@output: output buffer
> >   *	@len: length of input
> >   *
> > - *	Expand a buffer by bytestuffing it. The worst case size change
> > + *	Expand a buffer by byte stuffing it. The worst case size change
> 
> This change is not described above, and is totally different and belongs in a different change.

You are absolutely right. Shall I create a new patch?

> >   *	is doubling and the caller is responsible for handing out
> >   *	suitable sized buffers.
> >   */
> > @@ -531,7 +532,8 @@ static int gsm_stuff_frame(const u8 *input, u8 *output, int len)
> >  	int olen = 0;
> >  	while (len--) {
> >  		if (*input == GSM1_SOF || *input == GSM1_ESCAPE
> > -		    || *input == XON || *input == XOFF) {
> > +		    || (*input & ASCII_MASK) == XON
> > +		    || (*input & ASCII_MASK) == XOFF) {
> >  			*output++ = GSM1_ESCAPE;
> >  			*output++ = *input++ ^ GSM1_ESCAPE_BITS;
> >  			olen++;
> > --
> > 2.25.1
> > 
> 
> What commit does this fix?

It fixes the initial commit for the n_gsm:
Commit e1eaea46bb40 (tty: n_gsm line discipline, 2010-03-26)
However, this patch is based on the main branch from the TTY/Serial driver
development tree.

With best regards,
Daniel Starke