| 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: <9911499.eNJFYEL58v@n9w6sw14>
Date: Wed, 25 Jun 2025 16:46:15 +0200
From: Christian Eggers <ceggers@...i.de>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
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 Luiz,
On Wednesday, 25 June 2025, 15:26:58 CEST, Luiz Augusto von Dentz wrote:
> 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.
I generally agree! In this particular case, I think that the current order of
advertising commands may be the result of "random" and was probably not intended this
way. While the command order of the "legacy" advertising commands looks perfectly
logical for me, the order of the "extended" commands seems to be broken by setting
the advertising data in the asynchronous response handler of set_ext_adv_params.
>
> > 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.
Directly after creation, the instance is disabled (which is fine). In my
opinion, the problem is then caused by enabling the instance _before_ setting
the advertisement data.
If the synchronous API is preferred, setting the advertisement data should
probably also be done synchronously (e.g. by calling hci_set_ext_adv_data_sync()
from hci_start_ext_adv_sync() rather than calling hci_update_adv_data() from
hci_cc_set_ext_adv_param()). But I guess that the "tx power" value is only
known after hci_cc_set_ext_adv_param() has been run (queued?) and this is probably
too late for the synchronous stuff called by hci_start_ext_adv_sync().
>
> > + 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
> >
>
>
>
regards,
Christian
regards,
Christian
Powered by blists - more mailing lists