netdev - Re: [PATCH 2/2] Bluetooth: Send AdvMonitor Dev Found for all matched devices

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: <B4529896-AA8A-40F5-BC3D-A887E2C690E1@holtmann.org>
Date:   Wed, 16 Mar 2022 15:42:04 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Manish Mandlik <mmandlik@...gle.com>
Cc:     Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        chromeos-bluetooth-upstreaming@...omium.org,
        linux-bluetooth@...r.kernel.org,
        Miao-chen Chou <mcchou@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] Bluetooth: Send AdvMonitor Dev Found for all matched
 devices

Hi Manish,

> When an Advertisement Monitor is configured with SamplingPeriod 0xFF,
> the controller reports only one adv report along with the MSFT Monitor
> Device event.
> 
> When an advertiser matches multiple monitors, some controllers send one
> adv report for each matched monitor; whereas, some controllers send just
> one adv report for all matched monitors.
> 
> In such a case, report Adv Monitor Device Found event for each matched
> monitor.
> 
> Signed-off-by: Manish Mandlik <mmandlik@...gle.com>
> Reviewed-by: Miao-chen Chou <mcchou@...omium.org>
> ---
> 
> net/bluetooth/mgmt.c | 70 +++++++++++++++++++++++---------------------
> 1 file changed, 37 insertions(+), 33 deletions(-)

patch has been applied to bluetooth-next tree.

> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index d59c70e9166f..e4da2318a2f6 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -9628,17 +9628,44 @@ void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> 		   NULL);
> }
> 
> +static void mgmt_send_adv_monitor_device_found(struct hci_dev *hdev,
> +					       struct sk_buff *skb,
> +					       struct sock *skip_sk,
> +					       u16 handle)
> +{
> +	struct sk_buff *advmon_skb;
> +	size_t advmon_skb_len;
> +	__le16 *monitor_handle;
> +
> +	if (!skb)
> +		return;
> +
> +	advmon_skb_len = (sizeof(struct mgmt_ev_adv_monitor_device_found) -
> +			  sizeof(struct mgmt_ev_device_found)) + skb->len;
> +	advmon_skb = mgmt_alloc_skb(hdev, MGMT_EV_ADV_MONITOR_DEVICE_FOUND,
> +				    advmon_skb_len);
> +	if (!advmon_skb)
> +		return;
> +
> +	/* ADV_MONITOR_DEVICE_FOUND is similar to DEVICE_FOUND event except
> +	 * that it also has 'monitor_handle'. Make a copy of DEVICE_FOUND and
> +	 * store monitor_handle of the matched monitor.
> +	 */
> +	monitor_handle = skb_put(advmon_skb, sizeof(*monitor_handle));
> +	*monitor_handle = cpu_to_le16(handle);
> +	skb_put_data(advmon_skb, skb->data, skb->len);
> +
> +	mgmt_event_skb(advmon_skb, skip_sk);
> +}
> +

However, this is rather hackish code. It will blow up in our faces at some point and we will spent weeks to find the bug.

I realized that you already got this pattern merged by Luiz and that is why I merged your patch. I would have not accepted this in the first place. Maybe you need to spent some development cycles and check how all DEVICE_FOUND events can be properly generalized.

Regards

Marcel