[PATCH 00/16] tty: TIOCSSERIAL fixes and clean ups

[PATCH 00/16] tty: TIOCSSERIAL fixes and clean ups

[Johan Hovold]

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
serial devices is only useful for setting the close_delay and
closing_wait parameters.

This series fixes up the various ways in which driver authors have
gotten the implementation wrong over the years, like, for example,
jiffies conversions, permissions checks and error handling.

The de-facto standard for error handling is to ignore any unsupported
features and immutable parameters (cf. UPF_FIXED_PORT and deprecated
ASYNC flags).

Permission checking should prevent an unprivileged user from changing
anything but the ASYNC_USR flags (and custom divisor) by returning
-EPERM.

These patches are against tty-next, but the staging ones could otherwise
go through either tree.

I'll be sending the corresponding USB fixes separately.

Johan



Johan Hovold (16):
  staging: fwserial: fix TIOCSSERIAL jiffies conversions
  staging: fwserial: fix TIOCSSERIAL permission check
  staging: fwserial: fix TIOCSSERIAL implementation
  staging: fwserial: fix TIOCGSERIAL implementation
  staging: greybus: uart: fix TIOCSSERIAL jiffies conversions
  staging: greybus: uart: fix unprivileged TIOCCSERIAL
  staging: greybus: uart: clean up TIOCGSERIAL
  tty: amiserial: fix TIOCSSERIAL permission check
  tty: amiserial: add missing TIOCSSERIAL jiffies conversions
  tty: moxa: fix TIOCSSERIAL jiffies conversions
  tty: moxa: fix TIOCSSERIAL permission check
  tty: moxa: fix TIOCSSERIAL implementation
  tty: mxser: fix TIOCSSERIAL jiffies conversions
  tty: mxser: fix TIOCSSERIAL permission check
  pcmcia: synclink_cs: drop redundant tty-port initialisation
  tty: synclink_gt: drop redundant tty-port initialisation

 drivers/char/pcmcia/synclink_cs.c   |  2 --
 drivers/staging/fwserial/fwserial.c | 19 +++++++++---------
 drivers/staging/greybus/uart.c      | 16 +++++++--------
 drivers/tty/amiserial.c             | 25 +++++++++++++++++------
 drivers/tty/moxa.c                  | 21 +++++++++----------
 drivers/tty/mxser.c                 | 31 ++++++++++++++++++++---------
 drivers/tty/synclink_gt.c           |  2 --
 7 files changed, 68 insertions(+), 48 deletions(-)

-- 
2.26.3

[PATCH 01/16] staging: fwserial: fix TIOCSSERIAL jiffies conversions

[Johan Hovold]

The port close_delay parameter set by TIOCSSERIAL is specified in
jiffies, while the value returned by TIOCGSERIAL is specified in
centiseconds.

Add the missing conversions so that TIOCGSERIAL works as expected also
when HZ is not 100.

Fixes: 7355ba3445f2 ("staging: fwserial: Add TTY-over-Firewire serial driver")
Cc: stable@vger.kernel.org      # 3.8
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/fwserial/fwserial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index c368082aae1a..c963848522b1 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -1223,7 +1223,7 @@ static int get_serial_info(struct tty_struct *tty,
 	ss->flags = port->port.flags;
 	ss->xmit_fifo_size = FWTTY_PORT_TXFIFO_LEN;
 	ss->baud_base = 400000000;
-	ss->close_delay = port->port.close_delay;
+	ss->close_delay = jiffies_to_msecs(port->port.close_delay) / 10;
 	mutex_unlock(&port->port.mutex);
 	return 0;
 }
@@ -1245,7 +1245,7 @@ static int set_serial_info(struct tty_struct *tty,
 			return -EPERM;
 		}
 	}
-	port->port.close_delay = ss->close_delay * HZ / 100;
+	port->port.close_delay = msecs_to_jiffies(ss->close_delay * 10);
 	mutex_unlock(&port->port.mutex);
 
 	return 0;
-- 
2.26.3

[PATCH 02/16] staging: fwserial: fix TIOCSSERIAL permission check

[Johan Hovold]

Changing the port close-delay parameter is a privileged operation so
make sure to return -EPERM if a regular user tries to change it.

Fixes: 7355ba3445f2 ("staging: fwserial: Add TTY-over-Firewire serial driver")
Cc: stable@vger.kernel.org      # 3.8
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/fwserial/fwserial.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index c963848522b1..440d11423812 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -1232,20 +1232,24 @@ static int set_serial_info(struct tty_struct *tty,
 			   struct serial_struct *ss)
 {
 	struct fwtty_port *port = tty->driver_data;
+	unsigned int cdelay;
 
 	if (ss->irq != 0 || ss->port != 0 || ss->custom_divisor != 0 ||
 	    ss->baud_base != 400000000)
 		return -EPERM;
 
+	cdelay = msecs_to_jiffies(ss->close_delay * 10);
+
 	mutex_lock(&port->port.mutex);
 	if (!capable(CAP_SYS_ADMIN)) {
-		if (((ss->flags & ~ASYNC_USR_MASK) !=
+		if (cdelay != port->port.close_delay ||
+		    ((ss->flags & ~ASYNC_USR_MASK) !=
 		     (port->port.flags & ~ASYNC_USR_MASK))) {
 			mutex_unlock(&port->port.mutex);
 			return -EPERM;
 		}
 	}
-	port->port.close_delay = msecs_to_jiffies(ss->close_delay * 10);
+	port->port.close_delay = cdelay;
 	mutex_unlock(&port->port.mutex);
 
 	return 0;
-- 
2.26.3

[PATCH 03/16] staging: fwserial: fix TIOCSSERIAL implementation

[Johan Hovold]

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
serial devices is only useful for setting the close_delay and
closing_wait parameters.

A non-privileged user has only ever been able to set the since long
deprecated ASYNC_SPD flags and trying to change any other *supported*
feature should result in -EPERM being returned. Setting the current
values for any supported features should return success.

Fix the fwserial implementation which was returning -EPERM also for a
privileged user when trying to change certain unsupported parameters,
and instead return success consistently.

Fixes: 7355ba3445f2 ("staging: fwserial: Add TTY-over-Firewire serial driver")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/fwserial/fwserial.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index 440d11423812..2888b80a2c1a 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -1234,10 +1234,6 @@ static int set_serial_info(struct tty_struct *tty,
 	struct fwtty_port *port = tty->driver_data;
 	unsigned int cdelay;
 
-	if (ss->irq != 0 || ss->port != 0 || ss->custom_divisor != 0 ||
-	    ss->baud_base != 400000000)
-		return -EPERM;
-
 	cdelay = msecs_to_jiffies(ss->close_delay * 10);
 
 	mutex_lock(&port->port.mutex);
-- 
2.26.3

[PATCH 04/16] staging: fwserial: fix TIOCGSERIAL implementation

[Johan Hovold]

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
serial devices is only useful for setting the close_delay and
closing_wait parameters.

The xmit_fifo_size parameter could be used to set the hardware transmit
fifo size of a legacy UART when it could not be detected, but the
interface is limited to eight bits and should be left unset when not
used.

Fix the fwserial implementation by dropping its custom interpretation of
the unused xmit_fifo_size field, which was overflowed with the driver
FIFO size. Also leave the type and flags fields unset as these cannot be
changed.

The close_delay and closing_wait parameters returned by TIOCGSERIAL are
specified in centiseconds. The driver does not yet support changing
closing_wait, but let's report back the default value actually used (30
seconds).

Fixes: 7355ba3445f2 ("staging: fwserial: Add TTY-over-Firewire serial driver")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/fwserial/fwserial.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index 2888b80a2c1a..0f4655d7d520 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -1218,13 +1218,12 @@ static int get_serial_info(struct tty_struct *tty,
 	struct fwtty_port *port = tty->driver_data;
 
 	mutex_lock(&port->port.mutex);
-	ss->type =  PORT_UNKNOWN;
-	ss->line =  port->port.tty->index;
-	ss->flags = port->port.flags;
-	ss->xmit_fifo_size = FWTTY_PORT_TXFIFO_LEN;
+	ss->line = port->index;
 	ss->baud_base = 400000000;
 	ss->close_delay = jiffies_to_msecs(port->port.close_delay) / 10;
+	ss->closing_wait = 3000;
 	mutex_unlock(&port->port.mutex);
+
 	return 0;
 }
 
-- 
2.26.3

[PATCH 05/16] staging: greybus: uart: fix TIOCSSERIAL jiffies conversions

[Johan Hovold]

The port close_delay and closing_wait parameters set by TIOCSSERIAL are
specified in jiffies and not milliseconds.

Add the missing conversions so that TIOCSSERIAL works as expected also
when HZ is not 1000.

Fixes: e68453ed28c5 ("greybus: uart-gb: now builds, more framework added")
Cc: stable@vger.kernel.org	# 4.9
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/greybus/uart.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index 607378bfebb7..29846dc1e1bf 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -614,10 +614,12 @@ static int get_serial_info(struct tty_struct *tty,
 	ss->line = gb_tty->minor;
 	ss->xmit_fifo_size = 16;
 	ss->baud_base = 9600;
-	ss->close_delay = gb_tty->port.close_delay / 10;
+	ss->close_delay = jiffies_to_msecs(gb_tty->port.close_delay) / 10;
 	ss->closing_wait =
 		gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
-		ASYNC_CLOSING_WAIT_NONE : gb_tty->port.closing_wait / 10;
+		ASYNC_CLOSING_WAIT_NONE :
+		jiffies_to_msecs(gb_tty->port.closing_wait) / 10;
+
 	return 0;
 }
 
@@ -629,9 +631,10 @@ static int set_serial_info(struct tty_struct *tty,
 	unsigned int close_delay;
 	int retval = 0;
 
-	close_delay = ss->close_delay * 10;
+	close_delay = msecs_to_jiffies(ss->close_delay * 10);
 	closing_wait = ss->closing_wait == ASYNC_CLOSING_WAIT_NONE ?
-			ASYNC_CLOSING_WAIT_NONE : ss->closing_wait * 10;
+			ASYNC_CLOSING_WAIT_NONE :
+			msecs_to_jiffies(ss->closing_wait * 10);
 
 	mutex_lock(&gb_tty->port.mutex);
 	if (!capable(CAP_SYS_ADMIN)) {
-- 
2.26.3

[PATCH 06/16] staging: greybus: uart: fix unprivileged TIOCCSERIAL

[Johan Hovold]

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
serial devices is only useful for setting the close_delay and
closing_wait parameters.

A non-privileged user has only ever been able to set the since long
deprecated ASYNC_SPD flags and trying to change any other *supported*
feature should result in -EPERM being returned. Setting the current
values for any supported features should return success.

Fix the greybus implementation which instead indicated that the
TIOCSSERIAL ioctl was not even implemented when a non-privileged user
set the current values.

Fixes: e68453ed28c5 ("greybus: uart-gb: now builds, more framework added")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/greybus/uart.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index 29846dc1e1bf..a520f7f213db 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -641,8 +641,6 @@ static int set_serial_info(struct tty_struct *tty,
 		if ((close_delay != gb_tty->port.close_delay) ||
 		    (closing_wait != gb_tty->port.closing_wait))
 			retval = -EPERM;
-		else
-			retval = -EOPNOTSUPP;
 	} else {
 		gb_tty->port.close_delay = close_delay;
 		gb_tty->port.closing_wait = closing_wait;
-- 
2.26.3

[PATCH 07/16] staging: greybus: uart: clean up TIOCGSERIAL

[Johan Hovold]

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
serial devices is only useful for setting the close_delay and
closing_wait parameters.

The xmit_fifo_size parameter could be used to set the hardware transmit
fifo size of a legacy UART when it could not be detected, but the
interface is limited to eight bits and should be left unset when not
used.

Similarly, baud_base could be used to set the UART base clock when it
could not be detected but might as well be left unset when it is not
known.

The type parameter could be used to set the UART type, but is
better left unspecified (type unknown) when it isn't used.

Note that some applications have historically expected TIOCGSERIAL to be
implemented, but judging from the Debian sources, the port type not
being PORT_UNKNOWN is only used to check for the existence of legacy
serial ports (ttySn). Notably USB serial drivers like ftdi_sio have been
using PORT_UNKNOWN for twenty years without any problems.

Drop the bogus values provided by the greybus implementation.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/staging/greybus/uart.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index a520f7f213db..b1e63f7798b0 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -610,10 +610,7 @@ static int get_serial_info(struct tty_struct *tty,
 {
 	struct gb_tty *gb_tty = tty->driver_data;
 
-	ss->type = PORT_16550A;
 	ss->line = gb_tty->minor;
-	ss->xmit_fifo_size = 16;
-	ss->baud_base = 9600;
 	ss->close_delay = jiffies_to_msecs(gb_tty->port.close_delay) / 10;
 	ss->closing_wait =
 		gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
-- 
2.26.3

[PATCH 08/16] tty: amiserial: fix TIOCSSERIAL permission check

[Johan Hovold]

Changing the port closing_wait parameter is a privileged operation.

Add the missing check to TIOCSSERIAL so that -EPERM is returned in case
an unprivileged user tries to change the closing-wait setting.

Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/amiserial.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 0c8157fab17f..ec6802ba2bf8 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -970,6 +970,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 	if (!serial_isroot()) {
 		if ((ss->baud_base != state->baud_base) ||
 		    (ss->close_delay != port->close_delay) ||
+		    (ss->closing_wait != port->closing_wait) ||
 		    (ss->xmit_fifo_size != state->xmit_fifo_size) ||
 		    ((ss->flags & ~ASYNC_USR_MASK) !=
 		     (port->flags & ~ASYNC_USR_MASK))) {
-- 
2.26.3

[PATCH 09/16] tty: amiserial: add missing TIOCSSERIAL jiffies conversions

[Johan Hovold]

The tty-port close_delay and closing_wait parameters set by TIOCSSERIAL
are specified in jiffies, while the values returned by TIOCGSERIAL are
specified in centiseconds.

Add the missing conversions so that TIOCSSERIAL works as expected also
if this code is ever reused on a system where HZ is not 100.

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

diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index ec6802ba2bf8..ca48ce5a0862 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -937,15 +937,21 @@ static void rs_unthrottle(struct tty_struct * tty)
 static int get_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 {
 	struct serial_state *state = tty->driver_data;
+	unsigned int close_delay, closing_wait;
 
 	tty_lock(tty);
+	close_delay = jiffies_to_msecs(state->tport.close_delay) / 10;
+	closing_wait = state->tport.closing_wait;
+	if (closing_wait != ASYNC_CLOSING_WAIT_NONE)
+		closing_wait = jiffies_to_msecs(closing_wait) / 10;
+
 	ss->line = tty->index;
 	ss->port = state->port;
 	ss->flags = state->tport.flags;
 	ss->xmit_fifo_size = state->xmit_fifo_size;
 	ss->baud_base = state->baud_base;
-	ss->close_delay = state->tport.close_delay;
-	ss->closing_wait = state->tport.closing_wait;
+	ss->close_delay = close_delay;
+	ss->closing_wait = closing_wait;
 	ss->custom_divisor = state->custom_divisor;
 	tty_unlock(tty);
 	return 0;
@@ -957,6 +963,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 	struct tty_port *port = &state->tport;
 	bool change_spd;
 	int 			retval = 0;
+	unsigned int close_delay, closing_wait;
 
 	tty_lock(tty);
 	change_spd = ((ss->flags ^ port->flags) & ASYNC_SPD_MASK) ||
@@ -966,11 +973,16 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 		tty_unlock(tty);
 		return -EINVAL;
 	}
-  
+
+	close_delay = msecs_to_jiffies(ss->close_delay * 10);
+	closing_wait = ss->closing_wait;
+	if (closing_wait != ASYNC_CLOSING_WAIT_NONE)
+		closing_wait = msecs_to_jiffies(closing_wait * 10);
+
 	if (!serial_isroot()) {
 		if ((ss->baud_base != state->baud_base) ||
-		    (ss->close_delay != port->close_delay) ||
-		    (ss->closing_wait != port->closing_wait) ||
+		    (close_delay != port->close_delay) ||
+		    (closing_wait != port->closing_wait) ||
 		    (ss->xmit_fifo_size != state->xmit_fifo_size) ||
 		    ((ss->flags & ~ASYNC_USR_MASK) !=
 		     (port->flags & ~ASYNC_USR_MASK))) {
@@ -997,8 +1009,8 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 	port->flags = ((port->flags & ~ASYNC_FLAGS) |
 			(ss->flags & ASYNC_FLAGS));
 	state->custom_divisor = ss->custom_divisor;
-	port->close_delay = ss->close_delay * HZ/100;
-	port->closing_wait = ss->closing_wait * HZ/100;
+	port->close_delay = close_delay;
+	port->closing_wait = closing_wait;
 
 check_and_exit:
 	if (tty_port_initialized(port)) {
-- 
2.26.3

[PATCH 10/16] tty: moxa: fix TIOCSSERIAL jiffies conversions

[Johan Hovold]

The port close_delay parameter set by TIOCSSERIAL is specified in
jiffies, while the value returned by TIOCGSERIAL is specified in
centiseconds.

Add the missing conversions so that TIOCGSERIAL works as expected also
when HZ is not 100.

Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/moxa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/moxa.c b/drivers/tty/moxa.c
index 32eb6b5e510f..5b7bc7af8b1e 100644
--- a/drivers/tty/moxa.c
+++ b/drivers/tty/moxa.c
@@ -2038,7 +2038,7 @@ static int moxa_get_serial_info(struct tty_struct *tty,
 	ss->line = info->port.tty->index,
 	ss->flags = info->port.flags,
 	ss->baud_base = 921600,
-	ss->close_delay = info->port.close_delay;
+	ss->close_delay = jiffies_to_msecs(info->port.close_delay) / 10;
 	mutex_unlock(&info->port.mutex);
 	return 0;
 }
@@ -2067,7 +2067,7 @@ static int moxa_set_serial_info(struct tty_struct *tty,
 			return -EPERM;
 		}
 	}
-	info->port.close_delay = ss->close_delay * HZ / 100;
+	info->port.close_delay = msecs_to_jiffies(ss->close_delay * 10);
 
 	MoxaSetFifo(info, ss->type == PORT_16550A);
 
-- 
2.26.3

[PATCH 11/16] tty: moxa: fix TIOCSSERIAL permission check

[Johan Hovold]

Changing the port close delay or type are privileged operations so make
sure to return -EPERM if a regular user tries to change them.

Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/moxa.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/moxa.c b/drivers/tty/moxa.c
index 5b7bc7af8b1e..63e440d900ff 100644
--- a/drivers/tty/moxa.c
+++ b/drivers/tty/moxa.c
@@ -2048,6 +2048,7 @@ static int moxa_set_serial_info(struct tty_struct *tty,
 		struct serial_struct *ss)
 {
 	struct moxa_port *info = tty->driver_data;
+	unsigned int close_delay;
 
 	if (tty->index == MAX_PORTS)
 		return -EINVAL;
@@ -2059,19 +2060,24 @@ static int moxa_set_serial_info(struct tty_struct *tty,
 			ss->baud_base != 921600)
 		return -EPERM;
 
+	close_delay = msecs_to_jiffies(ss->close_delay * 10);
+
 	mutex_lock(&info->port.mutex);
 	if (!capable(CAP_SYS_ADMIN)) {
-		if (((ss->flags & ~ASYNC_USR_MASK) !=
+		if (close_delay != info->port.close_delay ||
+		    ss->type != info->type ||
+		    ((ss->flags & ~ASYNC_USR_MASK) !=
 		     (info->port.flags & ~ASYNC_USR_MASK))) {
 			mutex_unlock(&info->port.mutex);
 			return -EPERM;
 		}
-	}
-	info->port.close_delay = msecs_to_jiffies(ss->close_delay * 10);
+	} else {
+		info->port.close_delay = close_delay;
 
-	MoxaSetFifo(info, ss->type == PORT_16550A);
+		MoxaSetFifo(info, ss->type == PORT_16550A);
 
-	info->type = ss->type;
+		info->type = ss->type;
+	}
 	mutex_unlock(&info->port.mutex);
 	return 0;
 }
-- 
2.26.3

[PATCH 12/16] tty: moxa: fix TIOCSSERIAL implementation

[Johan Hovold]

TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
serial devices is only useful for setting the close_delay and
closing_wait parameters.

A non-privileged user has only ever been able to set the since long
deprecated ASYNC_SPD flags and trying to change any other *supported*
feature should result in -EPERM being returned. Setting the current
values for any supported features should return success.

Fix the moxa implementation which was returning -EPERM also for a
privileged user when trying to change certain unsupported parameters and
instead return success consistently.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/moxa.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/tty/moxa.c b/drivers/tty/moxa.c
index 63e440d900ff..4d4f15b5cd29 100644
--- a/drivers/tty/moxa.c
+++ b/drivers/tty/moxa.c
@@ -2055,11 +2055,6 @@ static int moxa_set_serial_info(struct tty_struct *tty,
 	if (!info)
 		return -ENODEV;
 
-	if (ss->irq != 0 || ss->port != 0 ||
-			ss->custom_divisor != 0 ||
-			ss->baud_base != 921600)
-		return -EPERM;
-
 	close_delay = msecs_to_jiffies(ss->close_delay * 10);
 
 	mutex_lock(&info->port.mutex);
-- 
2.26.3

[PATCH 13/16] tty: mxser: fix TIOCSSERIAL jiffies conversions

[Johan Hovold]

The port close_delay and closing wait parameters set by TIOCSSERIAL are
specified in jiffies, while the values returned by TIOCGSERIAL are
specified in centiseconds.

Add the missing conversions so that TIOCSSERIAL works as expected also
when HZ is not 100.

Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/mxser.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 4203b64bccdb..914b23071961 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -1208,19 +1208,26 @@ static int mxser_get_serial_info(struct tty_struct *tty,
 {
 	struct mxser_port *info = tty->driver_data;
 	struct tty_port *port = &info->port;
+	unsigned int closing_wait, close_delay;
 
 	if (tty->index == MXSER_PORTS)
 		return -ENOTTY;
 
 	mutex_lock(&port->mutex);
+
+	close_delay = jiffies_to_msecs(info->port.close_delay) / 10;
+	closing_wait = info->port.closing_wait;
+	if (closing_wait != ASYNC_CLOSING_WAIT_NONE)
+		closing_wait = jiffies_to_msecs(closing_wait) / 10;
+
 	ss->type = info->type,
 	ss->line = tty->index,
 	ss->port = info->ioaddr,
 	ss->irq = info->board->irq,
 	ss->flags = info->port.flags,
 	ss->baud_base = info->baud_base,
-	ss->close_delay = info->port.close_delay,
-	ss->closing_wait = info->port.closing_wait,
+	ss->close_delay = close_delay;
+	ss->closing_wait = closing_wait;
 	ss->custom_divisor = info->custom_divisor,
 	mutex_unlock(&port->mutex);
 	return 0;
@@ -1233,7 +1240,7 @@ static int mxser_set_serial_info(struct tty_struct *tty,
 	struct tty_port *port = &info->port;
 	speed_t baud;
 	unsigned long sl_flags;
-	unsigned int flags;
+	unsigned int flags, close_delay, closing_wait;
 	int retval = 0;
 
 	if (tty->index == MXSER_PORTS)
@@ -1255,9 +1262,14 @@ static int mxser_set_serial_info(struct tty_struct *tty,
 
 	flags = port->flags & ASYNC_SPD_MASK;
 
+	close_delay = msecs_to_jiffies(ss->close_delay * 10);
+	closing_wait = ss->closing_wait;
+	if (closing_wait != ASYNC_CLOSING_WAIT_NONE)
+		closing_wait = msecs_to_jiffies(closing_wait * 10);
+
 	if (!capable(CAP_SYS_ADMIN)) {
 		if ((ss->baud_base != info->baud_base) ||
-				(ss->close_delay != info->port.close_delay) ||
+				(close_delay != info->port.close_delay) ||
 				((ss->flags & ~ASYNC_USR_MASK) != (info->port.flags & ~ASYNC_USR_MASK))) {
 			mutex_unlock(&port->mutex);
 			return -EPERM;
@@ -1271,8 +1283,8 @@ static int mxser_set_serial_info(struct tty_struct *tty,
 		 */
 		port->flags = ((port->flags & ~ASYNC_FLAGS) |
 				(ss->flags & ASYNC_FLAGS));
-		port->close_delay = ss->close_delay * HZ / 100;
-		port->closing_wait = ss->closing_wait * HZ / 100;
+		port->close_delay = close_delay;
+		port->closing_wait = closing_wait;
 		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST &&
 				(ss->baud_base != info->baud_base ||
 				ss->custom_divisor !=
-- 
2.26.3

[PATCH 14/16] tty: mxser: fix TIOCSSERIAL permission check

[Johan Hovold]

Changing the port type and closing_wait parameter are privileged
operations so make sure to return -EPERM if a regular user tries to
change them.

Note that the closing_wait parameter would not actually have been
changed but the return value did not indicate that.

Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/mxser.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 914b23071961..2d8e76263a25 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -1270,6 +1270,7 @@ static int mxser_set_serial_info(struct tty_struct *tty,
 	if (!capable(CAP_SYS_ADMIN)) {
 		if ((ss->baud_base != info->baud_base) ||
 				(close_delay != info->port.close_delay) ||
+				(closing_wait != info->port.closing_wait) ||
 				((ss->flags & ~ASYNC_USR_MASK) != (info->port.flags & ~ASYNC_USR_MASK))) {
 			mutex_unlock(&port->mutex);
 			return -EPERM;
@@ -1296,11 +1297,11 @@ static int mxser_set_serial_info(struct tty_struct *tty,
 			baud = ss->baud_base / ss->custom_divisor;
 			tty_encode_baud_rate(tty, baud, baud);
 		}
-	}
 
-	info->type = ss->type;
+		info->type = ss->type;
 
-	process_txrx_fifo(info);
+		process_txrx_fifo(info);
+	}
 
 	if (tty_port_initialized(port)) {
 		if (flags != (port->flags & ASYNC_SPD_MASK)) {
-- 
2.26.3

[PATCH 15/16] pcmcia: synclink_cs: drop redundant tty-port initialisation

[Johan Hovold]

The port close_delay and closing_wait parameters have already been by
tty_port_init() so drop the redundant driver initialisation to the
default values.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/char/pcmcia/synclink_cs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 2be8d9a8eec5..3287a7627ed0 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -530,8 +530,6 @@ static int mgslpc_probe(struct pcmcia_device *link)
 	info->port.ops = &mgslpc_port_ops;
 	INIT_WORK(&info->task, bh_handler);
 	info->max_frame_size = 4096;
-	info->port.close_delay = 5*HZ/10;
-	info->port.closing_wait = 30*HZ;
 	init_waitqueue_head(&info->status_event_wait_q);
 	init_waitqueue_head(&info->event_wait_q);
 	spin_lock_init(&info->lock);
-- 
2.26.3

[PATCH 16/16] tty: synclink_gt: drop redundant tty-port initialisation

[Johan Hovold]

The port close_delay and closing_wait parameters have already been by
tty_port_init() so drop the redundant driver initialisation to the
default values.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/synclink_gt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 1db908f62fde..994618618466 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -3511,8 +3511,6 @@ static struct slgt_info *alloc_dev(int adapter_num, int port_num, struct pci_dev
 		info->max_frame_size = 4096;
 		info->base_clock = 14745600;
 		info->rbuf_fill_level = DMABUFSIZE;
-		info->port.close_delay = 5*HZ/10;
-		info->port.closing_wait = 30*HZ;
 		init_waitqueue_head(&info->status_event_wait_q);
 		init_waitqueue_head(&info->event_wait_q);
 		spin_lock_init(&info->netlock);
-- 
2.26.3

Re: [PATCH 00/16] tty: TIOCSSERIAL fixes and clean ups

[Greg Kroah-Hartman]

On Wed, Apr 07, 2021 at 12:23:18PM +0200, Johan Hovold wrote:
> TIOCSSERIAL is a horrid, underspecified, legacy interface which for most
> serial devices is only useful for setting the close_delay and
> closing_wait parameters.
> 
> This series fixes up the various ways in which driver authors have
> gotten the implementation wrong over the years, like, for example,
> jiffies conversions, permissions checks and error handling.
> 
> The de-facto standard for error handling is to ignore any unsupported
> features and immutable parameters (cf. UPF_FIXED_PORT and deprecated
> ASYNC flags).
> 
> Permission checking should prevent an unprivileged user from changing
> anything but the ASYNC_USR flags (and custom divisor) by returning
> -EPERM.
> 
> These patches are against tty-next, but the staging ones could otherwise
> go through either tree.

I've taken all of these through my tty tree, thanks for cleaning this
mess up.

greg k-h