From da04c3926a73304489a320796045814ac48f2e37 Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Fri, 1 Feb 2019 10:44:55 +0100
Subject: [PATCH 15/15] tty: n_r3964: add reference counting to the client
 structure

The client structure pointer is thrown around a lot, and trying to keep
track of who has, and does not have, a valid pointer is almost
impossible to audit properly, especially when looking up client
structures from the lists.  So add a kref to the structure so that it
will be automatically reference counted and no one can access a stale
pointer and memory will be freed when everything is finished.

This should resolve the problem with the client structure pointer beging
able to be accessed when it was removed by someone else at the same
time.

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

--- a/drivers/tty/n_r3964.c
+++ b/drivers/tty/n_r3964.c
@@ -106,6 +106,7 @@ struct tx_block_header;
 
 struct r3964_client_info {
 	spinlock_t lock;
+	struct kref kref;
 	struct pid *pid;
 	unsigned int sig_flags;
 
@@ -298,6 +299,30 @@ static int __init r3964_init(void)
 module_init(r3964_init);
 module_exit(r3964_exit);
 
+static struct r3964_client_info *client_get(struct r3964_client_info *client)
+{
+	if (client)
+		kref_get(&client->kref);
+	return client;
+}
+
+static void client_free(struct kref *kref)
+{
+	struct r3964_client_info *client;
+
+	client = container_of(kref, struct r3964_client_info, kref);
+
+	put_pid(client->pid);
+	list_del(&client->node);
+	kfree(client);
+}
+
+static void client_put(struct r3964_client_info *client)
+{
+	if (client)
+		kref_put(&client->kref, client_free);
+}
+
 /*************************************************************
  * Protocol implementation routines
  *************************************************************/
@@ -339,6 +364,7 @@ static void remove_from_tx_queue(struct
 		wake_up_interruptible(&pInfo->tty->read_wait);
 	}
 
+	client_put(pHeader->owner);
 	kfree(pHeader);
 }
 
@@ -550,6 +576,7 @@ static void on_receive_block(struct r396
 		unsigned long client_flags;
 		unsigned int sig_flags;
 
+		client_get(pClient);
 		spin_lock_irqsave(&pClient->lock, client_flags);
 		sig_flags = pClient->sig_flags;
 		spin_unlock_irqrestore(&pClient->lock, client_flags);
@@ -558,6 +585,7 @@ static void on_receive_block(struct r396
 			add_msg(pClient, R3964_MSG_DATA, length, R3964_OK,
 				pBlock);
 		}
+		client_put(pClient);
 	}
 	spin_unlock_irqrestore(&pInfo->lock, flags);
 	wake_up_interruptible(&pInfo->tty->read_wait);
@@ -752,25 +780,40 @@ static void on_timeout(struct timer_list
 	}
 }
 
+/*
+ * The reference count of the pointer returned will be incremented
+ * (if it is not NULL) so client_put() must be called on it when the
+ * caller is finished with it.
+ */
 static struct r3964_client_info *findClient(struct r3964_info *pInfo,
 		struct pid *pid)
 {
 	struct r3964_client_info *pClient;
-	unsigned long flags;
+	unsigned long client_flags;
+	unsigned long info_flags;
 
-	spin_lock_irqsave(&pInfo->lock, flags);
+	spin_lock_irqsave(&pInfo->lock, info_flags);
 	list_for_each_entry(pClient, &pInfo->clients, node) {
+		client_get(pClient);
+		spin_lock_irqsave(&pClient->lock, client_flags);
 		if (pClient->pid == pid) {
+			spin_unlock_irqrestore(&pClient->lock, client_flags);
 			goto exit;
 		}
+		spin_unlock_irqrestore(&pClient->lock, client_flags);
+		client_put(pClient);
 	}
 	pClient = NULL;
 exit:
-	spin_unlock_irqrestore(&pInfo->lock, flags);
+	spin_unlock_irqrestore(&pInfo->lock, info_flags);
 	return pClient;
 }
 
-/* Find a client that refers to the pid of the current task */
+/*
+ * Find a client that refers to the pid of the current task
+ * The reference count of the pointer will be incremented so
+ * client_put() must be called when the caller is finished with it
+ */
 static struct r3964_client_info *find_client_current(struct r3964_info *info)
 {
 	struct r3964_client_info *client;
@@ -793,6 +836,7 @@ static int enable_signals(struct r3964_i
 
 		/* Remove client from client list */
 		list_for_each_entry(pClient, &pInfo->clients, node) {
+			client_get(pClient);
 			if (pClient->pid == pid) {
 				TRACE_PS("removing client %d from client list",
 					 pid_nr(pid));
@@ -804,13 +848,13 @@ static int enable_signals(struct r3964_i
 							"kfree %p", pMsg);
 					}
 				}
-				put_pid(pClient->pid);
-				list_del(&pClient->node);
-				kfree(pClient);
-				TRACE_M("enable_signals - kfree %p", pClient);
+				/* Second put will free the structure */
+				client_put(pClient);
+				client_put(pClient);
 				spin_unlock_irqrestore(&pInfo->lock, flags);
 				return 0;
 			}
+			client_put(pClient);
 		}
 		spin_unlock_irqrestore(&pInfo->lock, flags);
 		return -EINVAL;
@@ -823,6 +867,7 @@ static int enable_signals(struct r3964_i
 			spin_lock_irqsave(&pClient->lock, client_flags);
 			pClient->sig_flags = arg;
 			spin_unlock_irqrestore(&pClient->lock, client_flags);
+			client_put(pClient);
 		} else {
 			/* add client to client list */
 			pClient = kmalloc(sizeof(struct r3964_client_info),
@@ -834,6 +879,7 @@ static int enable_signals(struct r3964_i
 			spin_lock_irqsave(&pInfo->lock, flags);
 			TRACE_PS("add client %d to client list", pid_nr(pid));
 			spin_lock_init(&pClient->lock);
+			kref_init(&pClient->kref);
 			pClient->sig_flags = arg;
 			pClient->pid = get_pid(pid);
 			INIT_LIST_HEAD(&pClient->msgs);
@@ -887,6 +933,7 @@ static int read_telegram(struct r3964_in
 
 	if (copy_to_user(buf, data, length)) {
 		kfree(data);
+		client_put(pClient);
 		return -EFAULT;
 	}
 	kfree(data);
@@ -906,6 +953,7 @@ static int read_telegram(struct r3964_in
 
 exit:
 	spin_unlock_irqrestore(&pClient->lock, flags);
+	client_put(pClient);
 	return retval;
 }
 
@@ -1091,10 +1139,7 @@ static void r3964_close(struct tty_struc
 				TRACE_M("r3964_close - msg kfree %p", pMsg);
 			}
 		}
-		put_pid(pClient->pid);
-		list_del(&pClient->node);
-		kfree(pClient);
-		TRACE_M("r3964_close - client kfree %p", pClient);
+		client_put(pClient);
 	}
 	/* Remove jobs from tx_queue: */
 	spin_lock_irqsave(&pInfo->lock, flags);
@@ -1174,6 +1219,7 @@ static ssize_t r3964_read(struct tty_str
 	ret = -EPERM;
 unlock:
 	mutex_unlock(&pInfo->read_lock);
+	client_put(pClient);
 	return ret;
 }
 
@@ -1182,7 +1228,6 @@ static ssize_t r3964_write(struct tty_st
 {
 	struct r3964_info *pInfo = tty->disc_data;
 	struct tx_block_header *pHeader;
-	struct r3964_client_info *pClient;
 	unsigned char *new_data;
 
 	TRACE_L("write request, %d characters", count);
@@ -1218,12 +1263,12 @@ static ssize_t r3964_write(struct tty_st
 	pHeader = (struct tx_block_header *)new_data;
 	pHeader->data = new_data + sizeof(*pHeader);
 	pHeader->length = count;
-	pHeader->owner = NULL;
 
-	pClient = find_client_current(pInfo);
-	if (pClient) {
-		pHeader->owner = pClient;
-	}
+	/*
+	 * The reference count is left incremented as the last user of
+	 * this pointer will drop it when needed
+	 */
+	pHeader->owner = find_client_current(pInfo);
 
 	memcpy(pHeader->data, data, count);	/* We already verified this */
 
@@ -1334,6 +1379,7 @@ static __poll_t r3964_poll(struct tty_st
 		if (!list_empty(&pClient->msgs))
 			result |= EPOLLIN | EPOLLRDNORM;
 		spin_unlock_irqrestore(&pClient->lock, flags);
+		client_put(pClient);
 	} else {
 		result = EPOLLNVAL | EPOLLERR;
 	}