linux-kernel - RE: [PATCH v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 22 May 2024 08:13:46 +0000
From: <Nobuaki.Tsunashima@...ineon.com>
To: <pmenzel@...gen.mpg.de>
CC: <marcel@...tmann.org>, <luiz.dentz@...il.com>,
	<linux-bluetooth@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<gargaditya08@...e.com>
Subject: RE: [PATCH v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER
 to CYW4373

Hi Paul,

Thanks for your comments.

> Please note, that the time on the system you sent the patch from is in the future:
>
>     Date: Wed, 22 May 2024 17:17:35 +0900

It was due to wrong time setting on my Linux system. I've corrected it.

> I forgot to add btbcm in the summary:
> Bluetooth: btbcm: …
Sure. I will add in next version.

>> CYW4373 ROM FW has an issue that it claims LE_Read_Transmit_Power 
>>  command as supported in a response of Read_Local_Supported_Command 
>>  command but rejects the LE_Read_Transmit_Power command with "Unknown HCI Command"
>>  status. Due to the issue, Bluetooth driver of 5.15 and later kernel 
>>  fails to hci up.
>
> As written in the other thread, it’d be great if you bisected the commit.
As I talked in another email, the issue happened after below commit. In this case, should I
add "Fixes: " tag?

>> Especially in USB i/f case, it would be difficult to download patch FW 
>> that includes Its fix unless hci is up.
>
>lowercase: its
Acked.

> Which firmware versions are fixed?
CYW4373A0_001.001.025.0081.* and later version are fixed.

>> The patch forces the driver to skip LE_Read_Transmit_Power Command 
>> when it detects CYW4373 with ROM FW build.
>
>Maybe add something like:
>
>The driver already contains infrastructure to apply the quirk, but currently it only
>supports DMI based matching. Add support to match by chip id and baseline, which ….
Acked.

>> --- a/drivers/bluetooth/btbcm.c
>> +++ b/drivers/bluetooth/btbcm.c
>> @@ -437,18 +437,48 @@ static const struct dmi_system_id disable_broken_read_transmit_power[] = {
>>       { }
>>   };
>>
>> +struct bcm_chip_version_table {
>> +     u8      chip_id;
>
> Please use one space. (Please also check the line below.)
Acked.

>> +     u16 baseline;
>
> Add a comment above the struct, what baseline means?
It means baseline version of patch FW. I will add a comment there.

>> +};
>> +#define BCM_ROMFW_BASELINE_NUM       0xFFFF
>> +static const struct bcm_chip_version_table disable_broken_read_transmit_power_by_chip_ver[] = {
>> +     {0x87, BCM_ROMFW_BASELINE_NUM}          /* CYW4373/4373E */
>
> Add one space after { and before }?
Acked.

>> +};
>> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 
>> +chip_id, u16 baseline) {
>> +     int i;
>> +     int table_size = 
>> +ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
>
> Use size_t?
Acked.

>>   static int btbcm_read_info(struct hci_dev *hdev)
>>   {
>>       struct sk_buff *skb;
>> +     u8 chip_id;
>> +     u16 baseline;
>>
>>       /* Read Verbose Config Version Info */
>>       skb = btbcm_read_verbose_config(hdev);
>>       if (IS_ERR(skb))
>>               return PTR_ERR(skb);
>> -
>> +     chip_id = skb->data[1];
>> +     baseline = skb->data[3] | (skb->data[4] << 8);
>>       bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
>>       kfree_skb(skb);
>>
>> +     /* Check Chip ID and disable broken Read LE Min/Max Tx Power */
>> +     if (btbcm_is_disable_broken_read_tx_power_by_chip_ver(chip_id, baseline))
>> +             set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, 
>> + &hdev->quirks);
>> +

> Commit 801b4c027b44 (Bluetooth: btbcm: disable read tx power for some Macs with the
> T2 Security chip) added the check in `btbcm_print_controller_features()`? No idea, where
> the best place is.
I wondered it too but finally decided setting the bit inside btbcm_read_info() to avoid
duplicated call of btbcm_read_verbose_config().

>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c 
>> index d31edad7a056..52561c8d8828 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -142,6 +142,10 @@ static const struct usb_device_id btusb_table[] = {
>>       { USB_VENDOR_AND_INTERFACE_INFO(0x04ca, 0xff, 0x01, 0x01),
>>         .driver_info = BTUSB_BCM_PATCHRAM },
>>
>> +     /* Cypress devices with vendor specific id */
>> +     { USB_VENDOR_AND_INTERFACE_INFO(0x04b4, 0xff, 0x01, 0x01),
>> +       .driver_info = BTUSB_BCM_PATCHRAM },
>> +
>
> Order 0x04b4 before 0x04ca?
Entire order of the table is not sorted but looks like random order.
The reason of the entry location is just before of Broadcom ID as our chip
was derived from them.

>       /* Broadcom devices with vendor specific id */
>       { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>         .driver_info = BTUSB_BCM_PATCHRAM },

> Kind regards,

> Paul

Best Regards,
Nobuaki Tsunashima