Skip to contentSkip to conversation

tty: TIOCSSERIAL fixes and clean ups

PATCH In review Johan Hovold 2 participants 18 messages Apr 7, 2021

Subsystems (8)
Stable branch Arm i.MX and Layerscape Staging TTY layer and serial drivers +4 more subsystems
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

cover tty: TIOCSSERIAL fixes and clean ups

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(-)
Signature · 1 line
2.26.3
author maintainer
#1 lore raw

PATCH 1/16 staging: fwserial: fix TIOCSSERIAL jiffies conversions

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 [email protected] # 3.8
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/staging/fwserial/fwserial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/fwserial/fwserial.c +2 -2
index c368082aae1a..c963848522b1 100644
@@ -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;
Signature · 1 line
2.26.3
author maintainer
#2 lore raw

PATCH 2/16 staging: fwserial: fix TIOCSSERIAL permission check

In reply to 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 [email protected] # 3.8
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/staging/fwserial/fwserial.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/staging/fwserial/fwserial.c +6 -2
index c963848522b1..440d11423812 100644
@@ -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;
Signature · 1 line
2.26.3
author maintainer
#3 lore raw

PATCH 3/16 staging: fwserial: fix TIOCSSERIAL implementation

In reply to 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 <[email protected]>
---
 drivers/staging/fwserial/fwserial.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/staging/fwserial/fwserial.c +0 -4
index 440d11423812..2888b80a2c1a 100644
@@ -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);
Signature · 1 line
2.26.3
author maintainer
#4 lore raw

PATCH 4/16 staging: fwserial: fix TIOCGSERIAL implementation

In reply to 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 <[email protected]>
---
 drivers/staging/fwserial/fwserial.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/staging/fwserial/fwserial.c +3 -4
index 2888b80a2c1a..0f4655d7d520 100644
@@ -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;
 }
Signature · 1 line
2.26.3
author maintainer
#5 lore raw

PATCH 5/16 staging: greybus: uart: fix TIOCSSERIAL jiffies conversions

In reply to 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 [email protected] # 4.9
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/staging/greybus/uart.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/staging/greybus/uart.c +7 -4
index 607378bfebb7..29846dc1e1bf 100644
@@ -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)) {
Signature · 1 line
2.26.3
author maintainer
#6 lore raw

PATCH 6/16 staging: greybus: uart: fix unprivileged TIOCCSERIAL

In reply to 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 <[email protected]>
---
 drivers/staging/greybus/uart.c | 2 --
 1 file changed, 2 deletions(-)
drivers/staging/greybus/uart.c +0 -2
index 29846dc1e1bf..a520f7f213db 100644
@@ -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;
Signature · 1 line
2.26.3
author maintainer
#7 lore raw

PATCH 7/16 staging: greybus: uart: clean up TIOCGSERIAL

In reply to 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 <[email protected]>
---
 drivers/staging/greybus/uart.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/staging/greybus/uart.c +0 -3
index a520f7f213db..b1e63f7798b0 100644
@@ -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 ?
Signature · 1 line
2.26.3
author maintainer
#8 lore raw

PATCH 8/16 tty: amiserial: fix TIOCSSERIAL permission check

In reply to 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 [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/amiserial.c | 1 +
 1 file changed, 1 insertion(+)
drivers/tty/amiserial.c +1 -0
index 0c8157fab17f..ec6802ba2bf8 100644
@@ -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))) {
Signature · 1 line
2.26.3
author maintainer
#9 lore raw

PATCH 9/16 tty: amiserial: add missing TIOCSSERIAL jiffies conversions

In reply to 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 <[email protected]>
---
 drivers/tty/amiserial.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)
drivers/tty/amiserial.c +19 -7
index ec6802ba2bf8..ca48ce5a0862 100644
@@ -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)) {
Signature · 1 line
2.26.3
author maintainer
#10 lore raw

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

In reply to 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 [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/moxa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/moxa.c +2 -2
index 32eb6b5e510f..5b7bc7af8b1e 100644
@@ -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);
Signature · 1 line
2.26.3
author maintainer
#11 lore raw

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

In reply to 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 [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/moxa.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/tty/moxa.c +11 -5
index 5b7bc7af8b1e..63e440d900ff 100644
@@ -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;
 }
Signature · 1 line
2.26.3
author maintainer
#12 lore raw

PATCH 12/16 tty: moxa: fix TIOCSSERIAL implementation

In reply to 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 <[email protected]>
---
 drivers/tty/moxa.c | 5 -----
 1 file changed, 5 deletions(-)
drivers/tty/moxa.c +0 -5
index 63e440d900ff..4d4f15b5cd29 100644
@@ -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);
Signature · 1 line
2.26.3
author maintainer
#13 lore raw

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

In reply to 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 [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/mxser.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
drivers/tty/mxser.c +18 -6
index 4203b64bccdb..914b23071961 100644
@@ -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 !=
Signature · 1 line
2.26.3
author maintainer
#14 lore raw

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

In reply to 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 [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/mxser.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/tty/mxser.c +4 -3
index 914b23071961..2d8e76263a25 100644
@@ -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)) {
Signature · 1 line
2.26.3
author maintainer
#15 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/char/pcmcia/synclink_cs.c | 2 --
 1 file changed, 2 deletions(-)
drivers/char/pcmcia/synclink_cs.c +0 -2
index 2be8d9a8eec5..3287a7627ed0 100644
@@ -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);
Signature · 1 line
2.26.3
author maintainer
#16 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/tty/synclink_gt.c | 2 --
 1 file changed, 2 deletions(-)
drivers/tty/synclink_gt.c +0 -2
index 1db908f62fde..994618618466 100644
@@ -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);
Signature · 1 line
2.26.3
maintainer
#17 lore raw
In reply to Johan Hovold
Quoted 12 lines · 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).

Quoted 6 lines

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