| 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: <7E257F30-F99C-47CB-8025-9797A10C3835@holtmann.org>
Date: Sun, 8 Mar 2020 09:37:09 +0100
From: Marcel Holtmann <marcel@...tmann.org>
To: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
Cc: Luiz Augusto von Dentz <luiz.dentz@...il.com>,
Alain Michaud <alainm@...omium.org>,
linux-bluetooth@...r.kernel.org,
chromeos-bluetooth-upstreaming@...omium.org,
"David S. Miller" <davem@...emloft.net>,
Johan Hedberg <johan.hedberg@...il.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [RFC PATCH v4 4/5] Bluetooth: Handle LE devices during suspend
Hi Abhishek,
> To handle LE devices, we must first disable passive scanning and
> disconnect all connected devices. Once that is complete, we update the
> whitelist and re-enable scanning
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> ---
>
> Changes in v4: None
> Changes in v3:
> * Split LE changes into its own commit
>
> Changes in v2: None
>
> net/bluetooth/hci_request.c | 164 ++++++++++++++++++++++++------------
> 1 file changed, 110 insertions(+), 54 deletions(-)
>
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index 4d67b1d08608..88fd95d70f89 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -34,6 +34,9 @@
> #define HCI_REQ_PEND 1
> #define HCI_REQ_CANCELED 2
>
> +#define LE_SUSPEND_SCAN_WINDOW 0x0012
> +#define LE_SUSPEND_SCAN_INTERVAL 0x0060
> +
> void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
> {
> skb_queue_head_init(&req->cmd_q);
> @@ -654,6 +657,11 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> {
> struct hci_dev *hdev = req->hdev;
>
> + if (hdev->scanning_paused) {
> + BT_DBG("Scanning is paused for suspend");
> + return;
> + }
> +
> if (use_ext_scan(hdev)) {
> struct hci_cp_le_set_ext_scan_enable cp;
>
> @@ -670,15 +678,53 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
> }
> }
>
> -static void add_to_white_list(struct hci_request *req,
> - struct hci_conn_params *params)
> +static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
> + u8 bdaddr_type)
> +{
> + struct hci_cp_le_del_from_white_list cp;
> +
> + cp.bdaddr_type = bdaddr_type;
> + bacpy(&cp.bdaddr, bdaddr);
> +
> + BT_DBG("Remove %pMR (0x%x) from whitelist", &cp.bdaddr, cp.bdaddr_type);
> + hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST, sizeof(cp), &cp);
> +}
> +
> +/* Adds connection to white list if needed. On error, returns -1. */
> +static int add_to_white_list(struct hci_request *req,
> + struct hci_conn_params *params, u8 *num_entries,
> + bool allow_rpa)
> {
> struct hci_cp_le_add_to_white_list cp;
> + struct hci_dev *hdev = req->hdev;
> +
> + /* Already in white list */
> + if (hci_bdaddr_list_lookup(&hdev->le_white_list, ¶ms->addr,
> + params->addr_type))
> + return 0;
> +
> + /* Select filter policy to accept all advertising */
> + if (*num_entries >= hdev->le_white_list_size)
> + return -1;
>
> + /* White list can not be used with RPAs */
> + if (!allow_rpa &&
> + hci_find_irk_by_addr(hdev, ¶ms->addr, params->addr_type)) {
> + return -1;
> + }
> +
> + /* During suspend, only wakeable devices can be in whitelist */
> + if (hdev->suspended && !params->wakeable)
> + return 0;
> +
> + *num_entries += 1;
> cp.bdaddr_type = params->addr_type;
> bacpy(&cp.bdaddr, ¶ms->addr);
>
> + BT_DBG("Add %pMR (0x%x) to whitelist", &cp.bdaddr, cp.bdaddr_type);
> hci_req_add(req, HCI_OP_LE_ADD_TO_WHITE_LIST, sizeof(cp), &cp);
> +
> + return 0;
> }
>
> static u8 update_white_list(struct hci_request *req)
> @@ -686,7 +732,14 @@ static u8 update_white_list(struct hci_request *req)
> struct hci_dev *hdev = req->hdev;
> struct hci_conn_params *params;
> struct bdaddr_list *b;
> - uint8_t white_list_entries = 0;
> + u8 num_entries = 0;
> + bool pend_conn, pend_report;
> + /* We allow whitelisting even with RPAs in suspend. In the worst case,
> + * we won't be able to wake from devices that use the privacy1.2
> + * features. Additionally, once we support privacy1.2 and IRK
> + * offloading, we can update this to also check for those conditions.
> + */
> + bool allow_rpa = hdev->suspended;
>
> /* Go through the current white list programmed into the
> * controller one by one and check if that address is still
> @@ -695,29 +748,28 @@ static u8 update_white_list(struct hci_request *req)
> * command to remove it from the controller.
> */
> list_for_each_entry(b, &hdev->le_white_list, list) {
> - /* If the device is neither in pend_le_conns nor
> - * pend_le_reports then remove it from the whitelist.
> + pend_conn = hci_pend_le_action_lookup(&hdev->pend_le_conns,
> + &b->bdaddr,
> + b->bdaddr_type);
> + pend_report = hci_pend_le_action_lookup(&hdev->pend_le_reports,
> + &b->bdaddr,
> + b->bdaddr_type);
> +
> + /* If the device is not likely to connect or report,
> + * remove it from the whitelist.
> */
> - if (!hci_pend_le_action_lookup(&hdev->pend_le_conns,
> - &b->bdaddr, b->bdaddr_type) &&
> - !hci_pend_le_action_lookup(&hdev->pend_le_reports,
> - &b->bdaddr, b->bdaddr_type)) {
> - struct hci_cp_le_del_from_white_list cp;
> -
> - cp.bdaddr_type = b->bdaddr_type;
> - bacpy(&cp.bdaddr, &b->bdaddr);
> -
> - hci_req_add(req, HCI_OP_LE_DEL_FROM_WHITE_LIST,
> - sizeof(cp), &cp);
> + if (!pend_conn && !pend_report) {
> + del_from_white_list(req, &b->bdaddr, b->bdaddr_type);
> continue;
> }
>
> - if (hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) {
> - /* White list can not be used with RPAs */
> + /* White list can not be used with RPAs */
> + if (!allow_rpa &&
> + hci_find_irk_by_addr(hdev, &b->bdaddr, b->bdaddr_type)) {
> return 0x00;
> }
>
> - white_list_entries++;
> + num_entries++;
> }
>
> /* Since all no longer valid white list entries have been
> @@ -731,47 +783,17 @@ static u8 update_white_list(struct hci_request *req)
> * white list.
> */
> list_for_each_entry(params, &hdev->pend_le_conns, action) {
> - if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> - ¶ms->addr, params->addr_type))
> - continue;
> -
> - if (white_list_entries >= hdev->le_white_list_size) {
> - /* Select filter policy to accept all advertising */
> + if (add_to_white_list(req, params, &num_entries, allow_rpa))
> return 0x00;
> - }
> -
> - if (hci_find_irk_by_addr(hdev, ¶ms->addr,
> - params->addr_type)) {
> - /* White list can not be used with RPAs */
> - return 0x00;
> - }
> -
> - white_list_entries++;
> - add_to_white_list(req, params);
> }
>
> /* After adding all new pending connections, walk through
> * the list of pending reports and also add these to the
> - * white list if there is still space.
> + * white list if there is still space. Abort if space runs out.
> */
> list_for_each_entry(params, &hdev->pend_le_reports, action) {
> - if (hci_bdaddr_list_lookup(&hdev->le_white_list,
> - ¶ms->addr, params->addr_type))
> - continue;
> -
> - if (white_list_entries >= hdev->le_white_list_size) {
> - /* Select filter policy to accept all advertising */
> + if (add_to_white_list(req, params, &num_entries, allow_rpa))
> return 0x00;
> - }
> -
> - if (hci_find_irk_by_addr(hdev, ¶ms->addr,
> - params->addr_type)) {
> - /* White list can not be used with RPAs */
> - return 0x00;
> - }
> -
> - white_list_entries++;
> - add_to_white_list(req, params);
> }
>
> /* Select filter policy to use white list */
> @@ -866,6 +888,12 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> struct hci_dev *hdev = req->hdev;
> u8 own_addr_type;
> u8 filter_policy;
> + u8 window, interval;
> +
> + if (hdev->scanning_paused) {
> + BT_DBG("Scanning is paused for suspend");
> + return;
> + }
>
> /* Set require_privacy to false since no SCAN_REQ are send
> * during passive scanning. Not using an non-resolvable address
> @@ -896,8 +924,17 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> (hdev->le_features[0] & HCI_LE_EXT_SCAN_POLICY))
> filter_policy |= 0x02;
>
> - hci_req_start_scan(req, LE_SCAN_PASSIVE, hdev->le_scan_interval,
> - hdev->le_scan_window, own_addr_type, filter_policy);
> + if (hdev->suspended) {
> + window = LE_SUSPEND_SCAN_WINDOW;
> + interval = LE_SUSPEND_SCAN_INTERVAL;
> + } else {
> + window = hdev->le_scan_window;
> + interval = hdev->le_scan_interval;
> + }
> +
> + BT_DBG("LE passive scan with whitelist = %d", filter_policy);
> + hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
> + own_addr_type, filter_policy);
> }
>
> static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> @@ -957,6 +994,18 @@ static void hci_req_set_event_filter(struct hci_request *req)
> hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
> }
>
> +static void hci_req_config_le_suspend_scan(struct hci_request *req)
> +{
> + /* Can't change params without disabling first */
> + hci_req_add_le_scan_disable(req);
> +
> + /* Configure params and enable scanning */
> + hci_req_add_le_passive_scan(req);
> +
> + /* Block suspend notifier on response */
> + set_bit(SUSPEND_SCAN_ENABLE, req->hdev->suspend_tasks);
> +}
> +
> static void suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
> {
> BT_DBG("Request complete opcode=0x%x, status=0x%x", opcode, status);
> @@ -991,6 +1040,9 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> page_scan = SCAN_DISABLED;
> hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1, &page_scan);
>
> + /* Disable LE passive scan */
> + hci_req_add_le_scan_disable(&req);
> +
> /* Mark task needing completion */
> set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
>
> @@ -1017,6 +1069,8 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> hdev->scanning_paused = false;
> /* Enable event filter for paired devices */
> hci_req_set_event_filter(&req);
> + /* Enable passive scan at lower duty cycle */
> + hci_req_config_le_suspend_scan(&req);
> /* Pause scan changes again. */
> hdev->scanning_paused = true;
> hci_req_run(&req, suspend_req_complete);
> @@ -1025,6 +1079,8 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> hdev->scanning_paused = false;
>
> hci_req_clear_event_filter(&req);
> + /* Reset passive/background scanning to normal */
> + hci_req_config_le_suspend_scan(&req);
> hci_req_run(&req, suspend_req_complete);
> }
looks good. As with the BR/EDR change, please include bool wakeable; change in this patch and make it 3/5 plus the bt_dev_dbg usage.
If Johan or Luiz have time, I would like to have a second pair of eyes on the whitelist modification function.
Regards
Marcel
Powered by blists - more mailing lists