Re: [PATCH] tty tty_buffer: fix uninit-value in n_tty_receive_buf_common

[Jiri Slaby]

On 08. 01. 22, 0:24, Andy Shevchenko wrote:
> On Friday, January 7, 2022, <tcs.kernel@xxxxxxxxx 
> <mailto:tcs.kernel@xxxxxxxxx>> wrote:
>
>     From: Haimin Zhang <tcs.kernel@xxxxxxxxx <mailto:tcs.kernel@xxxxxxxxx>>
>
>     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
>
>
> It’s rather long and noisy trace back, can you decrease it to the point, 
> please?
>
>     Reported-by: syzbot+b68d24ad0de64bdba684@xxxxxxxxxxxxxxxxxxxxxxxxx
>     <mailto:syzbot+b68d24ad0de64bdba684@xxxxxxxxxxxxxxxxxxxxxxxxx>
>     Signed-off-by: Haimin Zhang <tcs.kernel@xxxxxxxxx
>     <mailto:tcs.kernel@xxxxxxxxx>>
>     ---
>       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);
>
>
> I’m wondering if you may use offsetof() or other suitable macro instead 
> of this cryptic pointer arithmetic.

That memset was one of the WTFs. Why not simply p->data?

Anyway, it remains to explain why something touches the data, when the 
metadata are properly reset?

thanks,
--
js
suse labs