Bluetooth: hci_sync: Fix advertising data UAFs

hci_find_adv_instance() returns an adv_info pointer that is valid only
while hdev->lock is held.  The advertising command-sync paths perform
instance lookups without that lock and, in some cases, retain the pointer
while waiting for a controller response.

An advertising termination event can therefore interleave as follows:

  hci_cmd_sync_work                 hci_rx_work
  hci_find_adv_instance()
  __hci_cmd_sync_status()
    wait for controller reply       hci_dev_lock()
                                    hci_remove_adv_instance()
                                      kfree(adv)
  adv->scan_rsp_changed = false

KASAN reported:

  BUG: KASAN: slab-use-after-free in hci_set_ext_scan_rsp_data_sync+0x2e1/0x300
  Write of size 1 at addr ffff88810a45d21d by task kworker/u17:0/88
  Workqueue: hci0 hci_cmd_sync_work
  Call Trace:
   hci_set_ext_scan_rsp_data_sync+0x2e1/0x300
   hci_schedule_adv_instance_sync+0x390/0x4c0
   hci_cmd_sync_work+0x173/0x300
  Allocated by task 87:
   hci_add_adv_instance+0x538/0xac0
   add_advertising+0x885/0x1160
  Freed by task 89:
   kfree+0x131/0x3c0
   hci_remove_adv_instance+0x1d8/0x3b0
   hci_le_ext_adv_term_evt+0x17b/0x730

Protect the instance lookup and payload construction in the extended
advertising, scan response, and periodic advertising data paths.  Snapshot
the advertising parameters under hdev->lock, but release the lock before
waiting for the controller.

Clear advertising-data dirty bits before issuing their commands and
restore them after a failure using a fresh lookup.  Likewise, update the
reported transmit power through a fresh lookup after the parameter command
completes.  No adv_info pointer then survives an HCI command wait.

Fixes: cba6b758711c ("Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 2")
Cc: stable@vger.kernel.org
Suggested-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index c0b1fc293b4962326bfc702dcb2a71988bce8a89..aa3d5381881245c7b3890a394a2431b0cf4b2f2e 100644 (file)
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1233,10 +1233,11 @@ static int hci_set_adv_set_random_addr_sync(struct hci_dev *hdev, u8 instance,
 }
 
 static int
-hci_set_ext_adv_params_sync(struct hci_dev *hdev, struct adv_info *adv,
+hci_set_ext_adv_params_sync(struct hci_dev *hdev, u8 instance,
                            const struct hci_cp_le_set_ext_adv_params *cp,
                            struct hci_rp_le_set_ext_adv_params *rp)
 {
+       struct adv_info *adv;
        struct sk_buff *skb;
 
        skb = __hci_cmd_sync(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS, sizeof(*cp),
@@ -1264,11 +1265,15 @@ hci_set_ext_adv_params_sync(struct hci_dev *hdev, struct adv_info *adv,
 
        if (!rp->status) {
                hdev->adv_addr_type = cp->own_addr_type;
-               if (!cp->handle) {
+               if (!instance) {
                        /* Store in hdev for instance 0 */
                        hdev->adv_tx_power = rp->tx_power;
-               } else if (adv) {
-                       adv->tx_power = rp->tx_power;
+               } else {
+                       hci_dev_lock(hdev);
+                       adv = hci_find_adv_instance(hdev, instance);
+                       if (adv)
+                               adv->tx_power = rp->tx_power;
+                       hci_dev_unlock(hdev);
                }
        }
 
@@ -1284,9 +1289,13 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
        int err;
 
        if (instance) {
+               hci_dev_lock(hdev);
+
                adv = hci_find_adv_instance(hdev, instance);
-               if (!adv || !adv->adv_data_changed)
+               if (!adv || !adv->adv_data_changed) {
+                       hci_dev_unlock(hdev);
                        return 0;
+               }
        }
 
        len = eir_create_adv_data(hdev, instance, pdu->data,
@@ -1297,16 +1306,27 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
        pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE;
        pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG;
 
+       if (adv) {
+               adv->adv_data_changed = false;
+               hci_dev_unlock(hdev);
+       }
+
        err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
                                    struct_size(pdu, data, len), pdu,
                                    HCI_CMD_TIMEOUT);
-       if (err)
+       if (err) {
+               if (instance) {
+                       hci_dev_lock(hdev);
+                       adv = hci_find_adv_instance(hdev, instance);
+                       if (adv)
+                               adv->adv_data_changed = true;
+                       hci_dev_unlock(hdev);
+               }
+
                return err;
+       }
 
-       /* Update data if the command succeed */
-       if (adv) {
-               adv->adv_data_changed = false;
-       } else {
+       if (!instance) {
                memcpy(hdev->adv_data, pdu->data, len);
                hdev->adv_data_len = len;
        }
@@ -1360,22 +1380,22 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
        struct adv_info *adv;
        bool secondary_adv;
 
-       if (instance > 0) {
-               adv = hci_find_adv_instance(hdev, instance);
-               if (!adv)
-                       return -EINVAL;
-       } else {
-               adv = NULL;
-       }
-
        /* Updating parameters of an active instance will return a
-        * Command Disallowed error, so we must first disable the
-        * instance if it is active.
+        * Command Disallowed error, so disable it before taking a snapshot.
         */
-       if (adv) {
+       if (instance > 0) {
                err = hci_disable_ext_adv_instance_sync(hdev, instance);
                if (err)
                        return err;
+
+               hci_dev_lock(hdev);
+               adv = hci_find_adv_instance(hdev, instance);
+               if (!adv) {
+                       hci_dev_unlock(hdev);
+                       return -EINVAL;
+               }
+       } else {
+               adv = NULL;
        }
 
        flags = hci_adv_instance_flags(hdev, instance);
@@ -1386,8 +1406,11 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
        connectable = (flags & MGMT_ADV_FLAG_CONNECTABLE) ||
                      mgmt_get_connectable(hdev);
 
-       if (!is_advertising_allowed(hdev, connectable))
+       if (!is_advertising_allowed(hdev, connectable)) {
+               if (instance)
+                       hci_dev_unlock(hdev);
                return -EPERM;
+       }
 
        /* Set require_privacy to true only when non-connectable
         * advertising is used and it is not periodic.
@@ -1398,8 +1421,11 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
        err = hci_get_random_address(hdev, require_privacy,
                                     adv_use_rpa(hdev, flags), adv,
                                     &own_addr_type, &random_addr);
-       if (err < 0)
+       if (err < 0) {
+               if (instance)
+                       hci_dev_unlock(hdev);
                return err;
+       }
 
        memset(&cp, 0, sizeof(cp));
 
@@ -1450,6 +1476,9 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
        cp.channel_map = hdev->le_adv_channel_map;
        cp.handle = adv ? adv->handle : instance;
 
+       if (instance)
+               hci_dev_unlock(hdev);
+
        if (flags & MGMT_ADV_FLAG_SEC_2M) {
                cp.primary_phy = HCI_ADV_PHY_1M;
                cp.secondary_phy = HCI_ADV_PHY_2M;
@@ -1462,12 +1491,12 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
                cp.secondary_phy = HCI_ADV_PHY_1M;
        }
 
-       err = hci_set_ext_adv_params_sync(hdev, adv, &cp, &rp);
+       err = hci_set_ext_adv_params_sync(hdev, instance, &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);
+       err = hci_set_ext_adv_data_sync(hdev, instance);
        if (err)
                return err;
 
@@ -1475,9 +1504,14 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance)
             own_addr_type == ADDR_LE_DEV_RANDOM_RESOLVED) &&
            bacmp(&random_addr, BDADDR_ANY)) {
                /* Check if random address need to be updated */
-               if (adv) {
-                       if (!bacmp(&random_addr, &adv->random_addr))
+               if (instance) {
+                       hci_dev_lock(hdev);
+                       adv = hci_find_adv_instance(hdev, instance);
+                       if (!adv || !bacmp(&random_addr, &adv->random_addr)) {
+                               hci_dev_unlock(hdev);
                                return 0;
+                       }
+                       hci_dev_unlock(hdev);
                } else {
                        if (!bacmp(&random_addr, &hdev->random_addr))
                                return 0;
@@ -1499,9 +1533,13 @@ static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
        int err;
 
        if (instance) {
+               hci_dev_lock(hdev);
+
                adv = hci_find_adv_instance(hdev, instance);
-               if (!adv || !adv->scan_rsp_changed)
+               if (!adv || !adv->scan_rsp_changed) {
+                       hci_dev_unlock(hdev);
                        return 0;
+               }
        }
 
        len = eir_create_scan_rsp(hdev, instance, pdu->data);
@@ -1511,15 +1549,27 @@ static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
        pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE;
        pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG;
 
+       if (adv) {
+               adv->scan_rsp_changed = false;
+               hci_dev_unlock(hdev);
+       }
+
        err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_SCAN_RSP_DATA,
                                    struct_size(pdu, data, len), pdu,
                                    HCI_CMD_TIMEOUT);
-       if (err)
+       if (err) {
+               if (instance) {
+                       hci_dev_lock(hdev);
+                       adv = hci_find_adv_instance(hdev, instance);
+                       if (adv)
+                               adv->scan_rsp_changed = true;
+                       hci_dev_unlock(hdev);
+               }
+
                return err;
+       }
 
-       if (adv) {
-               adv->scan_rsp_changed = false;
-       } else {
+       if (!instance) {
                memcpy(hdev->scan_rsp_data, pdu->data, len);
                hdev->scan_rsp_data_len = len;
        }
@@ -1534,8 +1584,14 @@ static int __hci_set_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
 
        memset(&cp, 0, sizeof(cp));
 
+       if (instance)
+               hci_dev_lock(hdev);
+
        len = eir_create_scan_rsp(hdev, instance, cp.data);
 
+       if (instance)
+               hci_dev_unlock(hdev);
+
        if (hdev->scan_rsp_data_len == len &&
            !memcmp(cp.data, hdev->scan_rsp_data, len))
                return 0;
@@ -1670,9 +1726,13 @@ static int hci_set_per_adv_data_sync(struct hci_dev *hdev, u8 instance)
        struct adv_info *adv = NULL;
 
        if (instance) {
+               hci_dev_lock(hdev);
+
                adv = hci_find_adv_instance(hdev, instance);
-               if (!adv || !adv->periodic)
+               if (!adv || !adv->periodic) {
+                       hci_dev_unlock(hdev);
                        return 0;
+               }
        }
 
        len = eir_create_per_adv_data(hdev, instance, pdu->data);
@@ -1681,6 +1741,9 @@ static int hci_set_per_adv_data_sync(struct hci_dev *hdev, u8 instance)
        pdu->handle = adv ? adv->handle : instance;
        pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE;
 
+       if (adv)
+               hci_dev_unlock(hdev);
+
        return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_PER_ADV_DATA,
                                     struct_size(pdu, data, len), pdu,
                                     HCI_CMD_TIMEOUT);
@@ -6523,7 +6586,7 @@ static int hci_le_ext_directed_advertising_sync(struct hci_dev *hdev,
        if (err)
                return err;
 
-       err = hci_set_ext_adv_params_sync(hdev, NULL, &cp, &rp);
+       err = hci_set_ext_adv_params_sync(hdev, 0, &cp, &rp);
        if (err)
                return err;