2023-09-20 19:23:21

by Arkadiusz Bokowy

[permalink] [raw]
Subject: [PATCH] Bluetooth: vhci: Fix race when opening vhci device

When the vhci device is opened in the two-step way, i.e.: open device
then write a vendor packet with requested controller type, the device
shall respond with a vendor packet which includes HCI index of created
interface.

When the virtual HCI is created, the host sends a reset request to the
controller. This request is processed by the vhci_send_frame() function.
However, this request is send by a different thread, so it might happen
that this HCI request will be received before the vendor response is
queued in the read queue. This results in the HCI vendor response and
HCI reset request inversion in the read queue which leads to improper
behavior of btvirt:

> dmesg
[1754256.640122] Bluetooth: MGMT ver 1.22
[1754263.023806] Bluetooth: MGMT ver 1.22
[1754265.043775] Bluetooth: hci1: Opcode 0x c03 failed: -110

In order to synchronize vhci two-step open/setup process with virtual
HCI initialization, this patch adds internal lock when queuing data in
the vhci_send_frame() function.

Signed-off-by: Arkadiusz Bokowy <[email protected]>
---
drivers/bluetooth/hci_vhci.c | 3 +++
net/bluetooth/hci_sync.c | 4 ++--
2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 40e2b9fa11a2..f3892e9ce800 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -74,7 +74,10 @@ static int vhci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
struct vhci_data *data = hci_get_drvdata(hdev);

memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+
+ mutex_lock(&data->open_mutex);
skb_queue_tail(&data->readq, skb);
+ mutex_unlock(&data->open_mutex);

wake_up_interruptible(&data->read_wait);
return 0;
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 3640d73f9595..2a7d432436a2 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -152,7 +152,7 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
struct sk_buff *skb;
int err = 0;

- bt_dev_dbg(hdev, "Opcode 0x%4x", opcode);
+ bt_dev_dbg(hdev, "Opcode 0x%4.4x", opcode);

hci_req_init(&req, hdev);

@@ -248,7 +248,7 @@ int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
skb = __hci_cmd_sync_sk(hdev, opcode, plen, param, event, timeout, sk);
if (IS_ERR(skb)) {
if (!event)
- bt_dev_err(hdev, "Opcode 0x%4x failed: %ld", opcode,
+ bt_dev_err(hdev, "Opcode 0x%4.4x failed: %ld", opcode,
PTR_ERR(skb));
return PTR_ERR(skb);
}
--
2.34.1


2023-09-20 21:49:55

by bluez.test.bot

[permalink] [raw]
Subject: RE: Bluetooth: vhci: Fix race when opening vhci device

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=785987

---Test result---

Test Summary:
CheckPatch PASS 0.79 seconds
GitLint PASS 0.24 seconds
SubjectPrefix PASS 0.08 seconds
BuildKernel PASS 35.89 seconds
CheckAllWarning PASS 44.57 seconds
CheckSparse PASS 47.86 seconds
CheckSmatch PASS 125.39 seconds
BuildKernel32 PASS 34.96 seconds
TestRunnerSetup PASS 560.66 seconds
TestRunner_l2cap-tester PASS 34.67 seconds
TestRunner_iso-tester PASS 68.10 seconds
TestRunner_bnep-tester PASS 11.73 seconds
TestRunner_mgmt-tester PASS 251.17 seconds
TestRunner_rfcomm-tester PASS 19.18 seconds
TestRunner_sco-tester PASS 22.19 seconds
TestRunner_ioctl-tester PASS 21.92 seconds
TestRunner_mesh-tester PASS 15.76 seconds
TestRunner_smp-tester PASS 16.69 seconds
TestRunner_userchan-tester PASS 13.02 seconds
IncrementalBuild PASS 35.12 seconds



---
Regards,
Linux Bluetooth

2023-09-20 21:58:59

by Arkadiusz Bokowy

[permalink] [raw]
Subject: [PATCH BlueZ] vhci: Check whether vhci open setup succeeded

Due to race condition in the vhci kernel driver, we might read not a
vendor response packet, but a HCI reset command. This extra check will
ensure that kernel driver behaves correctly. Otherwise, the HCI setup
process will fail, because our controller will not respond to "missing"
HCI reset command. In result the virtual HCI will be DOWN and without
initialized Bluetooth address, e.g:

> hciconfig
hci2: Type: Primary Bus: Virtual
BD Address: 00:AA:01:01:00:02 ACL MTU: 192:1 SCO MTU: 0:0
UP RUNNING
RX bytes:0 acl:0 sco:0 events:66 errors:0
TX bytes:3086 acl:0 sco:0 commands:66 errors:0

hci1: Type: Primary Bus: Virtual
BD Address: 00:00:00:00:00:00 ACL MTU: 0:0 SCO MTU: 0:0
DOWN
RX bytes:0 acl:0 sco:0 events:0 errors:0
TX bytes:8 acl:0 sco:0 commands:1 errors:0

> dmesg
[1754256.640122] Bluetooth: MGMT ver 1.22
[1754263.023806] Bluetooth: MGMT ver 1.22
[1754265.043775] Bluetooth: hci1: Opcode 0x c03 failed: -110
---
emulator/vhci.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/emulator/vhci.c b/emulator/vhci.c
index 7b363009a..355ab6389 100644
--- a/emulator/vhci.c
+++ b/emulator/vhci.c
@@ -122,14 +122,16 @@ struct vhci *vhci_open(uint8_t type)
break;
}

- if (write(fd, &req, sizeof(req)) < 0) {
+ if (write(fd, &req, sizeof(req)) != sizeof(req)) {
close(fd);
return NULL;
}

memset(&rsp, 0, sizeof(rsp));

- if (read(fd, &rsp, sizeof(rsp)) < 0) {
+ if (read(fd, &rsp, sizeof(rsp)) != sizeof(rsp) ||
+ rsp.pkt_type != HCI_VENDOR_PKT ||
+ rsp.opcode != req.opcode) {
close(fd);
return NULL;
}
--
2.39.2