[PATCH] Bluetooth: hci_core: Fix leaking sent_cmd skb

[PATCH] Bluetooth: hci_core: Fix leaking sent_cmd skb

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

sent_cmd memory is not freed before freeing hci_dev causing it to leak
it contents.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5bde0ec41177..b4782a6c1025 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2739,6 +2739,7 @@ void hci_release_dev(struct hci_dev *hdev)
 	hci_dev_unlock(hdev);
 
 	ida_simple_remove(&hci_index_ida, hdev->id);
+	kfree_skb(hdev->sent_cmd);
 	kfree(hdev);
 }
 EXPORT_SYMBOL(hci_release_dev);
-- 
2.34.1

[RFC] Bluetooth: hci_core: Introduce hci_recv_event_data

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This introduces hci_recv_event_data to make it simpler to access the
contents of last received event rather than having to pass its contents
to the likes of *_ind/*_cfm callbacks.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_core.c         | 32 ++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |  3 +++
 3 files changed, 37 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f5caff1ddb29..c454913794bf 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -516,6 +516,7 @@ struct hci_dev {
 	struct sk_buff_head	cmd_q;
 
 	struct sk_buff		*sent_cmd;
+	struct sk_buff		*recv_event;
 
 	struct mutex		req_lock;
 	wait_queue_head_t	req_wait_q;
@@ -1727,6 +1728,7 @@ void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
 void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
 
 void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
+void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
 
 u32 hci_conn_get_phy(struct hci_conn *conn);
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b4782a6c1025..5d1167b67a47 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2740,6 +2740,7 @@ void hci_release_dev(struct hci_dev *hdev)
 
 	ida_simple_remove(&hci_index_ida, hdev->id);
 	kfree_skb(hdev->sent_cmd);
+	kfree_skb(hdev->recv_event);
 	kfree(hdev);
 }
 EXPORT_SYMBOL(hci_release_dev);
@@ -3024,6 +3025,37 @@ void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
 	return hdev->sent_cmd->data + HCI_COMMAND_HDR_SIZE;
 }
 
+/* Get data from last received event */
+void *hci_recv_event_data(struct hci_dev *hdev, __u8 event)
+{
+	struct hci_event_hdr *hdr;
+	int offset;
+
+	if (!hdev->recv_event)
+		return NULL;
+
+	hdr = (void *) hdev->recv_event->data;
+	offset = sizeof(hdr);
+
+	if (hdr->evt != event) {
+		/* In case of LE metaevent check the subevent match */
+		if (hdr->evt == HCI_EV_LE_META) {
+			struct hci_ev_le_meta *ev;
+
+			ev = (void *) hdev->recv_event->data + offset;
+			offset += sizeof(*ev);
+			if (ev->subevent == event)
+				goto found;
+		}
+		return NULL;
+	}
+
+found:
+	bt_dev_dbg(hdev, "event 0x%2.2x", event);
+
+	return hdev->recv_event->data + offset;
+}
+
 /* Send ACL data */
 static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
 {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 63b925921c87..613050bd80cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6898,6 +6898,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		goto done;
 	}
 
+	kfree_skb(hdev->recv_event);
+	hdev->recv_event = skb_clone(skb, GFP_KERNEL);
+
 	event = hdr->evt;
 	if (!event) {
 		bt_dev_warn(hdev, "Received unexpected HCI Event 0x%2.2x",
-- 
2.34.1

RE: [RFC] Bluetooth: hci_core: Introduce hci_recv_event_data

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----
error: patch failed: net/bluetooth/hci_core.c:2740
error: net/bluetooth/hci_core.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch


Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

RE: Bluetooth: hci_core: Fix leaking sent_cmd skb

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=611428

---Test result---

Test Summary:
CheckPatch                    PASS      1.53 seconds
GitLint                       PASS      1.01 seconds
SubjectPrefix                 PASS      0.91 seconds
BuildKernel                   PASS      30.69 seconds
BuildKernel32                 PASS      27.09 seconds
Incremental Build with patchesPASS      37.23 seconds
TestRunner: Setup             PASS      478.07 seconds
TestRunner: l2cap-tester      PASS      13.54 seconds
TestRunner: bnep-tester       PASS      6.13 seconds
TestRunner: mgmt-tester       PASS      105.48 seconds
TestRunner: rfcomm-tester     PASS      7.53 seconds
TestRunner: sco-tester        PASS      7.78 seconds
TestRunner: smp-tester        PASS      7.66 seconds
TestRunner: userchan-tester   PASS      6.39 seconds



---
Regards,
Linux Bluetooth

Re: [RFC] Bluetooth: hci_core: Introduce hci_recv_event_data

[Marcel Holtmann]

Hi Luiz,

> This introduces hci_recv_event_data to make it simpler to access the
> contents of last received event rather than having to pass its contents
> to the likes of *_ind/*_cfm callbacks.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci_core.h |  2 ++
> net/bluetooth/hci_core.c         | 32 ++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c        |  3 +++
> 3 files changed, 37 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f5caff1ddb29..c454913794bf 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -516,6 +516,7 @@ struct hci_dev {
> 	struct sk_buff_head	cmd_q;
> 
> 	struct sk_buff		*sent_cmd;
> +	struct sk_buff		*recv_event;
> 
> 	struct mutex		req_lock;
> 	wait_queue_head_t	req_wait_q;
> @@ -1727,6 +1728,7 @@ void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
> void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
> 
> void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
> +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
> 
> u32 hci_conn_get_phy(struct hci_conn *conn);
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b4782a6c1025..5d1167b67a47 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2740,6 +2740,7 @@ void hci_release_dev(struct hci_dev *hdev)
> 
> 	ida_simple_remove(&hci_index_ida, hdev->id);
> 	kfree_skb(hdev->sent_cmd);
> +	kfree_skb(hdev->recv_event);
> 	kfree(hdev);
> }
> EXPORT_SYMBOL(hci_release_dev);
> @@ -3024,6 +3025,37 @@ void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
> 	return hdev->sent_cmd->data + HCI_COMMAND_HDR_SIZE;
> }
> 
> +/* Get data from last received event */
> +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event)
> +{
> +	struct hci_event_hdr *hdr;
> +	int offset;
> +
> +	if (!hdev->recv_event)
> +		return NULL;
> +
> +	hdr = (void *) hdev->recv_event->data;
> +	offset = sizeof(hdr);
> +
> +	if (hdr->evt != event) {
> +		/* In case of LE metaevent check the subevent match */
> +		if (hdr->evt == HCI_EV_LE_META) {
> +			struct hci_ev_le_meta *ev;
> +
> +			ev = (void *) hdev->recv_event->data + offset;
> +			offset += sizeof(*ev);
> +			if (ev->subevent == event)
> +				goto found;
> +		}
> +		return NULL;
> +	}
> +
> +found:
> +	bt_dev_dbg(hdev, "event 0x%2.2x", event);
> +
> +	return hdev->recv_event->data + offset;
> +}
> +
> /* Send ACL data */
> static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
> {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 63b925921c87..613050bd80cc 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -6898,6 +6898,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> 		goto done;
> 	}
> 
> +	kfree_skb(hdev->recv_event);
> +	hdev->recv_event = skb_clone(skb, GFP_KERNEL);
> +

fill me into why this a good idea. We end up creating clones of an SKB. Is this a good idea?

Regards

Marcel

Re: [PATCH] Bluetooth: hci_core: Fix leaking sent_cmd skb

[Marcel Holtmann]

Hi Luiz,

> sent_cmd memory is not freed before freeing hci_dev causing it to leak
> it contents.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_core.c | 1 +
> 1 file changed, 1 insertion(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [RFC] Bluetooth: hci_core: Introduce hci_recv_event_data

[Luiz Augusto von Dentz]

Hi Marcel,

On Mon, Feb 7, 2022 at 7:53 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This introduces hci_recv_event_data to make it simpler to access the
> > contents of last received event rather than having to pass its contents
> > to the likes of *_ind/*_cfm callbacks.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/hci_core.h |  2 ++
> > net/bluetooth/hci_core.c         | 32 ++++++++++++++++++++++++++++++++
> > net/bluetooth/hci_event.c        |  3 +++
> > 3 files changed, 37 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index f5caff1ddb29..c454913794bf 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -516,6 +516,7 @@ struct hci_dev {
> >       struct sk_buff_head     cmd_q;
> >
> >       struct sk_buff          *sent_cmd;
> > +     struct sk_buff          *recv_event;
> >
> >       struct mutex            req_lock;
> >       wait_queue_head_t       req_wait_q;
> > @@ -1727,6 +1728,7 @@ void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
> > void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
> >
> > void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
> > +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
> >
> > u32 hci_conn_get_phy(struct hci_conn *conn);
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index b4782a6c1025..5d1167b67a47 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -2740,6 +2740,7 @@ void hci_release_dev(struct hci_dev *hdev)
> >
> >       ida_simple_remove(&hci_index_ida, hdev->id);
> >       kfree_skb(hdev->sent_cmd);
> > +     kfree_skb(hdev->recv_event);
> >       kfree(hdev);
> > }
> > EXPORT_SYMBOL(hci_release_dev);
> > @@ -3024,6 +3025,37 @@ void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
> >       return hdev->sent_cmd->data + HCI_COMMAND_HDR_SIZE;
> > }
> >
> > +/* Get data from last received event */
> > +void *hci_recv_event_data(struct hci_dev *hdev, __u8 event)
> > +{
> > +     struct hci_event_hdr *hdr;
> > +     int offset;
> > +
> > +     if (!hdev->recv_event)
> > +             return NULL;
> > +
> > +     hdr = (void *) hdev->recv_event->data;
> > +     offset = sizeof(hdr);
> > +
> > +     if (hdr->evt != event) {
> > +             /* In case of LE metaevent check the subevent match */
> > +             if (hdr->evt == HCI_EV_LE_META) {
> > +                     struct hci_ev_le_meta *ev;
> > +
> > +                     ev = (void *) hdev->recv_event->data + offset;
> > +                     offset += sizeof(*ev);
> > +                     if (ev->subevent == event)
> > +                             goto found;
> > +             }
> > +             return NULL;
> > +     }
> > +
> > +found:
> > +     bt_dev_dbg(hdev, "event 0x%2.2x", event);
> > +
> > +     return hdev->recv_event->data + offset;
> > +}
> > +
> > /* Send ACL data */
> > static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
> > {
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 63b925921c87..613050bd80cc 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -6898,6 +6898,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> >               goto done;
> >       }
> >
> > +     kfree_skb(hdev->recv_event);
> > +     hdev->recv_event = skb_clone(skb, GFP_KERNEL);
> > +
>
> fill me into why this a good idea. We end up creating clones of an SKB. Is this a good idea?

It is limited to just the last event, Id like to introduce since in
most cases _ind/_cfm callbacks are done with use of an hci_conn but in
case of BIS we don't really have a connection until we have created a
BIG Sync so I end up doing the following:

https://gist.github.com/Vudentz/2dd08291a0ae0df7fff1034653b01e97#file-diff-L279
https://gist.github.com/Vudentz/2dd08291a0ae0df7fff1034653b01e97#file-diff-L337

So that saves us the trouble of having to add or change the existing
callbacks to sockets, so the socket themselves can check the last
event data instead if it really needs to.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz