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 <[email protected]>
---
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
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
Hello:
This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html