| 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: <CABBYNZ+cfFCzBMNBv6imodUG1twK5=MSwoVCnR8St_w9-HiU_w@mail.gmail.com>
Date: Wed, 25 Jun 2025 09:26:58 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Christian Eggers <ceggers@...i.de>
Cc: Marcel Holtmann <marcel@...tmann.org>, Johan Hedberg <johan.hedberg@...il.com>,
Jaganath Kanakkassery <jaganath.k.os@...il.com>, linux-bluetooth@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: HCI: Fix HCI command order for extended advertising
Hi Christian,
On Wed, Jun 25, 2025 at 9:05 AM Christian Eggers <ceggers@...i.de> wrote:
>
> For extended advertising capable controllers, hci_start_ext_adv_sync()
> at the moment synchronously calls SET_EXT_ADV_PARAMS [1],
> SET_ADV_SET_RAND_ADDR [2], SET_EXT_SCAN_RSP_DATA [3](optional) and
> SET_EXT_ADV_ENABLE [4]. After all synchronous commands are finished,
> SET_EXT_ADV_DATA is called from the async response handler of
> SET_EXT_ADV_PARAMS [5] (via hci_update_adv_data).
>
> So the current implementation sets the advertising data AFTER enabling
> the advertising instance. The BT Core specification explicitly allows
> for this [6]:
>
> > If advertising is currently enabled for the specified advertising set,
> > the Controller shall use the new data in subsequent extended
> > advertising events for this advertising set. If an extended
> > advertising event is in progress when this command is issued, the
> > Controller may use the old or new data for that event.
Ok, lets stop right here, if the controller deviates from the spec it
needs a quirk and not make the whole stack work around a bug in the
firmware.
> In case of the Realtek RTL8761BU chip (almost all contemporary BT USB
> dongles are built on it), updating the advertising data after enabling
> the instance produces (at least one) corrupted advertising message.
> Under normal conditions, a single corrupted advertising message would
> probably not attract much attention, but during MESH provisioning (via
> MGMT I/O / mesh_send(_sync)), up to 3 different messages (BEACON, ACK,
> CAPS) are sent within a loop which causes corruption of ALL provisioning
> messages.
>
> I have no idea whether this could be fixed in the firmware of the USB
> dongles (I didn't even find the chip on the Realtek homepage), but
> generally I would suggest changing the order of the HCI commands as this
> matches the command order for "non-extended adv capable" controllers and
> simply is more natural.
>
> This patch only considers advertising instances with handle > 0, I don't
> know whether this should be extended to further cases.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_sync.c#n1319
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_sync.c#n1204
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_sync.c#n1471
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_sync.c#n1469
> [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bluetooth/hci_event.c#n2180
> [6] https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/Core-60/out/en/host-controller-interface/host-controller-interface-functional-specification.html#UUID-d4f36cb5-f26c-d053-1034-e7a547ed6a13
>
> Signed-off-by: Christian Eggers <ceggers@...i.de>
> Fixes: a0fb3726ba55 ("Bluetooth: Use Set ext adv/scan rsp data if controller supports")
> Cc: stable@...r.kernel.org
> ---
> include/net/bluetooth/hci_core.h | 1 +
> include/net/bluetooth/hci_sync.h | 1 +
> net/bluetooth/hci_event.c | 33 +++++++++++++++++++++++++++++
> net/bluetooth/hci_sync.c | 36 ++++++++++++++++++++++++++------
> 4 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 9fc8f544e20e..8d37f127ddba 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -237,6 +237,7 @@ struct oob_data {
>
> struct adv_info {
> struct list_head list;
> + bool enable_after_set_ext_data;
> bool enabled;
> bool pending;
> bool periodic;
> diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
> index 5224f57f6af2..00eceffeec87 100644
> --- a/include/net/bluetooth/hci_sync.h
> +++ b/include/net/bluetooth/hci_sync.h
> @@ -112,6 +112,7 @@ int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance,
> int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance);
> int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance);
> int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance);
> +int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance);
> int hci_enable_advertising_sync(struct hci_dev *hdev);
> int hci_enable_advertising(struct hci_dev *hdev);
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 66052d6aaa1d..eb018d8a3c4b 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2184,6 +2184,37 @@ static u8 hci_cc_set_ext_adv_param(struct hci_dev *hdev, void *data,
> return rp->status;
> }
>
> +static u8 hci_cc_le_set_ext_adv_data(struct hci_dev *hdev, void *data,
> + struct sk_buff *skb)
> +{
> + struct hci_cp_le_set_ext_adv_data *cp;
> + struct hci_ev_status *rp = data;
> + struct adv_info *adv_instance;
> +
> + bt_dev_dbg(hdev, "status 0x%2.2x", rp->status);
> +
> + if (rp->status)
> + return rp->status;
> +
> + cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_EXT_ADV_DATA);
> + if (!cp)
> + return rp->status;
> +
> + hci_dev_lock(hdev);
> +
> + if (cp->handle) {
> + adv_instance = hci_find_adv_instance(hdev, cp->handle);
> + if (adv_instance) {
> + if (adv_instance->enable_after_set_ext_data)
> + hci_enable_ext_advertising(hdev, cp->handle);
> + }
> + }
> +
> + hci_dev_unlock(hdev);
> +
> + return rp->status;
> +}
> +
> static u8 hci_cc_read_rssi(struct hci_dev *hdev, void *data,
> struct sk_buff *skb)
> {
> @@ -4166,6 +4197,8 @@ static const struct hci_cc {
> sizeof(struct hci_rp_le_read_num_supported_adv_sets)),
> HCI_CC(HCI_OP_LE_SET_EXT_ADV_PARAMS, hci_cc_set_ext_adv_param,
> sizeof(struct hci_rp_le_set_ext_adv_params)),
> + HCI_CC_STATUS(HCI_OP_LE_SET_EXT_ADV_DATA,
> + hci_cc_le_set_ext_adv_data),
> HCI_CC_STATUS(HCI_OP_LE_SET_EXT_ADV_ENABLE,
> hci_cc_le_set_ext_adv_enable),
> HCI_CC_STATUS(HCI_OP_LE_SET_ADV_SET_RAND_ADDR,
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 1f8806dfa556..da0e39cce721 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1262,6 +1262,7 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
> hci_cpu_to_le24(adv->max_interval, cp.max_interval);
> cp.tx_power = adv->tx_power;
> cp.sid = adv->sid;
> + adv->enable_after_set_ext_data = true;
> } else {
> hci_cpu_to_le24(hdev->le_adv_min_interval, cp.min_interval);
> hci_cpu_to_le24(hdev->le_adv_max_interval, cp.max_interval);
> @@ -1456,6 +1457,23 @@ int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance)
> data, HCI_CMD_TIMEOUT);
> }
>
> +static int enable_ext_advertising_sync(struct hci_dev *hdev, void *data)
> +{
> + u8 instance = PTR_UINT(data);
> +
> + return hci_enable_ext_advertising_sync(hdev, instance);
> +}
> +
> +int hci_enable_ext_advertising(struct hci_dev *hdev, u8 instance)
> +{
> + if (!hci_dev_test_flag(hdev, HCI_ADVERTISING) &&
> + list_empty(&hdev->adv_instances))
> + return 0;
> +
> + return hci_cmd_sync_queue(hdev, enable_ext_advertising_sync,
> + UINT_PTR(instance), NULL);
> +}
> +
> int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance)
> {
> int err;
> @@ -1464,11 +1482,11 @@ int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance)
> if (err)
> return err;
>
> - err = hci_set_ext_scan_rsp_data_sync(hdev, instance);
> - if (err)
> - return err;
> -
> - return hci_enable_ext_advertising_sync(hdev, instance);
> + /* SET_EXT_ADV_DATA and SET_EXT_ADV_ENABLE are called in the
> + * asynchronous response chain of set_ext_adv_params in order to
> + * set the advertising data first prior enabling it.
> + */
Doing things asynchronously is known to create problems, which is why
we introduced the cmd_sync infra to handle a chain of commands like
this, so Id suggest sticking to the synchronous way, if the order
needs to be changed then use a quirk to detect it and then make sure
the instance is disabled on hci_set_ext_adv_data_sync and then
re-enable after updating it.
> + return hci_set_ext_scan_rsp_data_sync(hdev, instance);
> }
>
> int hci_disable_per_advertising_sync(struct hci_dev *hdev, u8 instance)
> @@ -1832,8 +1850,14 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
>
> if (instance) {
> adv = hci_find_adv_instance(hdev, instance);
> - if (!adv || !adv->adv_data_changed)
> + if (!adv)
> return 0;
> + if (!adv->adv_data_changed) {
> + if (adv->enable_after_set_ext_data)
> + hci_enable_ext_advertising_sync(hdev,
> + adv->handle);
> + return 0;
> + }
> }
>
> len = eir_create_adv_data(hdev, instance, pdu->data,
> --
> 2.43.0
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists