linux-kernel - Re: How to handle concurrent access to /dev/ttyprintk ?

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: <32e75be6-6e9f-b33f-d585-13db220519da@i-love.sakura.ne.jp>
Date:   Mon, 12 Apr 2021 20:25:27 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Samo Pogacnik <samo_pogacnik@....net>,
        Petr Mladek <pmladek@...e.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        John Ogness <john.ogness@...utronix.de>,
        linux-kernel@...r.kernel.org,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: How to handle concurrent access to /dev/ttyprintk ?

On 2021/04/12 19:44, Greg Kroah-Hartman wrote:
> And trying to "open exclusive only" just does not work, the kernel can
> not enforce that at all, sorry.  Any driver that you see trying to do
> that is trivial to work around in userspace, making the kernel code
> pointless.

You mean something like below cannot be used?

diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index 6a0059e508e3..57200569918a 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -84,14 +84,26 @@ static int tpk_printk(const unsigned char *buf, int count)
 	return count;
 }
 
+static DEFINE_MUTEX(open_close_lock);
+
 /*
  * TTY operations open function.
  */
 static int tpk_open(struct tty_struct *tty, struct file *filp)
 {
-	tty->driver_data = &tpk_port;
+	int ret;
+
+	if (mutex_lock_killable(&open_close_lock))
+		return -EINTR;
 
-	return tty_port_open(&tpk_port.port, tty, filp);
+	if (tpk_port.port.count) {
+		ret = -EBUSY;
+	} else {
+		tty->driver_data = &tpk_port;
+		ret = tty_port_open(&tpk_port.port, tty, filp);
+	}
+	mutex_unlock(&open_close_lock);
+	return ret;
 }
 
 /*
@@ -102,12 +114,14 @@ static void tpk_close(struct tty_struct *tty, struct file *filp)
 	struct ttyprintk_port *tpkp = tty->driver_data;
 	unsigned long flags;
 
+	mutex_lock(&open_close_lock);
 	spin_lock_irqsave(&tpkp->spinlock, flags);
 	/* flush tpk_printk buffer */
 	tpk_printk(NULL, 0);
 	spin_unlock_irqrestore(&tpkp->spinlock, flags);
 
 	tty_port_close(&tpkp->port, tty, filp);
+	mutex_unlock(&open_close_lock);
 }
 
 /*

> Like any tty port, if you have multiple accesses, all bets are off and
> hilarity ensues.  Just don't do that and expect things to be working
> well.

Since syzkaller is a fuzzer, syzkaller happily opens /dev/ttyprintk from
multiple threads. Should we update syzkaller to use CONFIG_TTY_PRINTK=n ?