[PATCH] Bluetooth: vhci: Fix race when opening vhci device

[PATCH] Bluetooth: vhci: Fix race when opening vhci device

[Arkadiusz Bokowy]

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 <arkadiusz.bokowy@gmail.com>
---
 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

[PATCH BlueZ] vhci: Check whether vhci open setup succeeded

[Arkadiusz Bokowy]

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/hciemu.c | 2 +-
 emulator/vhci.c   | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/emulator/hciemu.c b/emulator/hciemu.c
index 25874ded5..e53fec0a2 100644
--- a/emulator/hciemu.c
+++ b/emulator/hciemu.c
@@ -313,7 +313,7 @@ static struct hciemu_client *hciemu_client_new(struct hciemu *hciemu,
 	if (!client)
 		return NULL;
 
-	client->dev = btdev_create(hciemu->btdev_type, id++);
+	client->dev = btdev_create(hciemu->btdev_type, id);
 	if (!client->dev) {
 		free(client);
 		return NULL;
diff --git a/emulator/vhci.c b/emulator/vhci.c
index 7b363009a..80e1825f3 100644
--- a/emulator/vhci.c
+++ b/emulator/vhci.c
@@ -122,14 +122,15 @@ 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

RE: Bluetooth: vhci: Fix race when opening vhci device

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

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

RE: [BlueZ] vhci: Check whether vhci open setup succeeded

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

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

---Test result---

Test Summary:
CheckPatch                    FAIL      0.67 seconds
GitLint                       PASS      0.30 seconds
BuildEll                      PASS      27.52 seconds
BluezMake                     PASS      781.14 seconds
MakeCheck                     PASS      12.21 seconds
MakeDistcheck                 PASS      160.15 seconds
CheckValgrind                 PASS      254.18 seconds
CheckSmatch                   PASS      345.37 seconds
bluezmakeextell               PASS      104.92 seconds
IncrementalBuild              PASS      649.07 seconds
ScanBuild                     PASS      1031.05 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ] vhci: Check whether vhci open setup succeeded
WARNING:LONG_LINE: line length of 88 exceeds 80 columns
#144: FILE: emulator/vhci.c:133:
+			!(rsp.pkt_type == HCI_VENDOR_PKT && rsp.opcode == req.opcode)) {

/github/workspace/src/src/13393018.patch total: 0 errors, 1 warnings, 25 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13393018.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




---
Regards,
Linux Bluetooth

[PATCH BlueZ] vhci: Check whether vhci open setup succeeded

[Arkadiusz Bokowy]

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

RE: [BlueZ] vhci: Check whether vhci open setup succeeded

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.55 seconds
GitLint                       PASS      0.38 seconds
BuildEll                      PASS      28.85 seconds
BluezMake                     PASS      907.25 seconds
MakeCheck                     PASS      12.89 seconds
MakeDistcheck                 PASS      161.41 seconds
CheckValgrind                 PASS      264.06 seconds
CheckSmatch                   PASS      358.21 seconds
bluezmakeextell               PASS      108.27 seconds
IncrementalBuild              PASS      730.15 seconds
ScanBuild                     PASS      1093.08 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ] vhci: Check whether vhci open setup succeeded

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed, 20 Sep 2023 20:43:13 +0200 you wrote:
> 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:
> 
> [...]

Here is the summary with links:
  - [BlueZ] vhci: Check whether vhci open setup succeeded
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=a2d47ef05226

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html