Re: Security block and Bluez - connection issue with Android

[fabien dvlt <fabiendvlt@gmail.com>]

Thank you both for your reply. Since you and Luiz answered, I have
been trying to do a better workaround at USB/HCI level instead of
L2CAP.

I understand this issue is more related to the firmware implementation
but for those who really need it, I think the patch below is a better
approach than the first I have submitted.

It will start queueing ACL packets when link key exchange begins until
the next HCI event. It still have some limitations I wish to fix soon
(see patch below).

I have a question about what I have read in bluetooth specifications:
 
 Version 5.0, Vol 2, Part C, - 4.2.5.1 Encryption Mode

   "ACL-U logical link traffic shall only be resumed after the attempt
   to encrypt or decrypt the logical transport is completed"

I was wondering if this would guarantee that no unencrypted connection
request could be forwarded to other layer once the link key exchange
has started.

Thank you

---

From bd17d5e519c2ad76ef1761b2d066d98ec4c723d1 Mon Sep 17 00:00:00 2001
From: fabien filhol <fabien.filhol@devialet.com>
Date: Wed, 3 Oct 2018 12:06:30 +0200
Subject: [PATCH] Bluetooth: btusb: Fix race condition between BT events and
 ACL

Workaround for a race condition observed with Android phones which makes
fail some connections (30% with a Oneplus 6).

After link key reply it is expected that HCI_EV_ENCRYPT_CHANGE is received
before any ACL connection request or it will be dropped by bluez L2CAP
layer.

The events are received from usb interrupt transfer and ACL packet from usb
bulk transfer. I guess the bluetooth controller does not handle correctly
write ordering between those two transfer types.

The workaround will start queueing ACL packets when link key are exchanged until
the next real event (other than HCI_EV_CMD_COMPLETE) is received, it should
be HCI_EV_ENCRYPT_CHANGE.

LIMITATIONS:

An unencrypted connection request forwarded by the controller at
"ACL queueing time" could pass. Bluetooth specification says that ACL
transfer is paused until encryption is setup so I hope it is safe to queue
ACL between HCI_EV_LINK_KEY_REQ and HCI_EV_CMD_COMPLETE.

With concurrent connections from multiple bluetooth devices the workaround
could not work as any event from any device would flush the queue.
---
 drivers/bluetooth/btusb.c | 50 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index bff67c5a5fe7..c55096fa8c4e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -406,6 +406,9 @@ struct btusb_data {
 	struct sk_buff *acl_skb;
 	struct sk_buff *sco_skb;
 
+	struct sk_buff_head acl_deferred_q;
+	int acl_deferred;
+
 	struct usb_endpoint_descriptor *intr_ep;
 	struct usb_endpoint_descriptor *bulk_tx_ep;
 	struct usb_endpoint_descriptor *bulk_rx_ep;
@@ -449,6 +452,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
 {
 	struct sk_buff *skb;
 	int err = 0;
+	int event = HCI_OP_NOP;
 
 	spin_lock(&data->rxlock);
 	skb = data->evt_skb;
@@ -488,6 +492,9 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
 		}
 
 		if (!hci_skb_expect(skb)) {
+			struct hci_event_hdr *hdr = (struct hci_event_hdr *)skb->data;
+			event = hdr->evt;
+
 			/* Complete frame */
 			data->recv_event(data->hdev, skb);
 			skb = NULL;
@@ -495,6 +502,39 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
 	}
 
 	data->evt_skb = skb;
+
+	/*
+	 * Workaround for a race condition observed with Android phones. After link
+	 * key reply it is expected that HCI_EV_ENCRYPT_CHANGE is received before
+	 * any ACL connection request or it will be dropped by bluez L2CAP layer.
+	 *
+	 * The events are received from usb interrupt transfer and ACL packet from
+	 * usb bulk transfer. I guess the bluetooth controller does not handle
+	 * correctly write ordering between those two transfer types.
+	 *
+	 * The workaround will start queueing ACL packets when link key are exchanged
+	 * until the next real event (not an HCI_EV_CMD_COMPLETE) is received, it
+	 * should be HCI_EV_ENCRYPT_CHANGE.
+	 *
+	 * LIMITATIONS:
+	 *
+	 * An unencrypted connection request forwarded by the controller at
+	 * "ACL queueing time" would pass. Bluetooth specification says that ACL
+	 * transfer is paused until encryption is setup so I hope it is safe to queue
+	 * ACL between HCI_EV_LINK_KEY_REQ and HCI_EV_CMD_COMPLETE.
+	 *
+	 * With concurrent connections from multiple bluetooth devices the workaround
+	 * could not work as any event from any device would flush the queue.
+	 */
+	if (event == HCI_EV_LINK_KEY_REQ) {
+		data->acl_deferred = 1;
+	} else if (data->acl_deferred && event != HCI_EV_CMD_COMPLETE) {
+		while ((skb = skb_dequeue(&data->acl_deferred_q)))
+			hci_recv_frame(data->hdev, skb);
+		skb = NULL;
+		data->acl_deferred = 0;
+	}
+
 	spin_unlock(&data->rxlock);
 
 	return err;
@@ -546,7 +586,10 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
 
 		if (!hci_skb_expect(skb)) {
 			/* Complete frame */
-			hci_recv_frame(data->hdev, skb);
+			if (data->acl_deferred)
+				skb_queue_tail(&data->acl_deferred_q, skb);
+			else
+				hci_recv_frame(data->hdev, skb);
 			skb = NULL;
 		}
 	}
@@ -2815,6 +2858,9 @@ static int btusb_probe(struct usb_interface *intf,
 	data->udev = interface_to_usbdev(intf);
 	data->intf = intf;
 
+	data->acl_deferred = 0;
+	skb_queue_head_init(&data->acl_deferred_q);
+
 	INIT_WORK(&data->work, btusb_work);
 	INIT_WORK(&data->waker, btusb_waker);
 	init_usb_anchor(&data->deferred);
@@ -3051,6 +3097,8 @@ static void btusb_disconnect(struct usb_interface *intf)
 	if (!data)
 		return;
 
+	skb_queue_purge(&data->acl_deferred_q);
+
 	hdev = data->hdev;
 	usb_set_intfdata(data->intf, NULL);
 
-- 
2.17.1


On Wed, Sep 26, 2018 at 02:28:01PM +0300, Johan Hedberg wrote:
> Hi Fabien,
> 
> On Tue, Sep 25, 2018, fabien dvlt wrote:
> > > ACL Data RX: Handle 13 flags 0x02 dlen 12           #198 [hci0] 21.813116
> >       L2CAP: Connection Request (0x02) ident 7 len 4
> >         PSM: 25 (0x0019)
> >         Source CID: 75
> > > HCI Event: Encryption Change (0x08) plen 4          #199 [hci0] 21.813155
> >         Status: Success (0x00)
> >         Handle: 13
> >         Encryption: Enabled with AES-CCM (0x02)
> > < ACL Data TX: Handle 13 flags 0x00 dlen 16           #200 [hci0]
> >       L2CAP: Connection Response (0x03) ident 7 len 8
> >         Destination CID: 0
> >         Source CID: 75
> >         Result: Connection refused - security block (0x0003)
> >         Status: No further information available (0x0000)
> 
> This looks like the well-known race condition for ACL data and HCI
> events on USB where the two come through different endpoints. From the
> host perspective there's not much we can do since we can't make
> assumptions that the connection request was sent over an encrypted
> connection if we haven't seen the encryption change request at that
> point.
> 
> Johan