0011-tty-n_r3964-fix-race-with-add_msg-and-read_telegram.patch

From b54481aa9fb9cab7a0ca3257c4a6ba6082ef9652 Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Fri, 25 Jan 2019 15:03:15 +0100
Subject: [PATCH 11/15] tty: n_r3964: fix race with add_msg() and read_telegram

read_telegram() could race with the client block access code that can be
called through add_msg() as it did not hold any locks.

Fix that up by properly holding the client->lock when touching the next
message to be read structures.  Gyrations ensue due to the data having
to be copied to userspace, but the potential corruption problem is now
gone, to be replaced with a possible race where we free a block that no
one sent to userspace yet.  Given that no one has reported this problem
in the 20+ years of this code, I'll take the potential race condition
over a known corruption problems.

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/n_r3964.c |   52 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 10 deletions(-)

--- a/drivers/tty/n_r3964.c
+++ b/drivers/tty/n_r3964.c
@@ -193,8 +193,6 @@ static void receive_char(struct r3964_in
 static void receive_error(struct r3964_info *pInfo, const char flag);
 static void on_timeout(struct timer_list *t);
 static int enable_signals(struct r3964_info *pInfo, struct pid *pid, int arg);
-static int read_telegram(struct r3964_info *pInfo, struct pid *pid,
-		unsigned char __user * buf);
 static void add_msg(struct r3964_client_info *pClient, int msg_id, int arg,
 		int error_code, struct rx_block_header *pBlock);
 static struct r3964_message *remove_msg(struct r3964_client_info *client);
@@ -831,6 +829,10 @@ static int read_telegram(struct r3964_in
 {
 	struct r3964_client_info *pClient;
 	struct rx_block_header *block;
+	unsigned long flags;
+	unsigned int length;
+	int retval = 0;
+	u8 *data;
 
 	if (!buf) {
 		return -EINVAL;
@@ -841,18 +843,47 @@ static int read_telegram(struct r3964_in
 		return -EINVAL;
 	}
 
+	spin_lock_irqsave(&pClient->lock, flags);
+
 	block = pClient->next_block_to_read;
-	if (!block) {
-		return 0;
-	} else {
-		if (copy_to_user(buf, block->data, block->length))
-			return -EFAULT;
+	if (!block)
+		goto exit;
+
+	/*
+	 * Duplicate the data so we can release the lock while we copy to
+	 * userspace
+	 */
+	length = block->length;
+	data = kmemdup(block->data, length, GFP_ATOMIC);
+	if (!data) {
+		retval = -ENOMEM;
+		goto exit;
+	}
+
+	spin_unlock_irqrestore(&pClient->lock, flags);
 
-		remove_client_block(pClient);
-		return block->length;
+	if (copy_to_user(buf, data, length)) {
+		kfree(data);
+		return -EFAULT;
 	}
+	kfree(data);
 
-	return -EINVAL;
+	/*
+	 * Copy succeeded, so grab the lock again, and then drop the buffer, as
+	 * remove_client_block() has to have the lock held.
+	 *
+	 * Note, the client's next_block_to_read could have changed here, so
+	 * worst case, we just dropped a buffer that wasn't read.  But, nothing
+	 * was corrupted or accidentally freed, so we are doing better than we
+	 * used to.  Ideally the whole issue of "read_telegram" would be handled
+	 * some other way as this races with the add_msg() path in a bad manner
+	 */
+	spin_lock_irqsave(&pClient->lock, flags);
+	remove_client_block(pClient);
+
+exit:
+	spin_unlock_irqrestore(&pClient->lock, flags);
+	return retval;
 }
 
 static void __add_msg(struct r3964_client_info *pClient, int msg_id, int arg,
@@ -935,6 +966,7 @@ static struct r3964_message *remove_msg(
 }
 
 static void remove_client_block(struct r3964_client_info *client)
+	__must_hold(&client->lock)
 {
 	struct rx_block_header *block;