Re: [PATCH] Bluetooth: btrtl: fix duplicate SCO packet

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux