Hi Hilda,
On Fri, Jun 28, 2024 at 6:16 AM Hilda Wu <hildawu@xxxxxxxxxxx> wrote:
>
> In some platform found unknown connection handle case when HFP. The
> unknown connection handle may affect SCO audio sound quality.
> After investigation, it was found the value of the unknown connection
> handle is actually a duplicated data.
>
> The duplicate data affected the unknown connection handle in some
> Realtek chip. This issue only occurs in (e)SCO, does not affect ACLs.
>
> This commit is to filter out the duplicate packet for avoiding influence
> SCO audio.
>
> Below btmon trace gives a better idea of what we're filtering.
> The following excerpts are part of SCO packets in the HCI log:
>
> > SCO Data RX: Handle 11 flags 0x00 dlen 72 #23327 [hci0] 132.343418
> 8c a3 55 4f 8a d5 56 e9 35 56 37 8d 55 87 53 55 ..UO..V.5V7.U.SU
> 59 66 d5 57 1d b5 54 00 01 08 ad 00 00 e0 10 00 Yf.W..T.........
> 00 00 85 c6 d5 60 e9 b5 52 94 6d 54 e4 9b 55 b1 .....`..R.mT..U.
> b6 d5 62 91 b5 57 84 6d 56 e4 5b 55 75 c6 d5 51 ..b..W.mV.[Uu..Q
> 2d b5 53 9a 6d 54 a5 1b -.S.mT..
> < SCO Data TX: Handle 11 flags 0x00 dlen 72 #23328 [hci0] 132.343600
> 01 c8 ad 00 00 aa db ba aa a9 72 b4 d9 5d af 14 ..........r..]..
> 53 0c 75 b0 a6 f3 8a 51 b3 54 17 b1 a6 d5 62 c5 S.u....Q.T....b.
> d5 6b 35 29 8d c5 1c 56 4c 24 96 9b 8d b5 d7 1a .k5)...VL$......
> b2 8d bc da 3b 8c 46 ae 1d 4d a4 04 01 f8 ad 00 ....;.F..M......
> 00 3d ec bb a9 98 8b 28 .=.....(
> > SCO Data RX: Handle 11 flags 0x00 dlen 72 #23329 [hci0] 132.353419
> 55 55 c6 d5 62 29 b5 57 b2 6d 54 00 01 38 ad 00 UU..b).W.mT..8..
> 00 e0 10 00 00 00 0b 00 d5 62 55 c6 57 b2 29 b5 .........bU.W.).
> 00 01 6d 54 00 00 38 ad 00 00 e0 10 00 00 00 92 ..mT..8.........
> 36 d5 5a ed b5 58 6c 6d 55 b3 1b 55 6b 26 d5 52 6.Z..XlmU..Uk&.R
> d1 b5 54 23 6d 56 82 db ..T#mV..
> < SCO Data TX: Handle 11 flags 0x00 dlen 72 #23330 [hci0] 132.353581
> 6d 5b be db 89 34 66 e9 fa 99 a6 6e e5 6d 9f 1a m[...4f....n.m..
> 1c 57 d2 66 92 63 98 99 a9 3b 8a 6c 3e 5b 5a 34 .W.f.c...;.l>[Z4
> a4 96 e2 21 21 8c f8 88 0f 3d e0 52 48 85 18 00 ...!!....=.RH...
> 01 08 ad 00 00 0c eb ba a9 a8 28 ca 9a d0 3c 33 ..........(...<3
> 45 4a f9 90 fb ca 4b 39 EJ....K9
> > SCO Data RX: Handle 2901 flags 0x0a dlen 54 #23331 [hci0] 132.373416
> d5 48 a9 b5 56 aa 6d 56 d2 db 55 75 36 d5 56 2d .H..V.mV..Uu6.V-
> b5 57 5b 6d 54 00 0b 00 48 01 c8 ad 00 00 e0 10 .W[mT...H.......
> 00 00 00 5e c6 d5 56 e1 b5 56 43 6d 55 ca db 55 ...^..V..VCmU..U
> 7d c6 d5 5b 31 b5
>
> This is HCI SCO data RX packets.
> The packet 23327 was a normal HCI SCO data RX packet.
> The packet 23329 was the abnormal HCI SCO data RX packet.
> The packet 23331 was the invalid connection handle case but the packet is
> affected by the packet 23329 abnormal HCI SCO Data RX packet.
>
> So this patch expects to filter the packet 23329 SCO data RX packet
> case. The packet 23329's connection handle (0x0B 00/11) and length
> (0x48/72) is normal.
> This btmon trace is SCO packets in USB alternate setting 3, payload
> length is 72 bytes that consist of three SCO data packets.
> The anomaly is due to the intermediate composed data.The duplicate
> data in the intermediate composition data, but it affects packet
> combination. Cause the system parses the next packet of the connection
> handle mistake that shows unknown connection handle messages.
>
> This commit can estimate and find out its abnormal rule to filter the
> duplicate packet out for avoiding influence.
> Check fragments and filter out the abnormal packet, and then it will
> not affect the system parsing of the connection handle subsequent.
>
> Signed-off-by: Alex Lu <alex_lu@xxxxxxxxxxxxxx>
> Signed-off-by: Hilda Wu <hildawu@xxxxxxxxxxx>
> ---
> drivers/bluetooth/btrtl.c | 49 +++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btrtl.h | 7 ++++++
> drivers/bluetooth/btusb.c | 8 +++++++
> 3 files changed, 64 insertions(+)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index f2f37143c454..f286654a8fae 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -1300,6 +1300,11 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
> btrtl_dev->project_id == CHIP_ID_8852C)
> set_bit(HCI_QUIRK_USE_MSFT_EXT_ADDRESS_FILTER, &hdev->quirks);
>
> + if (btrtl_dev->project_id == CHIP_ID_8822C ||
> + btrtl_dev->project_id == CHIP_ID_8852A ||
> + btrtl_dev->project_id == CHIP_ID_8852B)
> + btrealtek_set_flag(hdev, REALTEK_SCO_CLEAN_DUPLICATE_DATA);
> +
> hci_set_aosp_capable(hdev);
> break;
> default:
> @@ -1479,6 +1484,50 @@ int btrtl_get_uart_settings(struct hci_dev *hdev,
> }
> EXPORT_SYMBOL_GPL(btrtl_get_uart_settings);
>
> +int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb)
> +{
> + u8 *prev;
> + u8 tmp[8];
> + u32 *a;
> + u32 *b;
> + u16 i;
> + u8 *next;
> + u8 *start = skb->data;
> +
> + for (i = 0; i < 2; i++) {
> + prev = start + i * mps;
> + next = prev + mps;
> +
> + if (!memcmp(prev + 4, next + 2, 8))
> + continue;
> +
> + /* Check the current fragment with the previous one.
> + * If the current fragment is redundant but it is a little bit
> + * different from the previous, drop it.
> + * For example,
> + * 04 00 48 55 4E CB 55 52 80 95 55 07 XX XX ...
> + * 04 00 55 52 4E CB 55 07 80 95 XX XX XX XX ...
> + */
> + memcpy(tmp, prev + 4, 8);
> + a = (u32 *)(tmp);
> + b = (u32 *)(tmp + 4);
> + *a = swahw32(*a);
> + *b = swahw32(*b);
> +
> + if (next[0] == prev[0] && next[1] == prev[1] &&
> + !memcmp(next + 2, tmp, 8)) {
> + if (i == 0)
> + memcpy(start + mps, start + 2 * mps, mps);
> + skb_trim(skb, 2 * mps);
> + hci_skb_expect(skb) = mps;
> + return -EILSEQ;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(btrtl_validate_isoc_data);
NAK, the driver has no business deep inspecting the packets like that,
specially since you are not doing any length checks.
> MODULE_AUTHOR("Daniel Drake <drake@xxxxxxxxxxxx>");
> MODULE_DESCRIPTION("Bluetooth support for Realtek devices ver " VERSION);
> MODULE_VERSION(VERSION);
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> index a2d9d34f9fb0..3ddb691dff94 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -105,6 +105,7 @@ struct rtl_vendor_cmd {
>
> enum {
> REALTEK_ALT6_CONTINUOUS_TX_CHIP,
> + REALTEK_SCO_CLEAN_DUPLICATE_DATA,
>
> __REALTEK_NUM_FLAGS,
> };
> @@ -148,6 +149,7 @@ int btrtl_get_uart_settings(struct hci_dev *hdev,
> unsigned int *controller_baudrate,
> u32 *device_baudrate, bool *flow_control);
> void btrtl_set_driver_name(struct hci_dev *hdev, const char *driver_name);
> +int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb);
>
> #else
>
> @@ -195,4 +197,9 @@ static inline void btrtl_set_driver_name(struct hci_dev *hdev, const char *drive
> {
> }
>
> +static inline int btrtl_validate_isoc_data(u16 mps, struct sk_buff *skb)
> +{
> + return -EILSEQ;
> +}
> +
> #endif
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 2d7d47f9d007..2b66211eb02c 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1317,6 +1317,14 @@ static int btusb_recv_isoc(struct btusb_data *data, void *buffer, int count)
> }
>
> if (!hci_skb_expect(skb)) {
> + if (btrealtek_test_flag(data->hdev, REALTEK_SCO_CLEAN_DUPLICATE_DATA) &&
> + data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP &&
> + test_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags) &&
> + data->isoc_altsetting == 3 &&
> + skb->len == 3 * data->isoc_rx_ep->wMaxPacketSize &&
> + btrtl_validate_isoc_data(data->isoc_rx_ep->wMaxPacketSize, skb))
> + continue;
Can't you take a simpler approach and not use the ALT3 setting to
begin with? So instead of handling this via
REALTEK_SCO_CLEAN_DUPLICATE_DATA just don't set
BTUSB_USE_ALT3_FOR_WBS, if that means WBS cannot be used then so be it
since NBS is still better than a broken WBS.
> /* Complete frame */
> hci_recv_frame(data->hdev, skb);
> skb = NULL;
> --
> 2.34.1
>
--
Luiz Augusto von Dentz
