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(-)
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;
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;
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);
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; }
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)) {
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;
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 ?
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))) {
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)) {
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);
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; }
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);
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 !=
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)) {
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);
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);
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
-
Greg Kroah-Hartman -
Johan Hovold