From 82776f6c75a90e1d2103e689b84a689de8f1aa02 Mon Sep 17 00:00:00 2001 From: Russell King Date: Sun, 18 Oct 2020 09:42:04 +0100 Subject: tty: serial: 21285: fix lockup on open Commit 293f89959483 ("tty: serial: 21285: stop using the unused[] variable from struct uart_port") introduced a bug which stops the transmit interrupt being disabled when there are no characters to transmit - disabling the transmit interrupt at the interrupt controller is the only way to stop an interrupt storm. If this interrupt is not disabled when there are no transmit characters, we end up with an interrupt storm which prevents the machine making forward progress. Fixes: 293f89959483 ("tty: serial: 21285: stop using the unused[] variable from struct uart_port") Signed-off-by: Russell King Cc: stable Link: https://lore.kernel.org/r/E1kU4GS-0006lE-OO@rmk-PC.armlinux.org.uk Signed-off-by: Greg Kroah-Hartman --- drivers/tty/serial/21285.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/tty/serial/21285.c b/drivers/tty/serial/21285.c index 718e010fcb04..09baef4ccc39 100644 --- a/drivers/tty/serial/21285.c +++ b/drivers/tty/serial/21285.c @@ -50,25 +50,25 @@ static const char serial21285_name[] = "Footbridge UART"; static bool is_enabled(struct uart_port *port, int bit) { - unsigned long private_data = (unsigned long)port->private_data; + unsigned long *private_data = (unsigned long *)&port->private_data; - if (test_bit(bit, &private_data)) + if (test_bit(bit, private_data)) return true; return false; } static void enable(struct uart_port *port, int bit) { - unsigned long private_data = (unsigned long)port->private_data; + unsigned long *private_data = (unsigned long *)&port->private_data; - set_bit(bit, &private_data); + set_bit(bit, private_data); } static void disable(struct uart_port *port, int bit) { - unsigned long private_data = (unsigned long)port->private_data; + unsigned long *private_data = (unsigned long *)&port->private_data; - clear_bit(bit, &private_data); + clear_bit(bit, private_data); } #define is_tx_enabled(port) is_enabled(port, tx_enabled_bit) -- cgit v1.2.3 From c97f2a6fb3dfbfbbc88edc8ea62ef2b944e18849 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Fri, 23 Oct 2020 04:34:29 +0300 Subject: tty: serial: fsl_lpuart: LS1021A has a FIFO size of 16 words, like LS1028A MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to the commit that this one fixes, the FIFO size was derived from the read-only register LPUARTx_FIFO[TXFIFOSIZE] using the following formula: TX FIFO size = 2 ^ (LPUARTx_FIFO[TXFIFOSIZE] - 1) The documentation for LS1021A is a mess. Under chapter 26.1.3 LS1021A LPUART module special consideration, it mentions TXFIFO_SZ and RXFIFO_SZ being equal to 4, and in the register description for LPUARTx_FIFO, it shows the out-of-reset value of TXFIFOSIZE and RXFIFOSIZE fields as "011", even though these registers read as "101" in reality. And when LPUART on LS1021A was working, the "101" value did correspond to "16 datawords", by applying the formula above, even though the documentation is wrong again (!!!!) and says that "101" means 64 datawords (hint: it doesn't). So the "new" formula created by commit f77ebb241ce0 has all the premises of being wrong for LS1021A, because it relied only on false data and no actual experimentation. Interestingly, in commit c2f448cff22a ("tty: serial: fsl_lpuart: add LS1028A support"), Michael Walle applied a workaround to this by manually setting the FIFO widths for LS1028A. It looks like the same values are used by LS1021A as well, in fact. When the driver thinks that it has a deeper FIFO than it really has, getty (user space) output gets truncated. Many thanks to Michael for pointing out where to look. Fixes: f77ebb241ce0 ("tty: serial: fsl_lpuart: correct the FIFO depth size") Suggested-by: Michael Walle Signed-off-by: Vladimir Oltean Link: https://lore.kernel.org/r/20201023013429.3551026-1-vladimir.oltean@nxp.com Reviewed-by：Fugang Duan Cc: stable Signed-off-by: Greg Kroah-Hartman --- drivers/tty/serial/fsl_lpuart.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index ff4b88c637d0..bd047e1f9bea 100644 --- a/drivers/tty/serial/fsl_lpuart.c +++ b/drivers/tty/serial/fsl_lpuart.c @@ -314,9 +314,10 @@ MODULE_DEVICE_TABLE(of, lpuart_dt_ids); /* Forward declare this for the dma callbacks*/ static void lpuart_dma_tx_complete(void *arg); -static inline bool is_ls1028a_lpuart(struct lpuart_port *sport) +static inline bool is_layerscape_lpuart(struct lpuart_port *sport) { - return sport->devtype == LS1028A_LPUART; + return (sport->devtype == LS1021A_LPUART || + sport->devtype == LS1028A_LPUART); } static inline bool is_imx8qxp_lpuart(struct lpuart_port *sport) @@ -1701,11 +1702,11 @@ static int lpuart32_startup(struct uart_port *port) UARTFIFO_FIFOSIZE_MASK); /* - * The LS1028A has a fixed length of 16 words. Although it supports the - * RX/TXSIZE fields their encoding is different. Eg the reference manual - * states 0b101 is 16 words. + * The LS1021A and LS1028A have a fixed FIFO depth of 16 words. + * Although they support the RX/TXSIZE fields, their encoding is + * different. Eg the reference manual states 0b101 is 16 words. */ - if (is_ls1028a_lpuart(sport)) { + if (is_layerscape_lpuart(sport)) { sport->rxfifo_size = 16; sport->txfifo_size = 16; sport->port.fifosize = sport->txfifo_size; -- cgit v1.2.3 From 6ca03f90527e499dd5e32d6522909e2ad390896b Mon Sep 17 00:00:00 2001 From: Jiri Slaby Date: Mon, 19 Oct 2020 10:55:16 +0200 Subject: vt: keyboard, simplify vt_kdgkbsent Use 'strlen' of the string, add one for NUL terminator and simply do 'copy_to_user' instead of the explicit 'for' loop. This makes the KDGKBSENT case more compact. The only thing we need to take care about is NULL 'func_table[i]'. Use an empty string in that case. The original check for overflow could never trigger as the func_buf strings are always shorter or equal to 'struct kbsentry's. Cc: Signed-off-by: Jiri Slaby Link: https://lore.kernel.org/r/20201019085517.10176-1-jslaby@suse.cz Signed-off-by: Greg Kroah-Hartman --- drivers/tty/vt/keyboard.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) (limited to 'drivers') diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c index 0db53b5b3acf..7bfa95c3252c 100644 --- a/drivers/tty/vt/keyboard.c +++ b/drivers/tty/vt/keyboard.c @@ -1995,9 +1995,7 @@ out: int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm) { struct kbsentry *kbs; - char *p; u_char *q; - u_char __user *up; int sz, fnw_sz; int delta; char *first_free, *fj, *fnw; @@ -2023,23 +2021,15 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm) i = array_index_nospec(kbs->kb_func, MAX_NR_FUNC); switch (cmd) { - case KDGKBSENT: - sz = sizeof(kbs->kb_string) - 1; /* sz should have been - a struct member */ - up = user_kdgkb->kb_string; - p = func_table[i]; - if(p) - for ( ; *p && sz; p++, sz--) - if (put_user(*p, up++)) { - ret = -EFAULT; - goto reterr; - } - if (put_user('\0', up)) { - ret = -EFAULT; - goto reterr; - } - kfree(kbs); - return ((p && *p) ? -EOVERFLOW : 0); + case KDGKBSENT: { + /* size should have been a struct member */ + unsigned char *from = func_table[i] ? : ""; + + ret = copy_to_user(user_kdgkb->kb_string, from, + strlen(from) + 1) ? -EFAULT : 0; + + goto reterr; + } case KDSKBSENT: if (!perm) { ret = -EPERM; -- cgit v1.2.3 From 82e61c3909db51d91b9d3e2071557b6435018b80 Mon Sep 17 00:00:00 2001 From: Jiri Slaby Date: Mon, 19 Oct 2020 10:55:17 +0200 Subject: vt: keyboard, extend func_buf_lock to readers Both read-side users of func_table/func_buf need locking. Without that, one can easily confuse the code by repeatedly setting altering strings like: while (1) for (a = 0; a < 2; a++) { struct kbsentry kbs = {}; strcpy((char *)kbs.kb_string, a ? ".\n" : "88888\n"); ioctl(fd, KDSKBSENT, &kbs); } When that program runs, one can get unexpected output by holding F1 (note the unxpected period on the last line): . 88888 .8888 So protect all accesses to 'func_table' (and func_buf) by preexisting 'func_buf_lock'. It is easy in 'k_fn' handler as 'puts_queue' is expected not to sleep. On the other hand, KDGKBSENT needs a local (atomic) copy of the string because copy_to_user can sleep. Use already allocated, but unused 'kbs->kb_string' for that purpose. Note that the program above needs at least CAP_SYS_TTY_CONFIG. This depends on the previous patch and on the func_buf_lock lock added in commit 46ca3f735f34 (tty/vt: fix write/write race in ioctl(KDSKBSENT) handler) in 5.2. Likely fixes CVE-2020-25656. Cc: Reported-by: Minh Yuan Signed-off-by: Jiri Slaby Link: https://lore.kernel.org/r/20201019085517.10176-2-jslaby@suse.cz Signed-off-by: Greg Kroah-Hartman --- drivers/tty/vt/keyboard.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c index 7bfa95c3252c..78acc270e39a 100644 --- a/drivers/tty/vt/keyboard.c +++ b/drivers/tty/vt/keyboard.c @@ -743,8 +743,13 @@ static void k_fn(struct vc_data *vc, unsigned char value, char up_flag) return; if ((unsigned)value < ARRAY_SIZE(func_table)) { + unsigned long flags; + + spin_lock_irqsave(&func_buf_lock, flags); if (func_table[value]) puts_queue(vc, func_table[value]); + spin_unlock_irqrestore(&func_buf_lock, flags); + } else pr_err("k_fn called with value=%d\n", value); } @@ -1991,7 +1996,7 @@ out: #undef s #undef v -/* FIXME: This one needs untangling and locking */ +/* FIXME: This one needs untangling */ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm) { struct kbsentry *kbs; @@ -2023,10 +2028,14 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm) switch (cmd) { case KDGKBSENT: { /* size should have been a struct member */ - unsigned char *from = func_table[i] ? : ""; + ssize_t len = sizeof(user_kdgkb->kb_string); + + spin_lock_irqsave(&func_buf_lock, flags); + len = strlcpy(kbs->kb_string, func_table[i] ? : "", len); + spin_unlock_irqrestore(&func_buf_lock, flags); - ret = copy_to_user(user_kdgkb->kb_string, from, - strlen(from) + 1) ? -EFAULT : 0; + ret = copy_to_user(user_kdgkb->kb_string, kbs->kb_string, + len + 1) ? -EFAULT : 0; goto reterr; } -- cgit v1.2.3 From d54654790302ccaa72589380dce060d376ef8716 Mon Sep 17 00:00:00 2001 From: Jiri Slaby Date: Mon, 26 Oct 2020 06:54:19 +0100 Subject: vt_ioctl: fix GIO_UNIMAP regression In commit 5ba127878722, we shuffled with the check of 'perm'. But my brain somehow inverted the condition in 'do_unimap_ioctl' (I thought it is ||, not &&), so GIO_UNIMAP stopped working completely. Move the 'perm' checks back to do_unimap_ioctl and do them right again. In fact, this reverts this part of code to the pre-5ba127878722 state. Except 'perm' is now a bool. Fixes: 5ba127878722 ("vt_ioctl: move perm checks level up") Cc: stable@vger.kernel.org Reported-by: Fabian Vogt Signed-off-by: Jiri Slaby Link: https://lore.kernel.org/r/20201026055419.30518-1-jslaby@suse.cz Signed-off-by: Greg Kroah-Hartman --- drivers/tty/vt/vt_ioctl.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c index 0a33b8ababe3..2321775ef098 100644 --- a/drivers/tty/vt/vt_ioctl.c +++ b/drivers/tty/vt/vt_ioctl.c @@ -549,7 +549,7 @@ static int vt_io_fontreset(struct console_font_op *op) } static inline int do_unimap_ioctl(int cmd, struct unimapdesc __user *user_ud, - struct vc_data *vc) + bool perm, struct vc_data *vc) { struct unimapdesc tmp; @@ -557,9 +557,11 @@ static inline int do_unimap_ioctl(int cmd, struct unimapdesc __user *user_ud, return -EFAULT; switch (cmd) { case PIO_UNIMAP: + if (!perm) + return -EPERM; return con_set_unimap(vc, tmp.entry_ct, tmp.entries); case GIO_UNIMAP: - if (fg_console != vc->vc_num) + if (!perm && fg_console != vc->vc_num) return -EPERM; return con_get_unimap(vc, tmp.entry_ct, &(user_ud->entry_ct), tmp.entries); @@ -639,10 +641,7 @@ static int vt_io_ioctl(struct vc_data *vc, unsigned int cmd, void __user *up, case PIO_UNIMAP: case GIO_UNIMAP: - if (!perm) - return -EPERM; - - return do_unimap_ioctl(cmd, up, vc); + return do_unimap_ioctl(cmd, up, perm, vc); default: return -ENOIOCTLCMD; -- cgit v1.2.3