uart: fix race between uart_put_char() and uart_shutdown() (#67)

Signed-off-by: Guohan Lu <[email protected]>

patch/driver-uart-fix-another-race-between-uart_put_char-and-uart_shutdow.patch

@@ -0,0 +1,28 @@
+uart: fix another race between uart_put_char() and uart_shutdown()
+
+From: Guohan Lu <[email protected]>
+
+this is complete the fix in a5ba1d95e46ecaea638ddd7cd144107c783acb5d
+---
+ drivers/tty/serial/serial_core.c |    6 ++++--
+ 1 file changed, 4 insertions(+), 2 deletions(-)
+
+diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
+index 53e6db8..ec1187d 100644
+--- a/drivers/tty/serial/serial_core.c
++++ b/drivers/tty/serial/serial_core.c
+@@ -530,10 +530,12 @@ static int uart_put_char(struct tty_struct *tty, unsigned char c)
+ 	int ret = 0;
+
+ 	circ = &state->xmit;
+-	if (!circ->buf)
++	port = uart_port_lock(state, flags);
++	if (!circ->buf) {
++		uart_port_unlock(port, flags);
+ 		return 0;
++	}
+
+-	port = uart_port_lock(state, flags);
+ 	if (port && uart_circ_chars_free(circ) != 0) {
+ 		circ->buf[circ->head] = c;
+ 		circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);

patch/driver-uart-fix-race-between-uart_put_char-and-uart_shutdow.patch

@@ -0,0 +1,186 @@
+From a5ba1d95e46ecaea638ddd7cd144107c783acb5d Mon Sep 17 00:00:00 2001
+From: Tycho Andersen <[email protected]>
+Date: Fri, 6 Jul 2018 10:24:57 -0600
+Subject: [PATCH] uart: fix race between uart_put_char() and uart_shutdown()
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+We have reports of the following crash:
+
+    PID: 7 TASK: ffff88085c6d61c0 CPU: 1 COMMAND: "kworker/u25:0"
+    #0 [ffff88085c6db710] machine_kexec at ffffffff81046239
+    #1 [ffff88085c6db760] crash_kexec at ffffffff810fc248
+    #2 [ffff88085c6db830] oops_end at ffffffff81008ae7
+    #3 [ffff88085c6db860] no_context at ffffffff81050b8f
+    #4 [ffff88085c6db8b0] __bad_area_nosemaphore at ffffffff81050d75
+    #5 [ffff88085c6db900] bad_area_nosemaphore at ffffffff81050e83
+    #6 [ffff88085c6db910] __do_page_fault at ffffffff8105132e
+    #7 [ffff88085c6db9b0] do_page_fault at ffffffff8105152c
+    #8 [ffff88085c6db9c0] page_fault at ffffffff81a3f122
+    [exception RIP: uart_put_char+149]
+    RIP: ffffffff814b67b5 RSP: ffff88085c6dba78 RFLAGS: 00010006
+    RAX: 0000000000000292 RBX: ffffffff827c5120 RCX: 0000000000000081
+    RDX: 0000000000000000 RSI: 000000000000005f RDI: ffffffff827c5120
+    RBP: ffff88085c6dba98 R8: 000000000000012c R9: ffffffff822ea320
+    R10: ffff88085fe4db04 R11: 0000000000000001 R12: ffff881059f9c000
+    R13: 0000000000000001 R14: 000000000000005f R15: 0000000000000fba
+    ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
+    #9 [ffff88085c6dbaa0] tty_put_char at ffffffff81497544
+    #10 [ffff88085c6dbac0] do_output_char at ffffffff8149c91c
+    #11 [ffff88085c6dbae0] __process_echoes at ffffffff8149cb8b
+    #12 [ffff88085c6dbb30] commit_echoes at ffffffff8149cdc2
+    #13 [ffff88085c6dbb60] n_tty_receive_buf_fast at ffffffff8149e49b
+    #14 [ffff88085c6dbbc0] __receive_buf at ffffffff8149ef5a
+    #15 [ffff88085c6dbc20] n_tty_receive_buf_common at ffffffff8149f016
+    #16 [ffff88085c6dbca0] n_tty_receive_buf2 at ffffffff8149f194
+    #17 [ffff88085c6dbcb0] flush_to_ldisc at ffffffff814a238a
+    #18 [ffff88085c6dbd50] process_one_work at ffffffff81090be2
+    #19 [ffff88085c6dbe20] worker_thread at ffffffff81091b4d
+    #20 [ffff88085c6dbeb0] kthread at ffffffff81096384
+    #21 [ffff88085c6dbf50] ret_from_fork at ffffffff81a3d69f​
+
+after slogging through some dissasembly:
+
+ffffffff814b6720 <uart_put_char>:
+ffffffff814b6720:	55                   	push   %rbp
+ffffffff814b6721:	48 89 e5             	mov    %rsp,%rbp
+ffffffff814b6724:	48 83 ec 20          	sub    $0x20,%rsp
+ffffffff814b6728:	48 89 1c 24          	mov    %rbx,(%rsp)
+ffffffff814b672c:	4c 89 64 24 08       	mov    %r12,0x8(%rsp)
+ffffffff814b6731:	4c 89 6c 24 10       	mov    %r13,0x10(%rsp)
+ffffffff814b6736:	4c 89 74 24 18       	mov    %r14,0x18(%rsp)
+ffffffff814b673b:	e8 b0 8e 58 00       	callq  ffffffff81a3f5f0 <mcount>
+ffffffff814b6740:	4c 8b a7 88 02 00 00 	mov    0x288(%rdi),%r12
+ffffffff814b6747:	45 31 ed             	xor    %r13d,%r13d
+ffffffff814b674a:	41 89 f6             	mov    %esi,%r14d
+ffffffff814b674d:	49 83 bc 24 70 01 00 	cmpq   $0x0,0x170(%r12)
+ffffffff814b6754:	00 00
+ffffffff814b6756:	49 8b 9c 24 80 01 00 	mov    0x180(%r12),%rbx
+ffffffff814b675d:	00
+ffffffff814b675e:	74 2f                	je     ffffffff814b678f <uart_put_char+0x6f>
+ffffffff814b6760:	48 89 df             	mov    %rbx,%rdi
+ffffffff814b6763:	e8 a8 67 58 00       	callq  ffffffff81a3cf10 <_raw_spin_lock_irqsave>
+ffffffff814b6768:	41 8b 8c 24 78 01 00 	mov    0x178(%r12),%ecx
+ffffffff814b676f:	00
+ffffffff814b6770:	89 ca                	mov    %ecx,%edx
+ffffffff814b6772:	f7 d2                	not    %edx
+ffffffff814b6774:	41 03 94 24 7c 01 00 	add    0x17c(%r12),%edx
+ffffffff814b677b:	00
+ffffffff814b677c:	81 e2 ff 0f 00 00    	and    $0xfff,%edx
+ffffffff814b6782:	75 23                	jne    ffffffff814b67a7 <uart_put_char+0x87>
+ffffffff814b6784:	48 89 c6             	mov    %rax,%rsi
+ffffffff814b6787:	48 89 df             	mov    %rbx,%rdi
+ffffffff814b678a:	e8 e1 64 58 00       	callq  ffffffff81a3cc70 <_raw_spin_unlock_irqrestore>
+ffffffff814b678f:	44 89 e8             	mov    %r13d,%eax
+ffffffff814b6792:	48 8b 1c 24          	mov    (%rsp),%rbx
+ffffffff814b6796:	4c 8b 64 24 08       	mov    0x8(%rsp),%r12
+ffffffff814b679b:	4c 8b 6c 24 10       	mov    0x10(%rsp),%r13
+ffffffff814b67a0:	4c 8b 74 24 18       	mov    0x18(%rsp),%r14
+ffffffff814b67a5:	c9                   	leaveq
+ffffffff814b67a6:	c3                   	retq
+ffffffff814b67a7:	49 8b 94 24 70 01 00 	mov    0x170(%r12),%rdx
+ffffffff814b67ae:	00
+ffffffff814b67af:	48 63 c9             	movslq %ecx,%rcx
+ffffffff814b67b2:	41 b5 01             	mov    $0x1,%r13b
+ffffffff814b67b5:	44 88 34 0a          	mov    %r14b,(%rdx,%rcx,1)
+ffffffff814b67b9:	41 8b 94 24 78 01 00 	mov    0x178(%r12),%edx
+ffffffff814b67c0:	00
+ffffffff814b67c1:	83 c2 01             	add    $0x1,%edx
+ffffffff814b67c4:	81 e2 ff 0f 00 00    	and    $0xfff,%edx
+ffffffff814b67ca:	41 89 94 24 78 01 00 	mov    %edx,0x178(%r12)
+ffffffff814b67d1:	00
+ffffffff814b67d2:	eb b0                	jmp    ffffffff814b6784 <uart_put_char+0x64>
+ffffffff814b67d4:	66 66 66 2e 0f 1f 84 	data32 data32 nopw %cs:0x0(%rax,%rax,1)
+ffffffff814b67db:	00 00 00 00 00
+
+for our build, this is crashing at:
+
+    circ->buf[circ->head] = c;
+
+Looking in uart_port_startup(), it seems that circ->buf (state->xmit.buf)
+protected by the "per-port mutex", which based on uart_port_check() is
+state->port.mutex. Indeed, the lock acquired in uart_put_char() is
+uport->lock, i.e. not the same lock.
+
+Anyway, since the lock is not acquired, if uart_shutdown() is called, the
+last chunk of that function may release state->xmit.buf before its assigned
+to null, and cause the race above.
+
+To fix it, let's lock uport->lock when allocating/deallocating
+state->xmit.buf in addition to the per-port mutex.
+
+v2: switch to locking uport->lock on allocation/deallocation instead of
+    locking the per-port mutex in uart_put_char. Note that since
+    uport->lock is a spin lock, we have to switch the allocation to
+    GFP_ATOMIC.
+v3: move the allocation outside the lock, so we can switch back to
+    GFP_KERNEL
+
+Signed-off-by: Tycho Andersen <[email protected]>
+Cc: stable <[email protected]>
+Signed-off-by: Greg Kroah-Hartman <[email protected]>
+---
+ drivers/tty/serial/serial_core.c | 17 ++++++++++++-----
+ 1 file changed, 12 insertions(+), 5 deletions(-)
+
+diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
+index 9c14a45..80bb56f 100644
+--- a/drivers/tty/serial/serial_core.c
++++ b/drivers/tty/serial/serial_core.c
+@@ -182,6 +182,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
+ {
+ 	struct uart_port *uport = uart_port_check(state);
+ 	unsigned long page;
++	unsigned long flags = 0;
+ 	int retval = 0;
+
+ 	if (uport->type == PORT_UNKNOWN)
+@@ -196,15 +197,18 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
+ 	 * Initialise and allocate the transmit and temporary
+ 	 * buffer.
+ 	 */
+-	if (!state->xmit.buf) {
+-		/* This is protected by the per port mutex */
+-		page = get_zeroed_page(GFP_KERNEL);
+-		if (!page)
+-			return -ENOMEM;
++	page = get_zeroed_page(GFP_KERNEL);
++	if (!page)
++		return -ENOMEM;
+
++	uart_port_lock(state, flags);
++	if (!state->xmit.buf) {
+ 		state->xmit.buf = (unsigned char *) page;
+ 		uart_circ_clear(&state->xmit);
++	} else {
++		free_page(page);
+ 	}
++	uart_port_unlock(uport, flags);
+
+ 	retval = uport->ops->startup(uport);
+ 	if (retval == 0) {
+@@ -263,6 +267,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
+ {
+ 	struct uart_port *uport = uart_port_check(state);
+ 	struct tty_port *port = &state->port;
++	unsigned long flags = 0;
+
+ 	/*
+ 	 * Set the TTY IO error marker
+@@ -295,10 +300,12 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
+ 	/*
+ 	 * Free the transmit buffer page.
+ 	 */
++	uart_port_lock(state, flags);
+ 	if (state->xmit.buf) {
+ 		free_page((unsigned long)state->xmit.buf);
+ 		state->xmit.buf = NULL;
+ 	}
++	uart_port_unlock(uport, flags);
+ }
+
+ /**
+-- 
+2.7.4
+

patch/series

@@ -21,6 +21,8 @@ driver-sff-8436-use-nvmem-framework.patch
 driver-sff-8436-use-nvmem_device_read.patch
 driver-support-sff-8436-read-write-fix.patch
 driver-i2c-bus-intel-ismt-add-delay-param.patch
+driver-uart-fix-race-between-uart_put_char-and-uart_shutdow.patch
+driver-uart-fix-another-race-between-uart_put_char-and-uart_shutdow.patch
 driver-pca954x-i2c-mux-force-deselect-on-exit-flag.patch
 kernel-add-kexec-reboot-string.patch
 driver-hwmon-max6620.patch