| 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: <CABBYNZLfDqh=49qtC2M6F+f+rmgZ-hQGnABseqWyk0H4QDGTqQ@mail.gmail.com>
Date: Thu, 26 Jun 2025 08:57:22 -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 v2] Bluetooth: HCI: Set extended advertising data synchronously
Hi Christian,
On Thu, Jun 26, 2025 at 7:52 AM Christian Eggers <ceggers@...i.de> 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
> v1: https://lore.kernel.org/linux-bluetooth/20250625130510.18382-1-ceggers@arri.de/
> ---
> 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)
>
> On Wednesday, 25 June 2025, 15:26:58 CEST, Luiz Augusto von Dentz wrote:
> > That said for the likes of MGMT_OP_ADD_EXT_ADV_DATA you will still
> > need to detect if the instance has already been enabled then do
> > disable/re-enable logic if the quirk is set.
>
> The critical opcode (HCI_OP_LE_SET_EXT_ADV_DATA) is only used in
> hci_set_ext_adv_data_sync(). Two of the callers already ensure that
> the advertising instance is disabled, so only hci_update_adv_data_sync()
> may need a quirk. I suggest doing this in a separate patch.
>
> regards,
> Christian
>
> net/bluetooth/hci_event.c | 36 -------
> net/bluetooth/hci_sync.c | 209 ++++++++++++++++++++++++--------------
> 2 files changed, 132 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..2a09b2cb983e 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1205,9 +1205,116 @@ 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,
> + 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);
> +
> + 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 +1423,20 @@ 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, &cp, &rp);
> + if (err)
> + return err;
> +
> + hdev->adv_addr_type = 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;
> + }
We can probably move the above code into hci_set_ext_adv_params_sync
so we guarantee the tx_power is updated whenever it is used, if there
are differences between the likes of directed advertisements, etc,
that can probably be handled internally as well, although I think it
doesn't seem to need a special handling since we restrict directected
advertisements to handle 0x00 only.
> + /* 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 +1941,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 +6315,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 +6357,16 @@ 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, &cp, &rp);
> + if (err)
> + return err;
> +
> + hdev->adv_addr_type = own_addr_type;
> + /* Store in hdev for instance 0 */
> + hdev->adv_tx_power = rp.tx_power;
> +
> + /* Update adv data as tx power is known now */
> + err = hci_set_ext_adv_data_sync(hdev, cp.handle);
> if (err)
> return err;
>
> --
> 2.44.1
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists