[PATCH 5.10.y and below 1/2] tty: Prevent writing chars during tcsetattr TCSADRAIN/FLUSH
If userspace races tcsetattr() with a write, the drained condition might not be guaranteed by the kernel. There is a race window after checking Tx is empty before tty_set_termios() takes termios_rwsem for write. During that race window, more characters can be queued by a racing writer.
Any ongoing transmission might produce garbage during HW's ->set_termios() call. The intent of TCSADRAIN/FLUSH seems to be preventing such a character corruption. If those flags are set, take tty's write lock to stop any writer before performing the lower layer Tx empty check and wait for the pending characters to be sent (if any).
The initial wait for all-writers-done must be placed outside of tty's write lock to avoid deadlock which makes it impossible to use tty_wait_until_sent(). The write lock is retried if a racing write is detected.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Link: https://lore.kernel.org/r/20230317113318.31327-2-ilpo.jarvinen@linux.intel.c... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org (cherry picked from commit 094fb49a2d0d6827c86d2e0840873e6db0c491d2) Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- drivers/tty/tty_io.c | 4 ++-- drivers/tty/tty_ioctl.c | 45 ++++++++++++++++++++++++++++++----------- include/linux/tty.h | 2 ++ 3 files changed, 37 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index c37d2657308c..8cc45f2b3a17 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -941,13 +941,13 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to) return i; }
-static void tty_write_unlock(struct tty_struct *tty) +void tty_write_unlock(struct tty_struct *tty) { mutex_unlock(&tty->atomic_write_lock); wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT); }
-static int tty_write_lock(struct tty_struct *tty, int ndelay) +int tty_write_lock(struct tty_struct *tty, int ndelay) { if (!mutex_trylock(&tty->atomic_write_lock)) { if (ndelay) diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c index 803da2d111c8..fa81ff535f92 100644 --- a/drivers/tty/tty_ioctl.c +++ b/drivers/tty/tty_ioctl.c @@ -397,21 +397,42 @@ static int set_termios(struct tty_struct *tty, void __user *arg, int opt) tmp_termios.c_ispeed = tty_termios_input_baud_rate(&tmp_termios); tmp_termios.c_ospeed = tty_termios_baud_rate(&tmp_termios);
- ld = tty_ldisc_ref(tty); + if (opt & (TERMIOS_FLUSH|TERMIOS_WAIT)) { +retry_write_wait: + retval = wait_event_interruptible(tty->write_wait, !tty_chars_in_buffer(tty)); + if (retval < 0) + return retval;
- if (ld != NULL) { - if ((opt & TERMIOS_FLUSH) && ld->ops->flush_buffer) - ld->ops->flush_buffer(tty); - tty_ldisc_deref(ld); - } + if (tty_write_lock(tty, 0) < 0) + goto retry_write_wait;
- if (opt & TERMIOS_WAIT) { - tty_wait_until_sent(tty, 0); - if (signal_pending(current)) - return -ERESTARTSYS; - } + /* Racing writer? */ + if (tty_chars_in_buffer(tty)) { + tty_write_unlock(tty); + goto retry_write_wait; + }
- tty_set_termios(tty, &tmp_termios); + ld = tty_ldisc_ref(tty); + if (ld != NULL) { + if ((opt & TERMIOS_FLUSH) && ld->ops->flush_buffer) + ld->ops->flush_buffer(tty); + tty_ldisc_deref(ld); + } + + if ((opt & TERMIOS_WAIT) && tty->ops->wait_until_sent) { + tty->ops->wait_until_sent(tty, 0); + if (signal_pending(current)) { + tty_write_unlock(tty); + return -ERESTARTSYS; + } + } + + tty_set_termios(tty, &tmp_termios); + + tty_write_unlock(tty); + } else { + tty_set_termios(tty, &tmp_termios); + }
/* FIXME: Arguably if tmp_termios == tty->termios AND the actual requested termios was not tmp_termios then we may diff --git a/include/linux/tty.h b/include/linux/tty.h index 5972f43b9d5a..ba33dd7c0847 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -480,6 +480,8 @@ extern void __stop_tty(struct tty_struct *tty); extern void stop_tty(struct tty_struct *tty); extern void __start_tty(struct tty_struct *tty); extern void start_tty(struct tty_struct *tty); +void tty_write_unlock(struct tty_struct *tty); +int tty_write_lock(struct tty_struct *tty, int ndelay); extern int tty_register_driver(struct tty_driver *driver); extern int tty_unregister_driver(struct tty_driver *driver); extern struct device *tty_register_device(struct tty_driver *driver,
There's a potential race before THRE/TEMT deasserts when DMA Tx is starting up (or the next batch of continuous Tx is being submitted). This can lead to misdetecting Tx empty condition.
It is entirely normal for THRE/TEMT to be set for some time after the DMA Tx had been setup in serial8250_tx_dma(). As Tx side is definitely not empty at that point, it seems incorrect for serial8250_tx_empty() claim Tx is empty.
Fix the race by also checking in serial8250_tx_empty() whether there's DMA Tx active.
Note: This fix only addresses in-kernel race mainly to make using TCSADRAIN/FLUSH robust. Userspace can still cause other races but they seem userspace concurrency control problems.
Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Link: https://lore.kernel.org/r/20230317113318.31327-3-ilpo.jarvinen@linux.intel.c... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org (cherry picked from commit 146a37e05d620cef4ad430e5d1c9c077fe6fa76f) Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- drivers/tty/serial/8250/8250.h | 12 ++++++++++++ drivers/tty/serial/8250/8250_port.c | 12 +++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h index b6dc9003b8c4..0771cd226581 100644 --- a/drivers/tty/serial/8250/8250.h +++ b/drivers/tty/serial/8250/8250.h @@ -330,6 +330,13 @@ extern int serial8250_rx_dma(struct uart_8250_port *); extern void serial8250_rx_dma_flush(struct uart_8250_port *); extern int serial8250_request_dma(struct uart_8250_port *); extern void serial8250_release_dma(struct uart_8250_port *); + +static inline bool serial8250_tx_dma_running(struct uart_8250_port *p) +{ + struct uart_8250_dma *dma = p->dma; + + return dma && dma->tx_running; +} #else static inline int serial8250_tx_dma(struct uart_8250_port *p) { @@ -345,6 +352,11 @@ static inline int serial8250_request_dma(struct uart_8250_port *p) return -1; } static inline void serial8250_release_dma(struct uart_8250_port *p) { } + +static inline bool serial8250_tx_dma_running(struct uart_8250_port *p) +{ + return false; +} #endif
static inline int ns16550a_goto_highspeed(struct uart_8250_port *up) diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 1f231fcda657..a12682a7012f 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -1967,19 +1967,25 @@ static int serial8250_tx_threshold_handle_irq(struct uart_port *port) static unsigned int serial8250_tx_empty(struct uart_port *port) { struct uart_8250_port *up = up_to_u8250p(port); + unsigned int result = 0; unsigned long flags; unsigned int lsr;
serial8250_rpm_get(up);
spin_lock_irqsave(&port->lock, flags); - lsr = serial_port_in(port, UART_LSR); - up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; + if (!serial8250_tx_dma_running(up)) { + lsr = serial_port_in(port, UART_LSR); + up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; + + if ((lsr & BOTH_EMPTY) == BOTH_EMPTY) + result = TIOCSER_TEMT; + } spin_unlock_irqrestore(&port->lock, flags);
serial8250_rpm_put(up);
- return (lsr & BOTH_EMPTY) == BOTH_EMPTY ? TIOCSER_TEMT : 0; + return result; }
unsigned int serial8250_do_get_mctrl(struct uart_port *port)
On Thu, May 11, 2023 at 03:32:43PM +0300, Ilpo Järvinen wrote:
If userspace races tcsetattr() with a write, the drained condition might not be guaranteed by the kernel. There is a race window after checking Tx is empty before tty_set_termios() takes termios_rwsem for write. During that race window, more characters can be queued by a racing writer.
Any ongoing transmission might produce garbage during HW's ->set_termios() call. The intent of TCSADRAIN/FLUSH seems to be preventing such a character corruption. If those flags are set, take tty's write lock to stop any writer before performing the lower layer Tx empty check and wait for the pending characters to be sent (if any).
The initial wait for all-writers-done must be placed outside of tty's write lock to avoid deadlock which makes it impossible to use tty_wait_until_sent(). The write lock is retried if a racing write is detected.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Link: https://lore.kernel.org/r/20230317113318.31327-2-ilpo.jarvinen@linux.intel.c... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org (cherry picked from commit 094fb49a2d0d6827c86d2e0840873e6db0c491d2) Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
drivers/tty/tty_io.c | 4 ++-- drivers/tty/tty_ioctl.c | 45 ++++++++++++++++++++++++++++++----------- include/linux/tty.h | 2 ++ 3 files changed, 37 insertions(+), 14 deletions(-)
Didn't apply to 4.14.y :(
But all others it did, so I've queued it up now there, thanks!
greg k-h
On Mon, May 15, 2023 at 02:24:32PM +0200, Greg Kroah-Hartman wrote:
On Thu, May 11, 2023 at 03:32:43PM +0300, Ilpo Järvinen wrote:
If userspace races tcsetattr() with a write, the drained condition might not be guaranteed by the kernel. There is a race window after checking Tx is empty before tty_set_termios() takes termios_rwsem for write. During that race window, more characters can be queued by a racing writer.
Any ongoing transmission might produce garbage during HW's ->set_termios() call. The intent of TCSADRAIN/FLUSH seems to be preventing such a character corruption. If those flags are set, take tty's write lock to stop any writer before performing the lower layer Tx empty check and wait for the pending characters to be sent (if any).
The initial wait for all-writers-done must be placed outside of tty's write lock to avoid deadlock which makes it impossible to use tty_wait_until_sent(). The write lock is retried if a racing write is detected.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Link: https://lore.kernel.org/r/20230317113318.31327-2-ilpo.jarvinen@linux.intel.c... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org (cherry picked from commit 094fb49a2d0d6827c86d2e0840873e6db0c491d2) Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
drivers/tty/tty_io.c | 4 ++-- drivers/tty/tty_ioctl.c | 45 ++++++++++++++++++++++++++++++----------- include/linux/tty.h | 2 ++ 3 files changed, 37 insertions(+), 14 deletions(-)
Didn't apply to 4.14.y :(
But all others it did, so I've queued it up now there, thanks!
Oh nevermind, I fixed it up, it wasn't hard, now queued up everywhere, thanks!
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Ilpo Järvinen