Received-SPF: pass (google.com: domain of linux-kernel+bounces-172970-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: kovalev@altlinux.org
To: gregkh@linuxfoundation.org,
	jirislaby@kernel.org,
	linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org
Cc: lvc-project@linuxtesting.org,
	dutyrok@altlinux.org,
	oficerovas@altlinux.org,
	Vasiliy Kovalev <kovalev@altlinux.org>,
	stable@vger.kernel.org
Subject: [PATCH] tty: Fix possible deadlock in tty_buffer_flush
Date: Wed,  8 May 2024 12:30:05 +0300
Message-Id: <20240508093005.1044815-1-kovalev@altlinux.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

From: Vasiliy Kovalev <kovalev@altlinux.org>

A possible scenario in which a deadlock may occur is as follows:

flush_to_ldisc() {

  mutex_lock(&buf->lock);

  tty_port_default_receive_buf() {
    tty_ldisc_receive_buf() {
      n_tty_receive_buf2() {
	n_tty_receive_buf_common() {
	  n_tty_receive_char_special() {
	    isig() {
	      tty_driver_flush_buffer() {
		pty_flush_buffer() {
		  tty_buffer_flush() {

		    mutex_lock(&buf->lock); (DEADLOCK)

flush_to_ldisc() and tty_buffer_flush() functions they use the same mutex
(&buf->lock), but not necessarily the same struct tty_bufhead object.
However, you should probably use a separate mutex for the
tty_buffer_flush() function to exclude such a situation.

Found by Syzkaller:
======================================================
WARNING: possible circular locking dependency detected
5.10.213-std-def-alt1 #1 Not tainted
------------------------------------------------------
kworker/u6:8/428 is trying to acquire lock:
ffff88810c3498b8 (&buf->lock){+.+.}-{3:3},
        at: tty_buffer_flush+0x7b/0x2b0 drivers/tty/tty_buffer.c:228

but task is already holding lock:
ffff888114dca2e8 (&o_tty->termios_rwsem/1){++++}-{3:3},
        at: isig+0xef/0x440 drivers/tty/n_tty.c:1127

which lock already depends on the new lock.

Chain exists of:
  &buf->lock --> &port->buf.lock/1 --> &o_tty->termios_rwsem/1

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&o_tty->termios_rwsem/1);
                               lock(&port->buf.lock/1);
                               lock(&o_tty->termios_rwsem/1);
  lock(&buf->lock);

stack backtrace:
CPU: 0 PID: 428 Comm: kworker/u6:8 Not tainted 5.10.213-std-def-alt1 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
                BIOS 1.16.0-alt1 04/01/2014
Workqueue: events_unbound flush_to_ldisc
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x19b/0x203 lib/dump_stack.c:118
 print_circular_bug.cold+0x162/0x171 kernel/locking/lockdep.c:2002
 check_noncircular+0x263/0x2e0 kernel/locking/lockdep.c:2123
 check_prev_add kernel/locking/lockdep.c:2988 [inline]
 check_prevs_add kernel/locking/lockdep.c:3113 [inline]
 validate_chain kernel/locking/lockdep.c:3729 [inline]
 __lock_acquire+0x298f/0x5500 kernel/locking/lockdep.c:4955
 lock_acquire kernel/locking/lockdep.c:5566 [inline]
 lock_acquire+0x1fe/0x550 kernel/locking/lockdep.c:5531
 __mutex_lock_common kernel/locking/mutex.c:968 [inline]
 __mutex_lock+0x142/0x10c0 kernel/locking/mutex.c:1109
 mutex_lock_nested+0x17/0x20 kernel/locking/mutex.c:1124
 tty_buffer_flush+0x7b/0x2b0 drivers/tty/tty_buffer.c:228
 pty_flush_buffer+0x4e/0x170 drivers/tty/pty.c:222
 tty_driver_flush_buffer+0x65/0x80 drivers/tty/tty_ioctl.c:96
 isig+0x1e4/0x440 drivers/tty/n_tty.c:1138
 n_tty_receive_signal_char+0x24/0x160 drivers/tty/n_tty.c:1239
 n_tty_receive_char_special+0x1261/0x2a70 drivers/tty/n_tty.c:1285
 n_tty_receive_buf_fast drivers/tty/n_tty.c:1606 [inline]
 __receive_buf drivers/tty/n_tty.c:1640 [inline]
 n_tty_receive_buf_common+0x1e76/0x2b60 drivers/tty/n_tty.c:1738
 n_tty_receive_buf2+0x34/0x40 drivers/tty/n_tty.c:1773
 tty_ldisc_receive_buf+0xb1/0x1a0 drivers/tty/tty_buffer.c:441
 tty_port_default_receive_buf+0x73/0xa0 drivers/tty/tty_port.c:39
 receive_buf drivers/tty/tty_buffer.c:461 [inline]
 flush_to_ldisc+0x21c/0x400 drivers/tty/tty_buffer.c:513
 process_one_work+0x9ae/0x14b0 kernel/workqueue.c:2282
 worker_thread+0x622/0x1320 kernel/workqueue.c:2428
 kthread+0x396/0x470 kernel/kthread.c:313
 ret_from_fork+0x22/0x30 arch/x86/entry/entry_64.S:299

Cc: stable@vger.kernel.org
Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
---
 drivers/tty/tty_buffer.c   | 5 +++--
 include/linux/tty_buffer.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 79f0ff94ce00da..e777bd5f3a2fca 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -226,7 +226,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 
 	atomic_inc(&buf->priority);
 
-	mutex_lock(&buf->lock);
+	mutex_lock(&buf->flush_mtx);
 	/* paired w/ release in __tty_buffer_request_room; ensures there are
 	 * no pending memory accesses to the freed buffer
 	 */
@@ -241,7 +241,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 		ld->ops->flush_buffer(tty);
 
 	atomic_dec(&buf->priority);
-	mutex_unlock(&buf->lock);
+	mutex_unlock(&buf->flush_mtx);
 }
 
 /**
@@ -577,6 +577,7 @@ void tty_buffer_init(struct tty_port *port)
 {
 	struct tty_bufhead *buf = &port->buf;
 
+	mutex_init(&buf->flush_mtx);
 	mutex_init(&buf->lock);
 	tty_buffer_reset(&buf->sentinel, 0);
 	buf->head = &buf->sentinel;
diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h
index 31125e3be3c55e..cea4eacc3b70d3 100644
--- a/include/linux/tty_buffer.h
+++ b/include/linux/tty_buffer.h
@@ -35,6 +35,7 @@ static inline u8 *flag_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 struct tty_bufhead {
 	struct tty_buffer *head;	/* Queue head */
 	struct work_struct work;
+	struct mutex	   flush_mtx;	/* For use in tty_buffer_flush() */
 	struct mutex	   lock;
 	atomic_t	   priority;
 	struct tty_buffer sentinel;
-- 
2.33.8