Hi Radosław,

On Thu, Jan 13, 2022 at 2:07 PM Radosław Biernacki [off-list ref] wrote:

Hi Luiz,

czw., 13 sty 2022 o 17:17 Luiz Augusto von Dentz
[off-list ref] napisał(a):

quoted

Hi Radoslaw,

On Thu, Jan 13, 2022 at 7:09 AM Radoslaw Biernacki [off-list ref] wrote:

quoted

From: Radoslaw Biernacki <redacted>

This patch fixes skb allocation, as lack of space for ev might push skb
tail beyond its end.
Also introduce eir_precalc_len() that can be used instead of magic
numbers for similar eir operations on skb.

Fixes: cf1bce1de7eeb ("Bluetooth: mgmt: Make use of mgmt_send_event_skb in MGMT_EV_DEVICE_FOUND")
Signed-off-by: Angela Czubak <redacted>
Signed-off-by: Marek Maslanka <redacted>
Signed-off-by: Radoslaw Biernacki <redacted>
---
 net/bluetooth/eir.h  |  5 +++++
 net/bluetooth/mgmt.c | 12 ++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/eir.h b/net/bluetooth/eir.h
index 05e2e917fc25..e5876751f07e 100644
--- a/net/bluetooth/eir.h
+++ b/net/bluetooth/eir.h

@@ -15,6 +15,11 @@ u8 eir_create_scan_rsp(struct hci_dev *hdev, u8 instance, u8 *ptr);
 u8 eir_append_local_name(struct hci_dev *hdev, u8 *eir, u8 ad_len);
 u8 eir_append_appearance(struct hci_dev *hdev, u8 *ptr, u8 ad_len);

+static inline u16 eir_precalc_len(u8 data_len)
+{
+       return sizeof(u8) * 2 + data_len;
+}
+
 static inline u16 eir_append_data(u8 *eir, u16 eir_len, u8 type,
                                  u8 *data, u8 data_len)
 {

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 37087cf7dc5a..d517fd847730 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c

@@ -9680,13 +9680,11 @@ void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 {
        struct sk_buff *skb;
        struct mgmt_ev_device_found *ev;
-       u16 eir_len;
-       u32 flags;
+       u16 eir_len = 0;
+       u32 flags = 0;

-       if (name_len)
-               skb = mgmt_alloc_skb(hdev, MGMT_EV_DEVICE_FOUND, 2 + name_len);
-       else
-               skb = mgmt_alloc_skb(hdev, MGMT_EV_DEVICE_FOUND, 0);
+       skb = mgmt_alloc_skb(hdev, MGMT_EV_DEVICE_FOUND,
+                            sizeof(*ev) + (name ? eir_precalc_len(name_len) : 0));

Looks like mgmt_device_connected also has a similar problem.

Yes, I was planning to send a patch to this one though it will not be as slick.
It would be nice to have a helper which will call skb_put() and add
eir data at once.
Basically skb operation in pair to, what eir_append_data() does with
help of eir_len but without awkwardness when passing return value to
skb_put() (as it returns offset not size).

Hmm, that might be a good idea indeed something like eir_append_skb,
if only we could grow the skb with skb_put directly that would
eliminate the problem with having to reserve enough space for the
worse case.

I will send V2 with two patches. I hope they will align with your
original goal of eliminating the necessity of intermediary buffers at
some point in future.

quoted

quoted

        ev = skb_put(skb, sizeof(*ev));
        bacpy(&ev->addr.bdaddr, bdaddr);

@@ -9696,10 +9694,8 @@ void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
        if (name) {
                eir_len = eir_append_data(ev->eir, 0, EIR_NAME_COMPLETE, name,
                                          name_len);
-               flags = 0;
                skb_put(skb, eir_len);
        } else {
-               eir_len = 0;
                flags = MGMT_DEV_FOUND_NAME_REQUEST_FAILED;
        }

These changes would leave flags and eir_len uninitialized.

Both are initialized to 0 by this patch.

Sorry, I must be blind that I didn't see you had changed that to be
initialized in their declaration.

quoted

quoted

--
2.34.1.703.g22d0c6ccf7-goog

--
Luiz Augusto von Dentz

-- 
Luiz Augusto von Dentz