| 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
| ||
|
Message-ID: <1c4514c8-73a2-41d5-a776-2c8fbda32ffe@amlogic.com>
Date: Mon, 12 May 2025 19:55:36 +0800
From: Yang Li <yang.li@...ogic.com>
To: Pauli Virtanen <pav@....fi>, Marcel Holtmann <marcel@...tmann.org>,
Johan Hedberg <johan.hedberg@...il.com>,
Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc: linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS
and CIS
Hi,
> [ EXTERNAL EMAIL ]
>
> Hi,
>
> pe, 2025-05-09 kello 18:17 +0800, Yang Li via B4 Relay kirjoitti:
>> From: Yang Li <yang.li@...ogic.com>
>>
>> When the DUT acts as a sink device, and a BIS already exists,
>> creating a CIS connection can cause the kernel to incorrectly
>> reference the BIS socket. This occurs because the socket
>> lookup only checks for state == BT_LISTEN, without
>> distinguishing between BIS and CIS socket types.
>>
>> To fix this, match the destination address (dst addr) during
>> ISO socket lookup to differentiate between BIS and CIS sockets
>> properly.
> Does it work if you have both CIS and BIS established between the same
> two machines?
Yes, it works.
The DUT functions as a BIS sink, synchronizing with a BIS source to
receive BIS ISO data. Simultaneously, it establishes a CIS connection
with a K70 smartphone to receive CIS ISO data as well.
>
> Now that CIS_LINK and BIS_LINK are separate hci_conn types, it could
> make sense to introduce `__u8 hcon_type;` in iso_pinfo, maybe set in
> iso_connect/listen so that also the socket types won't be confused.
The |hcon_type| can only distinguish whether the connection type is BIS
or CIS, but it cannot be used in the |iso_get_sock| function to
differentiate between BIS and CIS sockets.
>
>> Link: https://github.com/bluez/bluez/issues/1224
>>
>> Signed-off-by: Yang Li <yang.li@...ogic.com>
>> ---
>> Changes in v2:
>> - Fix compilation errors
>> - Improved the problem description for clarity.
>> - Link to v1: https://lore.kernel.org/r/20250507-iso-v1-1-6f60d243e037@amlogic.com
>> ---
>> net/bluetooth/hci_event.c | 34 +++++++++++++++++++---------------
>> net/bluetooth/iso.c | 12 +++++++++---
>> 2 files changed, 28 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 66052d6aaa1d..6b26344ad69f 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -6413,6 +6413,8 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data,
>>
>> conn->sync_handle = le16_to_cpu(ev->handle);
>> conn->sid = HCI_SID_INVALID;
>> + conn->dst = ev->bdaddr;
>> + conn->dst_type = ev->bdaddr_type;
>>
>> mask |= hci_proto_connect_ind(hdev, &ev->bdaddr, BIS_LINK,
>> &flags);
>> @@ -6425,7 +6427,7 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data,
>> goto unlock;
>>
>> /* Add connection to indicate PA sync event */
>> - pa_sync = hci_conn_add_unset(hdev, BIS_LINK, BDADDR_ANY,
>> + pa_sync = hci_conn_add_unset(hdev, BIS_LINK, &ev->bdaddr,
>> HCI_ROLE_SLAVE);
> Do these make the update of conn->dst in iso_conn_ready() unnecessary?
>
> It should be documented somewhere what are the different types of
> BIS_LINK hci_conn that exist, and what are their invariants...
The pa_sync structure needs to retain the dst addr so that it can be
used later to match the correct BIS socket.
>
>> if (IS_ERR(pa_sync))
>> @@ -6456,13 +6458,6 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data,
>>
>> hci_dev_lock(hdev);
>>
>> - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags);
>> - if (!(mask & HCI_LM_ACCEPT))
>> - goto unlock;
>> -
>> - if (!(flags & HCI_PROTO_DEFER))
>> - goto unlock;
>> -
>> pa_sync = hci_conn_hash_lookup_pa_sync_handle
>> (hdev,
>> le16_to_cpu(ev->sync_handle));
>> @@ -6470,6 +6465,13 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data,
>> if (!pa_sync)
>> goto unlock;
>>
>> + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags);
>> + if (!(mask & HCI_LM_ACCEPT))
>> + goto unlock;
>> +
>> + if (!(flags & HCI_PROTO_DEFER))
>> + goto unlock;
>> +
> Commit message should explain what this reordering of *_ind after
> pa_sync lookup/update are for.
The iso_connect_ind() function needs to locate the appropriate BIS
socket by matching the destination address stored in pa_sync->dst.
This is just a position swap and does not introduce any logical changes.
>
>> if (ev->data_status == LE_PA_DATA_COMPLETE &&
>> !test_and_set_bit(HCI_CONN_PA_SYNC, &pa_sync->flags)) {
>> /* Notify iso layer */
>> @@ -6993,6 +6995,8 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data,
>> set_bit(HCI_CONN_PA_SYNC, &bis->flags);
>>
>> bis->sync_handle = conn->sync_handle;
>> + bis->dst = conn->dst;
>> + bis->dst_type = conn->dst_type;
>> bis->iso_qos.bcast.big = ev->handle;
>> memset(&interval, 0, sizeof(interval));
>> memcpy(&interval, ev->latency, sizeof(ev->latency));
>> @@ -7038,13 +7042,6 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data,
>>
>> hci_dev_lock(hdev);
>>
>> - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags);
>> - if (!(mask & HCI_LM_ACCEPT))
>> - goto unlock;
>> -
>> - if (!(flags & HCI_PROTO_DEFER))
>> - goto unlock;
>> -
>> pa_sync = hci_conn_hash_lookup_pa_sync_handle
>> (hdev,
>> le16_to_cpu(ev->sync_handle));
>> @@ -7054,6 +7051,13 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data,
>>
>> pa_sync->iso_qos.bcast.encryption = ev->encryption;
>>
>> + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags);
>> + if (!(mask & HCI_LM_ACCEPT))
>> + goto unlock;
>> +
>> + if (!(flags & HCI_PROTO_DEFER))
>> + goto unlock;
>> +
>>
>> /* Notify iso layer */
>> hci_connect_cfm(pa_sync, 0);
>>
>> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
>> index 6e2c752aaa8f..1dc233f04dbe 100644
>> --- a/net/bluetooth/iso.c
>> +++ b/net/bluetooth/iso.c
>> @@ -641,11 +641,12 @@ static struct sock *iso_get_sock(bdaddr_t *src, bdaddr_t *dst,
>> continue;
>>
>> /* Exact match. */
>> - if (!bacmp(&iso_pi(sk)->src, src)) {
>> + if (!bacmp(&iso_pi(sk)->src, src)
>> + && !bacmp(&iso_pi(sk)->dst, dst)
>> + ){
> Code style issues here.
Will do it.
>> sock_hold(sk);
>> break;
>> }
>> -
>> /* Closest match */
>> if (!bacmp(&iso_pi(sk)->src, BDADDR_ANY)) {
>> if (sk1)
>> @@ -1962,7 +1963,7 @@ static void iso_conn_ready(struct iso_conn *conn)
>> }
>>
>> if (!parent)
>> - parent = iso_get_sock(&hcon->src, BDADDR_ANY,
>> + parent = iso_get_sock(&hcon->src, &hcon->dst,
>> BT_LISTEN, NULL, NULL);
> I think the code here would be more clear if it's refactored to handle
> hcon->type == CIS_LINK and hcon->type == BIS_LINK with explicitly
> separate code path.
>
> What happens here if we have a BIS listener socket for `dst`, and `dst`
> initiates a CIS connection? Won't the CIS connection get resolved to
> the BIS listener socket?
>
> IIUC CIS listeners always have dst == BDADDR_ANY. BIS listeners have
> dst != BDADDR_ANY.
>
> Perhaps there could also be `__u8 hcon_type` in iso_pinfo that gets set
> to CIS_LINK or BIS_LINK in iso_connect/listen.
I completely agree with your point that |hconn->type| should be
specified for both |bis_listen| and |cis_listen|. However, I believe
that the destination address (|dst addr|) should also be provided. Since
a CIS connection is based on an LE connection, the |dst addr| must
already be known at the time of listening. Similarly, for BIS listening,
the |dst addr| can be determined when the BIG Info is received.
Therefore, it is possible to know the |dst addr| at that point as well.
Moreover, if multiple BIS streams are to be supported simultaneously,
relying solely on |hconn->type| is not sufficient.
So, if we can set the |dst addr| during the |iso_sock_listen()| call,
there would be no need to assign it later in the |*_connect_ind| functions.
>> if (!parent)
>> @@ -2203,6 +2204,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
>> } else {
>> sk = iso_get_sock(&hdev->bdaddr, BDADDR_ANY,
>> BT_LISTEN, NULL, NULL);
>> + if (!sk)
>> + sk = iso_get_sock(&hdev->bdaddr, bdaddr,
>> + BT_LISTEN, NULL, NULL);
>> + else
>> + iso_pi(sk)->dst = *bdaddr;
> This updates the listener socket dst address with that of the
> connecting device? I think what is set in bind() shouldn't be modified
> later on.
>
> Isn't this wrong for CIS, won't it block connecting another device?
Yes, exactly. So far, I haven't found a better approach other than
updating the CIS socket's |dst addr| within the |*_connect_ind| function.
Thanks~
>
>> }
>>
>> done:
>>
>> ---
>> base-commit: f3daca9b490154fbb0459848cc2ed61e8367bddc
>> change-id: 20250506-iso-6515893b5bb3
>>
>> Best regards,
> --
> Pauli Virtanen
Powered by blists - more mailing lists