linux-kernel - [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20191121152239.28405-2-sudipm.mukherjee@gmail.com>
Date:   Thu, 21 Nov 2019 15:22:39 +0000
From:   Sudip Mukherjee <sudipm.mukherjee@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>
Cc:     linux-kernel@...r.kernel.org,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>
Subject: [PATCH v2 2/2] tty: use tty_init_dev_retry() to workaround a race condition

There seems to be a race condition in tty drivers and I could see on
many boot cycles a NULL pointer dereference as tty_init_dev() tries to
do 'tty->port->itty = tty' even though tty->port is NULL.
'tty->port' will be set by the driver and if the driver has not yet done
it before we open the tty device we can get to this situation. By adding
some extra debug prints, I noticed that:

6.650130: uart_add_one_port
6.663849: register_console
6.664846: tty_open
6.674391: tty_init_dev
6.675456: tty_port_link_device

uart_add_one_port() registers the console, as soon as it registers, the
userspace tries to use it and that leads to tty_open() but
uart_add_one_port() has not yet done tty_port_link_device() and so
tty->port is not yet configured when control reaches tty_init_dev().

So, add one retry and use tty_init_dev_retry().

Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@...il.com>
---

v1: had some hardcoded numbers which were difficult to understand.
https://lore.kernel.org/lkml/20191120151709.14148-2-sudipm.mukherjee@gmail.com/

I know this is not a proper fix, and the proper fix should have been to
have a lock. But that will be too intrusive and adding retry was a safer
option than that.

 drivers/tty/tty_io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 95d7abeca254..f71a11895230 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1945,6 +1945,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
 /**
  *	tty_open_by_driver	-	open a tty device
  *	@device: dev_t of device to open
+ *	@retry: number of times to retry if tty_init_dev_retry fails
  *	@filp: file pointer to tty
  *
  *	Performs the driver lookup, checks for a reopen, or otherwise
@@ -1957,7 +1958,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
  *	  - concurrent tty driver removal w/ lookup
  *	  - concurrent tty removal from driver table
  */
-static struct tty_struct *tty_open_by_driver(dev_t device,
+static struct tty_struct *tty_open_by_driver(dev_t device, int retry,
 					     struct file *filp)
 {
 	struct tty_struct *tty;
@@ -2001,7 +2002,7 @@ static struct tty_struct *tty_open_by_driver(dev_t device,
 			tty = ERR_PTR(retval);
 		}
 	} else { /* Returns with the tty_lock held for now */
-		tty = tty_init_dev(driver, index);
+		tty = tty_init_dev_retry(driver, index, retry);
 		mutex_unlock(&tty_mutex);
 	}
 out:
@@ -2036,7 +2037,7 @@ static struct tty_struct *tty_open_by_driver(dev_t device,
 static int tty_open(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty;
-	int noctty, retval;
+	int noctty, retval, retry = 1;
 	dev_t device = inode->i_rdev;
 	unsigned saved_flags = filp->f_flags;
 
@@ -2049,7 +2050,7 @@ static int tty_open(struct inode *inode, struct file *filp)
 
 	tty = tty_open_current_tty(device, filp);
 	if (!tty)
-		tty = tty_open_by_driver(device, filp);
+		tty = tty_open_by_driver(device, retry--, filp);
 
 	if (IS_ERR(tty)) {
 		tty_free_file(filp);
-- 
2.11.0