diff mbox series

Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers

Message ID 0018a41a-5a46-2db6-5df2-e1b026b81bae@gmail.com (mailing list archive)
State New, archived
Headers show
Series Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers | expand

Commit Message

Ismael Ferreras Morezuelas June 22, 2020, 9:07 p.m. UTC 
For some reason they tend to squat on the very first CSR/
Cambridge Silicon Radio VID/PID instead of paying fees.

This is an extremely common problem; the issue goes as back as 2013
and these devices are only getting more popular, even rebranded by
reputable vendors and sold by retailers everywhere.

So, at this point in time there are hundreds of modern dongles reusing
the ID of what originally was an early Bluetooth 1.1 controller.

Linux is the only place where they don't work due to spotty checks
in our detection code. It only covered a minimum subset.

So what's the big idea? Flip the check around, we know that there
are potentially going to be more. But CSR is done for and the
real device only worked in an old and narrow subset of the
protocol that has been amply superseded.

Known fake bcdDevices: 0x0100, 0x0134, 0x1915, 0x2520, 0x7558, 0x8891
IC markings on 0x7558: FR3191AHAL 749H15143 (???)

https://bugzilla.kernel.org/show_bug.cgi?id=60824

Fixes: 81cac64ba258ae (Deal with USB devices that are faking CSR vendor)
Reported-by: Michał Wiśniewski <brylozketrzyn@gmail.com>
Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
---
 drivers/bluetooth/btusb.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

 PS: I'm wondering how flexible the new 100-column limit really is,
     I tried to trim the comment a bit but it looked ugly. :)

Comments

Marcel Holtmann June 23, 2020, 6:08 a.m. UTC | #1 
Hi Ismael,

> For some reason they tend to squat on the very first CSR/
> Cambridge Silicon Radio VID/PID instead of paying fees.
> 
> This is an extremely common problem; the issue goes as back as 2013
> and these devices are only getting more popular, even rebranded by
> reputable vendors and sold by retailers everywhere.
> 
> So, at this point in time there are hundreds of modern dongles reusing
> the ID of what originally was an early Bluetooth 1.1 controller.
> 
> Linux is the only place where they don't work due to spotty checks
> in our detection code. It only covered a minimum subset.
> 
> So what's the big idea? Flip the check around, we know that there
> are potentially going to be more. But CSR is done for and the
> real device only worked in an old and narrow subset of the
> protocol that has been amply superseded.
> 
> Known fake bcdDevices: 0x0100, 0x0134, 0x1915, 0x2520, 0x7558, 0x8891
> IC markings on 0x7558: FR3191AHAL 749H15143 (???)
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=60824
> 
> Fixes: 81cac64ba258ae (Deal with USB devices that are faking CSR vendor)
> Reported-by: Michał Wiśniewski <brylozketrzyn@gmail.com>
> Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> ---
> drivers/bluetooth/btusb.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
> 
> PS: I'm wondering how flexible the new 100-column limit really is,
>    I tried to trim the comment a bit but it looked ugly. :)

it might be in general for Linus, but here lets try to keep it at 80.

> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 5f022e9cf..880fe46aa 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1739,9 +1739,22 @@ static int btusb_setup_csr(struct hci_dev *hdev)
> 
> 	rp = (struct hci_rp_read_local_version *)skb->data;
> 
> -	/* Detect controllers which aren't real CSR ones. */
> +	/* Detect a wide host of Chinese controllers that aren't CSR. Some of these clones even
> +	 * respond with the correct HCI manufacturer, and their bcdDevice tags are all over the place,
> +	 * which may be another good angle to look into if we really want to have really long quirk lists.
> +	 *
> +	 * Known fake bcdDevices: 0x0100, 0x0134, 0x1915, 0x2520, 0x7558, 0x8891
> +	 * IC markings on 0x7558: FR3191AHAL 749H15143 (???)
> +	 *
> +	 * But the main thing they have in common is that these are really popular low-cost
> +	 * options that support newer Bluetooth versions but rely on heavy VID/PID
> +	 * squatting of this poor old Bluetooth 1.1 device. Even sold as such.
> +	 */
> 	if (le16_to_cpu(rp->manufacturer) != 10 ||
> -	    le16_to_cpu(rp->lmp_subver) == 0x0c5c) {
> +	    le16_to_cpu(rp->lmp_subver) == 0x0c5c ||
> +	    le16_to_cpu(rp->hci_ver) >= BLUETOOTH_VER_1_2) {

This check will also catch actually Bluetooth 2.0 and later made devices from CSR.

> +		bt_dev_info(hdev, "CSR: Unbranded CSR clone detected; adding workaround");
> +
> 		/* Clear the reset quirk since this is not an actual
> 		 * early Bluetooth 1.1 device from CSR.
> 		 */
> @@ -1751,6 +1764,9 @@ static int btusb_setup_csr(struct hci_dev *hdev)
> 		 * stored link key handling and so just disable it.
> 		 */
> 		set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
> +	} else {
> +		/* Only apply these quirks to the actual, old CSR devices */
> +		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> 	}
> 
> 	kfree_skb(skb);
> @@ -3995,17 +4011,13 @@ static int btusb_probe(struct usb_interface *intf,
> 
> 	if (id->driver_info & BTUSB_CSR) {
> 		struct usb_device *udev = data->udev;
> -		u16 bcdDevice = le16_to_cpu(udev->descriptor.bcdDevice);
> 
> 		/* Old firmware would otherwise execute USB reset */
> -		if (bcdDevice < 0x117)
> +		if (le16_to_cpu(udev->descriptor.bcdDevice) < 0x117)
> 			set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);

Keep this as is.

> 
> 		/* Fake CSR devices with broken commands */
> -		if (bcdDevice <= 0x100 || bcdDevice == 0x134)
> -			hdev->setup = btusb_setup_csr;
> -
> -		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> +		hdev->setup = btusb_setup_csr;

Frankly, I rather add a switch statement and list all the known broken bcdDevice instead of trying to penalize real CSR devices.

Regards

Marcel
Ismael Ferreras Morezuelas June 23, 2020, 7:59 p.m. UTC | #2 
On 23/06/2020 8:08, Marcel Holtmann wrote:
> Hi Ismael,

Hi, Marcel. Thanks for reviewing my (first ever) patch.


>
>> PS: I'm wondering how flexible the new 100-column limit really is,
>>    I tried to trim the comment a bit but it looked ugly. :)
> 
> it might be in general for Linus, but here lets try to keep it at 80.
> 

Okay, understood. I'll keep in in mind for the second version. I was curious. 


>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 5f022e9cf..880fe46aa 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -1739,9 +1739,22 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>>
>> 	rp = (struct hci_rp_read_local_version *)skb->data;
>>
>> -	/* Detect controllers which aren't real CSR ones. */
>> +	/* Detect a wide host of Chinese controllers that aren't CSR. Some of these clones even
>> +	 * respond with the correct HCI manufacturer, and their bcdDevice tags are all over the place,
>> +	 * which may be another good angle to look into if we really want to have really long quirk lists.
>> +	 *
>> +	 * Known fake bcdDevices: 0x0100, 0x0134, 0x1915, 0x2520, 0x7558, 0x8891
>> +	 * IC markings on 0x7558: FR3191AHAL 749H15143 (???)
>> +	 *
>> +	 * But the main thing they have in common is that these are really popular low-cost
>> +	 * options that support newer Bluetooth versions but rely on heavy VID/PID
>> +	 * squatting of this poor old Bluetooth 1.1 device. Even sold as such.
>> +	 */
>> 	if (le16_to_cpu(rp->manufacturer) != 10 ||
>> -	    le16_to_cpu(rp->lmp_subver) == 0x0c5c) {
>> +	    le16_to_cpu(rp->lmp_subver) == 0x0c5c ||
>> +	    le16_to_cpu(rp->hci_ver) >= BLUETOOTH_VER_1_2) {
> 
> This check will also catch actually Bluetooth 2.0 and later made devices from CSR.

Yeah, you are right. I have been comparing HCI and lsusb dumps for this VID/PID
pair and I've found a much better way of distinguishing which is which.

Without breaking actual CSR chips. Turns out my dongle is legit.


>> +		bt_dev_info(hdev, "CSR: Unbranded CSR clone detected; adding workaround");
>> +
>> 		/* Clear the reset quirk since this is not an actual
>> 		 * early Bluetooth 1.1 device from CSR.
>> 		 */
>> @@ -1751,6 +1764,9 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>> 		 * stored link key handling and so just disable it.
>> 		 */
>> 		set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
>> +	} else {
>> +		/* Only apply these quirks to the actual, old CSR devices */
>> +		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>> 	}
>>
>> 	kfree_skb(skb);
>> @@ -3995,17 +4011,13 @@ static int btusb_probe(struct usb_interface *intf,
>>
>> 	if (id->driver_info & BTUSB_CSR) {
>> 		struct usb_device *udev = data->udev;
>> -		u16 bcdDevice = le16_to_cpu(udev->descriptor.bcdDevice);
>>
>> 		/* Old firmware would otherwise execute USB reset */
>> -		if (bcdDevice < 0x117)
>> +		if (le16_to_cpu(udev->descriptor.bcdDevice) < 0x117)
>> 			set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
> 
> Keep this as is.

Okay, makes sense. Keeps things tidy.

For the next version I think I'll keep all the quirk logic in the btusb_setup_csr()
function instead. As it will require to know bcdDevice, hci_rev, lmp_subver
and manufacturer in advance.


> 
>>
>> 		/* Fake CSR devices with broken commands */
>> -		if (bcdDevice <= 0x100 || bcdDevice == 0x134)
>> -			hdev->setup = btusb_setup_csr;
>> -
>> -		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>> +		hdev->setup = btusb_setup_csr;
> 
> Frankly, I rather add a switch statement and list all the known broken bcdDevice instead of trying to penalize real CSR devices.
> 
> Regards
> 
> Marcel
> 

Yeah, and I have also found a better list of bcdDevice elements. I looked a bit 
more in-depth and I have found out that there are actually three classes of 
controllers reusing the same 0A12:0001 VID/PID:

  * Old CSR Bluetooth 1.1 devices (BlueCore?):
      = bcdDevice < 0x117
      HCI_QUIRK_SIMULTANEOUS_DISCOVERY
      HCI_QUIRK_RESET_ON_CLOSE

  * New CSR Bluetooth devices CSR8510 A10 (BlueSoleil?):
      = bcdDevice with 0134 1915 1958 3164 4839 5276 7558 8891
      HCI_QUIRK_BROKEN_STORED_LINK_KEY

  * Unbranded CSR clone:
      = Their HCI chip uses a different manufacturer number;
        real CSR chips use manufacturer 10 and the HCIRevision
        and LMP Subversion always matches.
      No quirks, varies depending on the real manufacturer.


I'll post a more throughout explanation in the work-in-progress patch.
Thanks again for your time, I'll submit a second version in a jiffy.

Hopefully we can get this old issue sorted out without breaking anything. 

--swyter
diff mbox series

Patch

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 5f022e9cf..880fe46aa 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1739,9 +1739,22 @@  static int btusb_setup_csr(struct hci_dev *hdev)
 
 	rp = (struct hci_rp_read_local_version *)skb->data;
 
-	/* Detect controllers which aren't real CSR ones. */
+	/* Detect a wide host of Chinese controllers that aren't CSR. Some of these clones even
+	 * respond with the correct HCI manufacturer, and their bcdDevice tags are all over the place,
+	 * which may be another good angle to look into if we really want to have really long quirk lists.
+	 *
+	 * Known fake bcdDevices: 0x0100, 0x0134, 0x1915, 0x2520, 0x7558, 0x8891
+	 * IC markings on 0x7558: FR3191AHAL 749H15143 (???)
+	 *
+	 * But the main thing they have in common is that these are really popular low-cost
+	 * options that support newer Bluetooth versions but rely on heavy VID/PID
+	 * squatting of this poor old Bluetooth 1.1 device. Even sold as such.
+	 */
 	if (le16_to_cpu(rp->manufacturer) != 10 ||
-	    le16_to_cpu(rp->lmp_subver) == 0x0c5c) {
+	    le16_to_cpu(rp->lmp_subver) == 0x0c5c ||
+	    le16_to_cpu(rp->hci_ver) >= BLUETOOTH_VER_1_2) {
+		bt_dev_info(hdev, "CSR: Unbranded CSR clone detected; adding workaround");
+
 		/* Clear the reset quirk since this is not an actual
 		 * early Bluetooth 1.1 device from CSR.
 		 */
@@ -1751,6 +1764,9 @@  static int btusb_setup_csr(struct hci_dev *hdev)
 		 * stored link key handling and so just disable it.
 		 */
 		set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
+	} else {
+		/* Only apply these quirks to the actual, old CSR devices */
+		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 	}
 
 	kfree_skb(skb);
@@ -3995,17 +4011,13 @@  static int btusb_probe(struct usb_interface *intf,
 
 	if (id->driver_info & BTUSB_CSR) {
 		struct usb_device *udev = data->udev;
-		u16 bcdDevice = le16_to_cpu(udev->descriptor.bcdDevice);
 
 		/* Old firmware would otherwise execute USB reset */
-		if (bcdDevice < 0x117)
+		if (le16_to_cpu(udev->descriptor.bcdDevice) < 0x117)
 			set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
 
 		/* Fake CSR devices with broken commands */
-		if (bcdDevice <= 0x100 || bcdDevice == 0x134)
-			hdev->setup = btusb_setup_csr;
-
-		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
+		hdev->setup = btusb_setup_csr;
 	}
 
 	if (id->driver_info & BTUSB_SNIFFER) {