Bluetooth: vhci: Fix info leak in force_devcd_write()

Commit Message

Dan Carpenter April 6, 2023, 8:55 a.m. UTC

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 <[e-mail]>
---
From static analysis.  Untested.

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

Comments

[e-mail] April 6, 2023, 9:32 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=737502

---Test result---

Test Summary:
CheckPatch                    PASS      0.68 seconds
GitLint                       PASS      0.33 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      31.67 seconds
CheckAllWarning               PASS      35.20 seconds
CheckSparse                   PASS      39.42 seconds
CheckSmatch                   PASS      110.45 seconds
BuildKernel32                 PASS      30.78 seconds
TestRunnerSetup               PASS      438.26 seconds
TestRunner_l2cap-tester       PASS      15.87 seconds
TestRunner_iso-tester         PASS      15.60 seconds
TestRunner_bnep-tester        PASS      5.10 seconds
TestRunner_mgmt-tester        PASS      106.48 seconds
TestRunner_rfcomm-tester      PASS      8.10 seconds
TestRunner_sco-tester         PASS      7.45 seconds
TestRunner_ioctl-tester       PASS      8.67 seconds
TestRunner_mesh-tester        PASS      6.42 seconds
TestRunner_smp-tester         PASS      7.40 seconds
TestRunner_userchan-tester    PASS      5.32 seconds
IncrementalBuild              PASS      28.53 seconds



---
Regards,
Linux Bluetooth

[e-mail] April 14, 2023, 8:10 p.m. UTC | #2

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Thu, 6 Apr 2023 11:55:17 +0300 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - Bluetooth: vhci: Fix info leak in force_devcd_write()
    https://git.kernel.org/bluetooth/bluetooth-next/c/0b1900708232

You are awesome, thank you!

Patch

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);