[PATCH 0/3] tty: fix input-speed handling

[PATCH 0/3] tty: fix input-speed handling

[Johan Hovold]

Turns out we had some long-standing bugs in how we handle termios input
speeds. Specifically, we could end up setting the CIBAUD bits despite
the user leaving them cleared (i.e. B0, which means that we use the same
input and output rate). And once any of these bits were set we failed to
clear them on later updates, leading to incorrect rates being reported
back to user space.

Both issues could lead to an unexpected input rate being set on
subsequent termios updates unless the user actively clears CIBAUD.

Fortunately, no in-tree tty driver seems to use the input speed for
anything but to suppress line-setting updates, so the impact of this
should be mostly limited to the CIBAUD bits sometimes being incorrectly
set in returned termios data.

The final patch cleans up the conditional compilation of the BOTHER and
CIBAUD functionality by not having the latter depend on the former.

Johan


Johan Hovold (3):
  tty: fix termios input-speed encoding
  tty: fix termios input-speed encoding when using BOTHER
  tty: support CIBAUD without BOTHER

 drivers/tty/tty_baudrate.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
2.18.0

[PATCH 1/3] tty: fix termios input-speed encoding

[Johan Hovold]

Make sure to clear the CIBAUD bits before OR-ing the new mask when
encoding the termios input baud rate.

This could otherwise lead to an incorrect input rate being reported back
and incidentally set on subsequent termios updates.

Fixes: edc6afc54968 ("[PATCH] tty: switch to ktermios and new framework")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_baudrate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/tty_baudrate.c b/drivers/tty/tty_baudrate.c
index 6ff8cdfc9d2a..a7a438f54e69 100644
--- a/drivers/tty/tty_baudrate.c
+++ b/drivers/tty/tty_baudrate.c
@@ -169,6 +169,9 @@ void tty_termios_encode_baud_rate(struct ktermios *termios,
 		ibinput = 1;	/* An input speed was specified */
 #endif
 	termios->c_cflag &= ~CBAUD;
+#ifdef IBSHIFT
+	termios->c_cflag &= ~(CBAUD << IBSHIFT);
+#endif
 
 	/*
 	 *	Our goal is to find a close match to the standard baud rate
-- 
2.18.0

[PATCH 2/3] tty: fix termios input-speed encoding when using BOTHER

[Johan Hovold]

When the termios CIBAUD bits are left unset (i.e. B0), we use the same
output and input speed and should leave CIBAUD unchanged.

When the user requests a rate using BOTHER and c_ospeed which the driver
cannot set exactly, the driver can report back the actual baud rate
using tty_termios_encode_baud_rate(). If this rate is close enough to a
standard rate however, we could end up setting CIBAUD to a Bfoo value
despite the user having left it unset.

This in turn could lead to an unexpected input rate being set on
subsequent termios updates.

Fix this by using a zero tolerance value also for the input rate when
CIBAUD is clear so that the matching logic works as expected.

Fixes: 78137e3b34e1 ("[PATCH] tty: improve encode_baud_rate logic")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_baudrate.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_baudrate.c b/drivers/tty/tty_baudrate.c
index a7a438f54e69..3e827a3d48d5 100644
--- a/drivers/tty/tty_baudrate.c
+++ b/drivers/tty/tty_baudrate.c
@@ -157,16 +157,20 @@ void tty_termios_encode_baud_rate(struct ktermios *termios,
 	termios->c_ospeed = obaud;
 
 #ifdef BOTHER
+	if ((termios->c_cflag >> IBSHIFT) & CBAUD)
+		ibinput = 1;	/* An input speed was specified */
+
 	/* If the user asked for a precise weird speed give a precise weird
 	   answer. If they asked for a Bfoo speed they may have problems
 	   digesting non-exact replies so fuzz a bit */
 
-	if ((termios->c_cflag & CBAUD) == BOTHER)
+	if ((termios->c_cflag & CBAUD) == BOTHER) {
 		oclose = 0;
+		if (!ibinput)
+			iclose = 0;
+	}
 	if (((termios->c_cflag >> IBSHIFT) & CBAUD) == BOTHER)
 		iclose = 0;
-	if ((termios->c_cflag >> IBSHIFT) & CBAUD)
-		ibinput = 1;	/* An input speed was specified */
 #endif
 	termios->c_cflag &= ~CBAUD;
 #ifdef IBSHIFT
-- 
2.18.0

[PATCH 3/3] tty: support CIBAUD without BOTHER

[Johan Hovold]

Since commit edc6afc54968 ("[PATCH] tty: switch to ktermios and new
framework") arbitrary baud rates can be requested using BOTHER and input
rates can be requested using the termios CIBAUD bits (CBAUD shifted
IBSHIFT bits).

This functionality has been conditionally compiled depending on whether
an architecture defines BOTHER and IBSHIFT respectively, but would in
fact fail to compile unless both symbols were defined due to cross
dependencies.

Relax the IBSHIFT => BOTHER dependency so that an architecture could
theoretically support CIBAUD without the Linux-specific BOTHER, while
hopefully making the current conditional-compilation directives a bit
less confusing.

Note that the long-term goal is still to have all architectures support
both features, so an alternative could just be to have the lot depend on
BOTHER.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_baudrate.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_baudrate.c b/drivers/tty/tty_baudrate.c
index 3e827a3d48d5..7576ceace571 100644
--- a/drivers/tty/tty_baudrate.c
+++ b/drivers/tty/tty_baudrate.c
@@ -100,11 +100,11 @@ speed_t tty_termios_input_baud_rate(struct ktermios *termios)
 
 	if (cbaud == B0)
 		return tty_termios_baud_rate(termios);
-
+#ifdef BOTHER
 	/* Magic token for arbitrary speed via c_ispeed*/
 	if (cbaud == BOTHER)
 		return termios->c_ispeed;
-
+#endif
 	if (cbaud & CBAUDEX) {
 		cbaud &= ~CBAUDEX;
 
@@ -114,9 +114,9 @@ speed_t tty_termios_input_baud_rate(struct ktermios *termios)
 			cbaud += 15;
 	}
 	return baud_table[cbaud];
-#else
+#else	/* IBSHIFT */
 	return tty_termios_baud_rate(termios);
-#endif
+#endif	/* IBSHIFT */
 }
 EXPORT_SYMBOL(tty_termios_input_baud_rate);
 
@@ -156,10 +156,11 @@ void tty_termios_encode_baud_rate(struct ktermios *termios,
 	termios->c_ispeed = ibaud;
 	termios->c_ospeed = obaud;
 
-#ifdef BOTHER
+#ifdef IBSHIFT
 	if ((termios->c_cflag >> IBSHIFT) & CBAUD)
 		ibinput = 1;	/* An input speed was specified */
-
+#endif
+#ifdef BOTHER
 	/* If the user asked for a precise weird speed give a precise weird
 	   answer. If they asked for a Bfoo speed they may have problems
 	   digesting non-exact replies so fuzz a bit */
-- 
2.18.0

Re: [PATCH 3/3] tty: support CIBAUD without BOTHER

[Greg Kroah-Hartman]

On Sun, Jul 15, 2018 at 03:39:35PM +0200, Johan Hovold wrote:
> Since commit edc6afc54968 ("[PATCH] tty: switch to ktermios and new
> framework") arbitrary baud rates can be requested using BOTHER and input
> rates can be requested using the termios CIBAUD bits (CBAUD shifted
> IBSHIFT bits).
> 
> This functionality has been conditionally compiled depending on whether
> an architecture defines BOTHER and IBSHIFT respectively, but would in
> fact fail to compile unless both symbols were defined due to cross
> dependencies.
> 
> Relax the IBSHIFT => BOTHER dependency so that an architecture could
> theoretically support CIBAUD without the Linux-specific BOTHER, while
> hopefully making the current conditional-compilation directives a bit
> less confusing.
> 
> Note that the long-term goal is still to have all architectures support
> both features, so an alternative could just be to have the lot depend on
> BOTHER.

I thought we had all arches converted to use BOTHER already, what ones
are not yet done?  It's hard to unwind the asm-generic use of termbits.h
to obviously see which ones are not doing this yet, any ideas?

Oh, and thanks for fixing this all up, odd that no one has noticed it
before.

thanks,

greg k-h

Re: [PATCH 3/3] tty: support CIBAUD without BOTHER

[Johan Hovold]

On Mon, Jul 16, 2018 at 12:00:28PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Jul 15, 2018 at 03:39:35PM +0200, Johan Hovold wrote:
> > Since commit edc6afc54968 ("[PATCH] tty: switch to ktermios and new
> > framework") arbitrary baud rates can be requested using BOTHER and input
> > rates can be requested using the termios CIBAUD bits (CBAUD shifted
> > IBSHIFT bits).
> > 
> > This functionality has been conditionally compiled depending on whether
> > an architecture defines BOTHER and IBSHIFT respectively, but would in
> > fact fail to compile unless both symbols were defined due to cross
> > dependencies.
> > 
> > Relax the IBSHIFT => BOTHER dependency so that an architecture could
> > theoretically support CIBAUD without the Linux-specific BOTHER, while
> > hopefully making the current conditional-compilation directives a bit
> > less confusing.
> > 
> > Note that the long-term goal is still to have all architectures support
> > both features, so an alternative could just be to have the lot depend on
> > BOTHER.
> 
> I thought we had all arches converted to use BOTHER already, what ones
> are not yet done?  It's hard to unwind the asm-generic use of termbits.h
> to obviously see which ones are not doing this yet, any ideas?

It looks like alpha does not yet define BOTHER at least.

> Oh, and thanks for fixing this all up, odd that no one has noticed it
> before.

Probably due to there being no in-tree drivers that support separate
input rates. And with no glibc support for BOTHER (still), it's somewhat
less likely that people will trigger the bug that could end up setting
CIBAUD for them.

Thanks,
Johan

Re: [PATCH 3/3] tty: support CIBAUD without BOTHER

[Greg Kroah-Hartman]

On Mon, Jul 16, 2018 at 12:18:24PM +0200, Johan Hovold wrote:
> On Mon, Jul 16, 2018 at 12:00:28PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Jul 15, 2018 at 03:39:35PM +0200, Johan Hovold wrote:
> > > Since commit edc6afc54968 ("[PATCH] tty: switch to ktermios and new
> > > framework") arbitrary baud rates can be requested using BOTHER and input
> > > rates can be requested using the termios CIBAUD bits (CBAUD shifted
> > > IBSHIFT bits).
> > > 
> > > This functionality has been conditionally compiled depending on whether
> > > an architecture defines BOTHER and IBSHIFT respectively, but would in
> > > fact fail to compile unless both symbols were defined due to cross
> > > dependencies.
> > > 
> > > Relax the IBSHIFT => BOTHER dependency so that an architecture could
> > > theoretically support CIBAUD without the Linux-specific BOTHER, while
> > > hopefully making the current conditional-compilation directives a bit
> > > less confusing.
> > > 
> > > Note that the long-term goal is still to have all architectures support
> > > both features, so an alternative could just be to have the lot depend on
> > > BOTHER.
> > 
> > I thought we had all arches converted to use BOTHER already, what ones
> > are not yet done?  It's hard to unwind the asm-generic use of termbits.h
> > to obviously see which ones are not doing this yet, any ideas?
> 
> It looks like alpha does not yet define BOTHER at least.

Someday we will get to delete alpha and many people will be happy :)

> > Oh, and thanks for fixing this all up, odd that no one has noticed it
> > before.
> 
> Probably due to there being no in-tree drivers that support separate
> input rates. And with no glibc support for BOTHER (still), it's somewhat
> less likely that people will trigger the bug that could end up setting
> CIBAUD for them.

Ugh, I thought glibc got support for it, I guess everyone just
hand-codes it in their applications for now.  Sad.

Anyway, thanks for the patches, all now applied.

greg k-h

Re: [PATCH 3/3] tty: support CIBAUD without BOTHER

[Alan Cox]

> Ugh, I thought glibc got support for it, I guess everyone just
> hand-codes it in their applications for now.  Sad.

The glibc people actively contributed to its design and then went radio
silent on the subject.

Alan