| 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: <4990184.OV4Wx5bFTl@n9w6sw14>
Date: Fri, 27 Jun 2025 12:09:38 +0200
From: Christian Eggers <ceggers@...i.de>
To: Marcel Holtmann <marcel@...tmann.org>, Johan Hedberg
<johan.hedberg@...il.com>, Luiz Augusto von Dentz <luiz.dentz@...il.com>,
Jaganath Kanakkassery <jaganath.k.os@...il.com>
CC: <linux-bluetooth@...r.kernel.org>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH v3] Bluetooth: HCI: Set extended advertising data synchronously
Hi Luiz,
after changing my test setup (I now only use Mesh, no "normal" advertising
anymore), I get many of these errors:
bluetooth-meshd[276]: @ MGMT Command: Mesh Send (0x0059) plen 40 {0x0001} [hci0] 43.846388
Address: 00:00:00:00:00:00 (OUI 00-00-00)
Addr Type: 2
Instant: 0x0000000000000000
Delay: 0
Count: 1
Data Length: 21
Data[21]: 142b003a8b6fe779bd4385a94fed0a9cf611880000
< HCI Command: LE Set Extended Advertising Parameters (0x08|0x0036) plen 25 #479 [hci0] 43.846505
Handle: 0x05
Properties: 0x0010
Use legacy advertising PDUs: ADV_NONCONN_IND
Min advertising interval: 1280.000 msec (0x0800)
Max advertising interval: 1280.000 msec (0x0800)
Channel map: 37, 38, 39 (0x07)
Own address type: Random (0x01)
Peer address type: Public (0x00)
Peer address: 00:00:00:00:00:00 (OUI 00-00-00)
Filter policy: Allow Scan Request from Any, Allow Connect Request from Any (0x00)
TX power: Host has no preference (0x7f)
Primary PHY: LE 1M (0x01)
Secondary max skip: 0x00
Secondary PHY: LE 1M (0x01)
SID: 0x00
Scan request notifications: Disabled (0x00)
> HCI Event: Command Complete (0x0e) plen 5 #480 [hci0] 43.847480
LE Set Extended Advertising Parameters (0x08|0x0036) ncmd 2
---> Status: Command Disallowed (0x0c)
TX power (selected): 0 dbm (0x00)
>From the btmon output it is obvious that advertising is not disabled before updating the parameters.
I added the following debug line in hci_setup_ext_adv_instance_sync():
printk(KERN_ERR "instance = %u, adv = %p, adv->pending = %d, adv->enabled = %d\n",
instance, adv, adv ? adv->pending : -1, adv ? adv->enabled : -1);
>From the debug output I see that adv->pending is still true (so advertising is not disabled
before setting the advertising params). After changing the check from
if (adv && !adv->pending) {
to
if (adv && adv->enabled) {
it seems to do the job correctly. What do you think?
regards,
Christian
On Friday, 27 June 2025, 09:05:08 CEST, Christian Eggers wrote:
> Currently, for controllers with extended advertising, the advertising
> data is set in the asynchronous response handler for extended
> adverstising params. As most advertising settings are performed in a
> synchronous context, the (asynchronous) setting of the advertising data
> is done too late (after enabling the advertising).
>
> Move setting of adverstising data from asynchronous response handler
> into synchronous context to fix ordering of HCI commands.
>
> 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
> v2: https://lore.kernel.org/linux-bluetooth/20250626115209.17839-1-ceggers@arri.de/
> ---
> v3: refactor: store adv_addr_type/tx_power within hci_set_ext_adv_params_sync()
>
> v2: convert setting of adv data into synchronous context (rather than moving
> more methods into asynchronous response handlers).
> - hci_set_ext_adv_params_sync: new method
> - hci_set_ext_adv_data_sync: move within source file (no changes)
> - hci_set_adv_data_sync: dito
> - hci_update_adv_data_sync: dito
> - hci_cc_set_ext_adv_param: remove (performed synchronously now)
>
> net/bluetooth/hci_event.c | 36 -------
> net/bluetooth/hci_sync.c | 207 ++++++++++++++++++++++++--------------
> 2 files changed, 130 insertions(+), 113 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 66052d6aaa1d..4d5ace9d245d 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2150,40 +2150,6 @@ static u8 hci_cc_set_adv_param(struct hci_dev *hdev, void *data,
> return rp->status;
> }
>
> -static u8 hci_cc_set_ext_adv_param(struct hci_dev *hdev, void *data,
> - struct sk_buff *skb)
> -{
> - struct hci_rp_le_set_ext_adv_params *rp = data;
> - struct hci_cp_le_set_ext_adv_params *cp;
> - 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_PARAMS);
> - if (!cp)
> - return rp->status;
> -
> - hci_dev_lock(hdev);
> - hdev->adv_addr_type = cp->own_addr_type;
> - if (!cp->handle) {
> - /* Store in hdev for instance 0 */
> - hdev->adv_tx_power = rp->tx_power;
> - } else {
> - adv_instance = hci_find_adv_instance(hdev, cp->handle);
> - if (adv_instance)
> - adv_instance->tx_power = rp->tx_power;
> - }
> - /* Update adv data as tx power is known now */
> - hci_update_adv_data(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)
> {
> @@ -4164,8 +4130,6 @@ static const struct hci_cc {
> HCI_CC(HCI_OP_LE_READ_NUM_SUPPORTED_ADV_SETS,
> hci_cc_le_read_num_adv_sets,
> 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_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..563614b53485 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1205,9 +1205,126 @@ static int hci_set_adv_set_random_addr_sync(struct hci_dev *hdev, u8 instance,
> sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> }
>
> +static int
> +hci_set_ext_adv_params_sync(struct hci_dev *hdev, struct adv_info *adv,
> + const struct hci_cp_le_set_ext_adv_params *cp,
> + struct hci_rp_le_set_ext_adv_params *rp)
> +{
> + struct sk_buff *skb;
> +
> + skb = __hci_cmd_sync(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS, sizeof(*cp),
> + cp, HCI_CMD_TIMEOUT);
> +
> + /* If command return a status event, skb will be set to -ENODATA */
> + if (skb == ERR_PTR(-ENODATA))
> + return 0;
> +
> + if (IS_ERR(skb)) {
> + bt_dev_err(hdev, "Opcode 0x%4.4x failed: %ld",
> + HCI_OP_LE_SET_EXT_ADV_PARAMS, PTR_ERR(skb));
> + return PTR_ERR(skb);
> + }
> +
> + if (skb->len != sizeof(*rp)) {
> + bt_dev_err(hdev, "Invalid response length for "
> + "HCI_OP_LE_SET_EXT_ADV_PARAMS: %u", skb->len);
> + kfree_skb(skb);
> + return -EIO;
> + }
> +
> + memcpy(rp, skb->data, sizeof(*rp));
> + kfree_skb(skb);
> +
> + if (!rp->status) {
> + hdev->adv_addr_type = cp->own_addr_type;
> + if (!cp->handle) {
> + /* Store in hdev for instance 0 */
> + hdev->adv_tx_power = rp->tx_power;
> + } else if (adv) {
> + adv->tx_power = rp->tx_power;
> + }
> + }
> +
> + return rp->status;
> +}
> +
> +static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
> +{
> + DEFINE_FLEX(struct hci_cp_le_set_ext_adv_data, pdu, data, length,
> + HCI_MAX_EXT_AD_LENGTH);
> + u8 len;
> + struct adv_info *adv = NULL;
> + int err;
> +
> + if (instance) {
> + adv = hci_find_adv_instance(hdev, instance);
> + if (!adv || !adv->adv_data_changed)
> + return 0;
> + }
> +
> + len = eir_create_adv_data(hdev, instance, pdu->data,
> + HCI_MAX_EXT_AD_LENGTH);
> +
> + pdu->length = len;
> + pdu->handle = adv ? adv->handle : instance;
> + pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE;
> + pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG;
> +
> + err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
> + struct_size(pdu, data, len), pdu,
> + HCI_CMD_TIMEOUT);
> + if (err)
> + return err;
> +
> + /* Update data if the command succeed */
> + if (adv) {
> + adv->adv_data_changed = false;
> + } else {
> + memcpy(hdev->adv_data, pdu->data, len);
> + hdev->adv_data_len = len;
> + }
> +
> + return 0;
> +}
> +
> +static int hci_set_adv_data_sync(struct hci_dev *hdev, u8 instance)
> +{
> + struct hci_cp_le_set_adv_data cp;
> + u8 len;
> +
> + memset(&cp, 0, sizeof(cp));
> +
> + len = eir_create_adv_data(hdev, instance, cp.data, sizeof(cp.data));
> +
> + /* There's nothing to do if the data hasn't changed */
> + if (hdev->adv_data_len == len &&
> + memcmp(cp.data, hdev->adv_data, len) == 0)
> + return 0;
> +
> + memcpy(hdev->adv_data, cp.data, sizeof(cp.data));
> + hdev->adv_data_len = len;
> +
> + cp.length = len;
> +
> + return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_ADV_DATA,
> + sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> +}
> +
> +int hci_update_adv_data_sync(struct hci_dev *hdev, u8 instance)
> +{
> + if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED))
> + return 0;
> +
> + if (ext_adv_capable(hdev))
> + return hci_set_ext_adv_data_sync(hdev, instance);
> +
> + return hci_set_adv_data_sync(hdev, instance);
> +}
> +
> int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
> {
> struct hci_cp_le_set_ext_adv_params cp;
> + struct hci_rp_le_set_ext_adv_params rp;
> bool connectable;
> u32 flags;
> bdaddr_t random_addr;
> @@ -1316,8 +1433,12 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
> cp.secondary_phy = HCI_ADV_PHY_1M;
> }
>
> - err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS,
> - sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> + err = hci_set_ext_adv_params_sync(hdev, adv, &cp, &rp);
> + if (err)
> + return err;
> +
> + /* Update adv data as tx power is known now */
> + err = hci_set_ext_adv_data_sync(hdev, cp.handle);
> if (err)
> return err;
>
> @@ -1822,79 +1943,6 @@ int hci_le_terminate_big_sync(struct hci_dev *hdev, u8 handle, u8 reason)
> sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> }
>
> -static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
> -{
> - DEFINE_FLEX(struct hci_cp_le_set_ext_adv_data, pdu, data, length,
> - HCI_MAX_EXT_AD_LENGTH);
> - u8 len;
> - struct adv_info *adv = NULL;
> - int err;
> -
> - if (instance) {
> - adv = hci_find_adv_instance(hdev, instance);
> - if (!adv || !adv->adv_data_changed)
> - return 0;
> - }
> -
> - len = eir_create_adv_data(hdev, instance, pdu->data,
> - HCI_MAX_EXT_AD_LENGTH);
> -
> - pdu->length = len;
> - pdu->handle = adv ? adv->handle : instance;
> - pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE;
> - pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG;
> -
> - err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
> - struct_size(pdu, data, len), pdu,
> - HCI_CMD_TIMEOUT);
> - if (err)
> - return err;
> -
> - /* Update data if the command succeed */
> - if (adv) {
> - adv->adv_data_changed = false;
> - } else {
> - memcpy(hdev->adv_data, pdu->data, len);
> - hdev->adv_data_len = len;
> - }
> -
> - return 0;
> -}
> -
> -static int hci_set_adv_data_sync(struct hci_dev *hdev, u8 instance)
> -{
> - struct hci_cp_le_set_adv_data cp;
> - u8 len;
> -
> - memset(&cp, 0, sizeof(cp));
> -
> - len = eir_create_adv_data(hdev, instance, cp.data, sizeof(cp.data));
> -
> - /* There's nothing to do if the data hasn't changed */
> - if (hdev->adv_data_len == len &&
> - memcmp(cp.data, hdev->adv_data, len) == 0)
> - return 0;
> -
> - memcpy(hdev->adv_data, cp.data, sizeof(cp.data));
> - hdev->adv_data_len = len;
> -
> - cp.length = len;
> -
> - return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_ADV_DATA,
> - sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> -}
> -
> -int hci_update_adv_data_sync(struct hci_dev *hdev, u8 instance)
> -{
> - if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED))
> - return 0;
> -
> - if (ext_adv_capable(hdev))
> - return hci_set_ext_adv_data_sync(hdev, instance);
> -
> - return hci_set_adv_data_sync(hdev, instance);
> -}
> -
> int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance,
> bool force)
> {
> @@ -6269,6 +6317,7 @@ static int hci_le_ext_directed_advertising_sync(struct hci_dev *hdev,
> struct hci_conn *conn)
> {
> struct hci_cp_le_set_ext_adv_params cp;
> + struct hci_rp_le_set_ext_adv_params rp;
> int err;
> bdaddr_t random_addr;
> u8 own_addr_type;
> @@ -6310,8 +6359,12 @@ static int hci_le_ext_directed_advertising_sync(struct hci_dev *hdev,
> if (err)
> return err;
>
> - err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS,
> - sizeof(cp), &cp, HCI_CMD_TIMEOUT);
> + err = hci_set_ext_adv_params_sync(hdev, NULL, &cp, &rp);
> + if (err)
> + return err;
> +
> + /* Update adv data as tx power is known now */
> + err = hci_set_ext_adv_data_sync(hdev, cp.handle);
> if (err)
> return err;
>
>
Powered by blists - more mailing lists