Received-SPF: pass (google.com: domain of linux-kernel+bounces-201310-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Aleksandr Mishin <amishin@t-argos.ru>
To: Jakob Koschel <jakobkoschel@gmail.com>
CC: Aleksandr Mishin <amishin@t-argos.ru>, Scott Branden
	<scott.branden@broadcom.com>, Broadcom internal kernel review list
	<bcm-kernel-feedback-list@broadcom.com>, Arnd Bergmann <arnd@arndb.de>, Greg
 Kroah-Hartman <gregkh@linuxfoundation.org>, <linux-kernel@vger.kernel.org>,
	<lvc-project@linuxtesting.org>
Subject: [PATCH] misc: bcm-vk: Fix NULL pointer dereference in case of buffer is not big enough in bcm_vk_read()
Date: Tue, 4 Jun 2024 22:58:20 +0300
Message-ID: <20240604195820.29426-1-amishin@t-argos.ru>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain

In case of entry is found but buffer is not big enough in bcm_vk_read()
found entry pointer remaining unset, but later dereferenced. This will lead
to NULL pointer dereference.
Fix this bug by moving pointer setting and correcting the conditions.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 88517757a829 ("misc: bcm-vk: replace usage of found with dedicated list iterator variable")
Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
---
 drivers/misc/bcm-vk/bcm_vk_msg.c | 38 +++++++++++++++++---------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c
index 1f42d1d5a630..566bb055fcf7 100644
--- a/drivers/misc/bcm-vk/bcm_vk_msg.c
+++ b/drivers/misc/bcm-vk/bcm_vk_msg.c
@@ -1031,11 +1031,11 @@ ssize_t bcm_vk_read(struct file *p_file,
 				    (iter->to_h_blks * VK_MSGQ_BLK_SIZE)) {
 					list_del(&iter->node);
 					atomic_dec(&ctx->pend_cnt);
-					entry = iter;
 				} else {
 					/* buffer not big enough */
 					rc = -EMSGSIZE;
 				}
+				entry = iter;
 				goto read_loop_exit;
 			}
 		}
@@ -1044,25 +1044,27 @@ ssize_t bcm_vk_read(struct file *p_file,
 	spin_unlock(&chan->pendq_lock);
 
 	if (entry) {
-		/* retrieve the passed down msg_id */
-		set_msg_id(&entry->to_h_msg[0], entry->usr_msg_id);
-		rsp_length = entry->to_h_blks * VK_MSGQ_BLK_SIZE;
-		if (copy_to_user(buf, entry->to_h_msg, rsp_length) == 0)
-			rc = rsp_length;
+		if (rc != -EMSGSIZE) {
+			/* retrieve the passed down msg_id */
+			set_msg_id(&entry->to_h_msg[0], entry->usr_msg_id);
+			rsp_length = entry->to_h_blks * VK_MSGQ_BLK_SIZE;
+			if (copy_to_user(buf, entry->to_h_msg, rsp_length) == 0)
+				rc = rsp_length;
 
-		bcm_vk_free_wkent(dev, entry);
-	} else if (rc == -EMSGSIZE) {
-		struct vk_msg_blk tmp_msg = entry->to_h_msg[0];
+			bcm_vk_free_wkent(dev, entry);
+		} else {
+			struct vk_msg_blk tmp_msg = entry->to_h_msg[0];
 
-		/*
-		 * in this case, return just the first block, so
-		 * that app knows what size it is looking for.
-		 */
-		set_msg_id(&tmp_msg, entry->usr_msg_id);
-		tmp_msg.size = entry->to_h_blks - 1;
-		if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) {
-			dev_err(dev, "Error return 1st block in -EMSGSIZE\n");
-			rc = -EFAULT;
+			/*
+			 * in this case, return just the first block, so
+			 * that app knows what size it is looking for.
+			 */
+			set_msg_id(&tmp_msg, entry->usr_msg_id);
+			tmp_msg.size = entry->to_h_blks - 1;
+			if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) {
+				dev_err(dev, "Error return 1st block in -EMSGSIZE\n");
+				rc = -EFAULT;
+			}
 		}
 	}
 	return rc;
-- 
2.30.2