Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Apr 2023 11:55:17 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Manish Mandlik <mmandlik@google.com>
Cc:     Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        linux-bluetooth@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: [PATCH] Bluetooth: vhci: Fix info leak in force_devcd_write()
Message-ID: <634ab328-3ef2-43b7-8f81-8a48c7661427@kili.mountain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Precedence: bulk

There are a number of bugs here:

1) If "count" is less than sizeof(dump_data.data) then it copies
   uninitialized data.
2) If simple_write_to_buffer() returns -EFAULT then we run into a
   problem "ret < count" comparison.  "count" is an unsigned long so the
   comparison is type promoted to unsigned long and the negative returns
   become high positive values.  That also results in copying
   uninitialized data.
3) If "*ppos" is non-zero then the first part of the dump_data
   buffer is uninitialized.  Using copy_from_user() instead of
   simple_write_to_buffer() is more appropriate here.

Fixes: d5d5df6da0aa ("Bluetooth: Add vhci devcoredump support")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
From static analysis.  Untested.

 drivers/bluetooth/hci_vhci.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 691fe93b1976..40e2b9fa11a2 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -323,17 +323,21 @@ static ssize_t force_devcd_write(struct file *file, const char __user *user_buf,
 	struct hci_dev *hdev = data->hdev;
 	struct sk_buff *skb = NULL;
 	struct devcoredump_test_data dump_data;
+	size_t data_size;
 	int ret;
 
-	ret = simple_write_to_buffer(&dump_data, sizeof(dump_data), ppos,
-				     user_buf, count);
-	if (ret < count)
-		return ret;
+	if (count < offsetof(struct devcoredump_test_data, data) ||
+	    count > sizeof(dump_data))
+		return -EINVAL;
+
+	if (copy_from_user(&dump_data, user_buf, count))
+		return -EFAULT;
 
-	skb = alloc_skb(sizeof(dump_data.data), GFP_ATOMIC);
+	data_size = count - offsetof(struct devcoredump_test_data, data);
+	skb = alloc_skb(data_size, GFP_ATOMIC);
 	if (!skb)
 		return -ENOMEM;
-	skb_put_data(skb, &dump_data.data, sizeof(dump_data.data));
+	skb_put_data(skb, &dump_data.data, data_size);
 
 	hci_devcd_register(hdev, vhci_coredump, vhci_coredump_hdr, NULL);
 
-- 
2.39.1