From: Haimin Zhang <tcs.kernel@gmail.com>

syzbot report an uninit-value issue in n_tty_receive_buf_common.
The root case is in the tty_buffer_reset() which in tty_buffer_alloc()
function, it initialized the tty_buffer struct but did not initialize 
"data[]" points area. So we should initialize the points area to avoid
using dirty data.
The syzbot report is as follows:

BUG: KMSAN: uninit-value in variable_test_bit arch/x86/include/asm/bitops.h:214 [inline]
BUG: KMSAN: uninit-value in test_bit include/asm-generic/bitops/instrumented-non-atomic.h:135 [inline]
BUG: KMSAN: uninit-value in n_tty_receive_buf_standard drivers/tty/n_tty.c:1557 [inline]
BUG: KMSAN: uninit-value in __receive_buf drivers/tty/n_tty.c:1577 [inline]
BUG: KMSAN: uninit-value in n_tty_receive_buf_common+0x1e6c/0x10360 drivers/tty/n_tty.c:1674
 variable_test_bit arch/x86/include/asm/bitops.h:214 [inline]
 test_bit include/asm-generic/bitops/instrumented-non-atomic.h:135 [inline]
 n_tty_receive_buf_standard drivers/tty/n_tty.c:1557 [inline]
 __receive_buf drivers/tty/n_tty.c:1577 [inline]
 n_tty_receive_buf_common+0x1e6c/0x10360 drivers/tty/n_tty.c:1674
 n_tty_receive_buf2+0xbe/0xd0 drivers/tty/n_tty.c:1709
 tty_ldisc_receive_buf+0x15e/0x390 drivers/tty/tty_buffer.c:471
 tty_port_default_receive_buf+0x14b/0x1e0 drivers/tty/tty_port.c:39
 receive_buf drivers/tty/tty_buffer.c:491 [inline]
 flush_to_ldisc+0x5bf/0xa10 drivers/tty/tty_buffer.c:543
 process_one_work+0xdc2/0x1820 kernel/workqueue.c:2298
 worker_thread+0x10f1/0x2290 kernel/workqueue.c:2445
 kthread+0x721/0x850 kernel/kthread.c:327
 ret_from_fork+0x1f/0x30

Uninit was created at:
 slab_post_alloc_hook mm/slab.h:524 [inline]
 slab_alloc_node mm/slub.c:3251 [inline]
 slab_alloc mm/slub.c:3259 [inline]
 __kmalloc+0xc3c/0x12d0 mm/slub.c:4437
 kmalloc include/linux/slab.h:595 [inline]
 tty_buffer_alloc drivers/tty/tty_buffer.c:177 [inline]
 __tty_buffer_request_room+0x4d2/0x900 drivers/tty/tty_buffer.c:275
 __tty_insert_flip_char+0xe5/0x3d0 drivers/tty/tty_buffer.c:392
 tty_insert_flip_char include/linux/tty_flip.h:36 [inline]
 uart_insert_char+0x495/0xb70 drivers/tty/serial/serial_core.c:3139
 serial8250_read_char+0x280/0x820 drivers/tty/serial/8250/8250_port.c:1769
 serial8250_rx_chars drivers/tty/serial/8250/8250_port.c:1784 [inline]
 serial8250_handle_irq+0x540/0x980 drivers/tty/serial/8250/8250_port.c:1927
 serial8250_default_handle_irq+0x18f/0x370 drivers/tty/serial/8250/8250_port.c:1949
 serial8250_interrupt+0x111/0x3f0 drivers/tty/serial/8250/8250_core.c:126
 __handle_irq_event_percpu+0x188/0xc90 kernel/irq/handle.c:158
 handle_irq_event_percpu kernel/irq/handle.c:198 [inline]
 handle_irq_event+0x188/0x420 kernel/irq/handle.c:215
 handle_edge_irq+0x472/0x13e0 kernel/irq/chip.c:822
 generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
 handle_irq arch/x86/kernel/irq.c:231 [inline]
 __common_interrupt+0xf8/0x360 arch/x86/kernel/irq.c:250
 common_interrupt+0xb1/0xd0 arch/x86/kernel/irq.c:240
 asm_common_interrupt+0x1e/0x40

Reported-by: syzbot+b68d24ad0de64bdba684@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com>
---
 drivers/tty/tty_buffer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6c7e65b1d9a1..0e7f3547d971 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -180,6 +180,7 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_port *port, size_t size)
 
 found:
 	tty_buffer_reset(p, size);
+	memset((char *)p + sizeof(struct tty_buffer), 0, 2 * size);
 	atomic_add(size, &port->buf.mem_used);
 	return p;
 }
-- 
2.25.1

On Fri, Jan 07, 2022 at 05:30:58PM +0800, tcs.kernel@gmail.com wrote:
> From: Haimin Zhang <tcs.kernel@gmail.com>
> 
> syzbot report an uninit-value issue in n_tty_receive_buf_common.
> The root case is in the tty_buffer_reset() which in tty_buffer_alloc()
> function, it initialized the tty_buffer struct but did not initialize 
> "data[]" points area. So we should initialize the points area to avoid
> using dirty data.

Why?  The data is not really "dirty", it is just data that the driver
used in the past.  The driver should always overwrite it with it's own
data anyway, it does not care what is in the buffer to start with.

As "proof" of this, look a few lines up from your change, the buffer is
allocated with kmalloc() just fine, not kzalloc(), right?  :)

Is this data getting sent to userspace somehow with stale data?  If so,
what tty driver is doing that?

> The syzbot report is as follows:
> 
> BUG: KMSAN: uninit-value in variable_test_bit arch/x86/include/asm/bitops.h:214 [inline]
> BUG: KMSAN: uninit-value in test_bit include/asm-generic/bitops/instrumented-non-atomic.h:135 [inline]
> BUG: KMSAN: uninit-value in n_tty_receive_buf_standard drivers/tty/n_tty.c:1557 [inline]
> BUG: KMSAN: uninit-value in __receive_buf drivers/tty/n_tty.c:1577 [inline]
> BUG: KMSAN: uninit-value in n_tty_receive_buf_common+0x1e6c/0x10360 drivers/tty/n_tty.c:1674
>  variable_test_bit arch/x86/include/asm/bitops.h:214 [inline]
>  test_bit include/asm-generic/bitops/instrumented-non-atomic.h:135 [inline]
>  n_tty_receive_buf_standard drivers/tty/n_tty.c:1557 [inline]
>  __receive_buf drivers/tty/n_tty.c:1577 [inline]
>  n_tty_receive_buf_common+0x1e6c/0x10360 drivers/tty/n_tty.c:1674
>  n_tty_receive_buf2+0xbe/0xd0 drivers/tty/n_tty.c:1709
>  tty_ldisc_receive_buf+0x15e/0x390 drivers/tty/tty_buffer.c:471
>  tty_port_default_receive_buf+0x14b/0x1e0 drivers/tty/tty_port.c:39
>  receive_buf drivers/tty/tty_buffer.c:491 [inline]
>  flush_to_ldisc+0x5bf/0xa10 drivers/tty/tty_buffer.c:543
>  process_one_work+0xdc2/0x1820 kernel/workqueue.c:2298
>  worker_thread+0x10f1/0x2290 kernel/workqueue.c:2445
>  kthread+0x721/0x850 kernel/kthread.c:327
>  ret_from_fork+0x1f/0x30

Are you sure the the tool is correct here?  If so, how come the
kmalloc() allocation does not trigger a report here?



> 
> Uninit was created at:
>  slab_post_alloc_hook mm/slab.h:524 [inline]
>  slab_alloc_node mm/slub.c:3251 [inline]
>  slab_alloc mm/slub.c:3259 [inline]
>  __kmalloc+0xc3c/0x12d0 mm/slub.c:4437
>  kmalloc include/linux/slab.h:595 [inline]
>  tty_buffer_alloc drivers/tty/tty_buffer.c:177 [inline]
>  __tty_buffer_request_room+0x4d2/0x900 drivers/tty/tty_buffer.c:275
>  __tty_insert_flip_char+0xe5/0x3d0 drivers/tty/tty_buffer.c:392
>  tty_insert_flip_char include/linux/tty_flip.h:36 [inline]
>  uart_insert_char+0x495/0xb70 drivers/tty/serial/serial_core.c:3139
>  serial8250_read_char+0x280/0x820 drivers/tty/serial/8250/8250_port.c:1769
>  serial8250_rx_chars drivers/tty/serial/8250/8250_port.c:1784 [inline]
>  serial8250_handle_irq+0x540/0x980 drivers/tty/serial/8250/8250_port.c:1927
>  serial8250_default_handle_irq+0x18f/0x370 drivers/tty/serial/8250/8250_port.c:1949
>  serial8250_interrupt+0x111/0x3f0 drivers/tty/serial/8250/8250_core.c:126
>  __handle_irq_event_percpu+0x188/0xc90 kernel/irq/handle.c:158
>  handle_irq_event_percpu kernel/irq/handle.c:198 [inline]
>  handle_irq_event+0x188/0x420 kernel/irq/handle.c:215
>  handle_edge_irq+0x472/0x13e0 kernel/irq/chip.c:822
>  generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
>  handle_irq arch/x86/kernel/irq.c:231 [inline]
>  __common_interrupt+0xf8/0x360 arch/x86/kernel/irq.c:250
>  common_interrupt+0xb1/0xd0 arch/x86/kernel/irq.c:240
>  asm_common_interrupt+0x1e/0x40
> 
> Reported-by: syzbot+b68d24ad0de64bdba684@syzkaller.appspotmail.com
> Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com>
> ---
>  drivers/tty/tty_buffer.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 6c7e65b1d9a1..0e7f3547d971 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -180,6 +180,7 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_port *port, size_t size)
>  
>  found:
>  	tty_buffer_reset(p, size);
> +	memset((char *)p + sizeof(struct tty_buffer), 0, 2 * size);

As others have pointed out, this is a crazy line to try to understand,
and could be made simpler.  If it were even needed, which I do not think
it is.

thanks,

greg k-h