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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CABBYNZL11npqO27DPjGz52X9zVD-5pqVwHEEVHY_aw1NH8GxjQ@mail.gmail.com>
Date: Fri, 27 Jun 2025 11:25:50 -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 v3] Bluetooth: HCI: Set extended advertising data synchronously

Hi Christian,

On Fri, Jun 27, 2025 at 6:09 AM Christian Eggers <ceggers@...i.de> wrote:
>
> 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?

Yeah, that is indeed a bug, in fact we can just leave for
hci_disable_ext_adv_instance_sync to detect if the instance is
enabled:

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 2e0e532384c3..13ebd1a380fd 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1228,7 +1228,7 @@ int hci_setup_ext_adv_instance_sync(struct
hci_dev *hdev, u8 instance)
         * Command Disallowed error, so we must first disable the
         * instance if it is active.
         */
-       if (adv && !adv->pending) {
+       if (adv) {
                err = hci_disable_ext_adv_instance_sync(hdev, instance);
                if (err)
                        return err;


>
> 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;
> >
> >
>
>
>
>


-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ