Re: How to handle concurrent access to /dev/ttyprintk ?

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Samo Pogacnik <samo_pogacnik@t-2.net>,
	Petr Mladek <pmladek@suse.com>, Jiri Slaby <jirislaby@kernel.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	John Ogness <john.ogness@linutronix.de>,
	linux-kernel@vger.kernel.org,
	syzkaller-bugs <syzkaller-bugs@googlegroups.com>
Subject: Re: How to handle concurrent access to /dev/ttyprintk ?
Date: Mon, 12 Apr 2021 20:25:27 +0900	[thread overview]
Message-ID: <32e75be6-6e9f-b33f-d585-13db220519da@i-love.sakura.ne.jp> (raw)
In-Reply-To: <YHQkeZVs3pmyie9e@kroah.com>

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 ?

