[PATCH v2 1/2] Bluetooth: Fix not checking advertisement bondaries

[PATCH v2 1/2] Bluetooth: Fix not checking advertisement bondaries

[Luiz Augusto von Dentz]

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

When receiving advertisements check if the length is actually within
the skb, this also make use of skb_pull to advance on the skb->data
instead of a custom ptr that way skb->len shall always indicates how
much data is remaining and can be used to perform checks if there is
enough data to parse.

Fixes: a2ec905d1e160a33b2e210e45ad30445ef26ce0e ("Bluetooth: fix kernel oops in store_pending_adv_report")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Fixes rssi parsing.

 net/bluetooth/hci_event.c | 73 ++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 17 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a4c3703f2e94..6925c090a9e0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5599,24 +5599,41 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	u8 num_reports = skb->data[0];
-	void *ptr = &skb->data[1];
 
 	hci_dev_lock(hdev);
 
+	skb_pull(skb, sizeof(num_reports));
+
 	while (num_reports--) {
-		struct hci_ev_le_advertising_info *ev = ptr;
+		struct hci_ev_le_advertising_info *ev;
 		s8 rssi;
 
-		if (ev->length <= HCI_MAX_AD_LENGTH) {
-			rssi = ev->data[ev->length];
-			process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
-					   ev->bdaddr_type, NULL, 0, rssi,
-					   ev->data, ev->length, false);
-		} else {
-			bt_dev_err(hdev, "Dropping invalid advertising data");
+		if (skb->len < sizeof(*ev)) {
+			bt_dev_err(hdev, "Malformed advertising report");
+			break;
+		}
+
+		ev = (void *) skb->data;
+		skb_pull(skb, sizeof(*ev));
+
+		if (skb->len < ev->length || ev->length > HCI_MAX_AD_LENGTH) {
+			bt_dev_err(hdev, "Malformed advertising data");
+			break;
 		}
 
-		ptr += sizeof(*ev) + ev->length + 1;
+		skb_pull(skb, ev->length);
+
+		if (skb->len < sizeof(rssi)) {
+			bt_dev_err(hdev, "Malformed advertising rssi");
+			break;
+		}
+
+		rssi = skb->data[0];
+		skb_pull(skb, sizeof(rssi));
+
+		process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
+				   ev->bdaddr_type, NULL, 0, rssi,
+				   ev->data, ev->length, false);
 	}
 
 	hci_dev_unlock(hdev);
@@ -5669,15 +5686,31 @@ static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type)
 static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	u8 num_reports = skb->data[0];
-	void *ptr = &skb->data[1];
 
 	hci_dev_lock(hdev);
 
+	skb_pull(skb, sizeof(num_reports));
+
 	while (num_reports--) {
-		struct hci_ev_le_ext_adv_report *ev = ptr;
+		struct hci_ev_le_ext_adv_report *ev;
 		u8 legacy_evt_type;
 		u16 evt_type;
 
+		if (skb->len < sizeof(*ev)) {
+			bt_dev_err(hdev, "Malformed ext advertising report");
+			break;
+		}
+
+		ev = (void *) skb->data;
+		skb_pull(skb, sizeof(*ev));
+
+		if (skb->len < ev->length || ev->length > HCI_MAX_AD_LENGTH) {
+			bt_dev_err(hdev, "Malformed ext advertising data");
+			break;
+		}
+
+		skb_pull(skb, ev->length);
+
 		evt_type = __le16_to_cpu(ev->evt_type);
 		legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type);
 		if (legacy_evt_type != LE_ADV_INVALID) {
@@ -5687,7 +5720,6 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 					   !(evt_type & LE_EXT_ADV_LEGACY_PDU));
 		}
 
-		ptr += sizeof(*ev) + ev->length;
 	}
 
 	hci_dev_unlock(hdev);
@@ -5873,19 +5905,26 @@ static void hci_le_direct_adv_report_evt(struct hci_dev *hdev,
 					 struct sk_buff *skb)
 {
 	u8 num_reports = skb->data[0];
-	void *ptr = &skb->data[1];
 
 	hci_dev_lock(hdev);
 
+	skb_pull(skb, sizeof(num_reports));
+
 	while (num_reports--) {
-		struct hci_ev_le_direct_adv_info *ev = ptr;
+		struct hci_ev_le_direct_adv_info *ev;
+
+		if (skb->len < sizeof(*ev)) {
+			bt_dev_err(hdev, "Malformed direct advertising");
+			break;
+		}
+
+		ev = (void *) skb->data;
+		skb_pull(skb, sizeof(*ev));
 
 		process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
 				   ev->bdaddr_type, &ev->direct_addr,
 				   ev->direct_addr_type, ev->rssi, NULL, 0,
 				   false);
-
-		ptr += sizeof(*ev);
 	}
 
 	hci_dev_unlock(hdev);
-- 
2.26.2

[PATCH v2 2/2] Bluetooth: A2MP: Fix not setting request ID

[Luiz Augusto von Dentz]

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

This fixes not resetting of the request ID when sending
A2MP_GETAMPASSOC_RSP.

Fixes: Bluetooth: A2MP: Fix not initializing all members
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/a2mp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
index da7fd7c8c2dc..7a1e0b785f45 100644
--- a/net/bluetooth/a2mp.c
+++ b/net/bluetooth/a2mp.c
@@ -381,10 +381,11 @@ static int a2mp_getampassoc_req(struct amp_mgr *mgr, struct sk_buff *skb,
 	hdev = hci_dev_get(req->id);
 	if (!hdev || hdev->amp_type == AMP_TYPE_BREDR || tmp) {
 		struct a2mp_amp_assoc_rsp rsp;
-		rsp.id = req->id;
 
 		memset(&rsp, 0, sizeof(rsp));
 
+		rsp.id = req->id;
+
 		if (tmp) {
 			rsp.status = A2MP_STATUS_COLLISION_OCCURED;
 			amp_mgr_put(tmp);
-- 
2.26.2

Re: [PATCH v2 1/2] Bluetooth: Fix not checking advertisement bondaries

[Abhishek Pandit-Subedi]

On Mon, Oct 19, 2020 at 10:25 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> When receiving advertisements check if the length is actually within
> the skb, this also make use of skb_pull to advance on the skb->data
> instead of a custom ptr that way skb->len shall always indicates how
> much data is remaining and can be used to perform checks if there is
> enough data to parse.
>
> Fixes: a2ec905d1e160a33b2e210e45ad30445ef26ce0e ("Bluetooth: fix kernel oops in store_pending_adv_report")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> v2: Fixes rssi parsing.
>
>  net/bluetooth/hci_event.c | 73 ++++++++++++++++++++++++++++++---------
>  1 file changed, 56 insertions(+), 17 deletions(-)

Tested-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

---

I cherry-picked this to our 4.19 kernel and ran our LE Health tests on
the Kohaku chromebook (AX201 controller). All tests are passing.

Re: [PATCH v2 1/2] Bluetooth: Fix not checking advertisement bondaries

[Johan Hedberg]

Hi Luiz,

On Mon, Oct 19, 2020, Luiz Augusto von Dentz wrote:
> When receiving advertisements check if the length is actually within
> the skb, this also make use of skb_pull to advance on the skb->data
> instead of a custom ptr that way skb->len shall always indicates how
> much data is remaining and can be used to perform checks if there is
> enough data to parse.
> 
> Fixes: a2ec905d1e160a33b2e210e45ad30445ef26ce0e ("Bluetooth: fix kernel oops in store_pending_adv_report")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> v2: Fixes rssi parsing.
> 
>  net/bluetooth/hci_event.c | 73 ++++++++++++++++++++++++++++++---------
>  1 file changed, 56 insertions(+), 17 deletions(-)

Could we get the matching HCI logs for these corrupted events? It'd be
good to include that in the commit message. Unless I misunderstood
something, from what I can see from the changes the fields you are
adding checks for are generated by the Bluetooth controller, i.e. only a
buggy or broken Bluetooth controller would generate such events
(meaning, this shouldn't be generally remotely exploitable), so it'd be
good to know exactly which controllers generate such broken events.

Johan

Re: [PATCH v2 2/2] Bluetooth: A2MP: Fix not setting request ID

[Johan Hedberg]

Hi Luiz,

On Mon, Oct 19, 2020, Luiz Augusto von Dentz wrote:
> This fixes not resetting of the request ID when sending
> A2MP_GETAMPASSOC_RSP.
> 
> Fixes: Bluetooth: A2MP: Fix not initializing all members
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  net/bluetooth/a2mp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

The fix itself looks fine, but I think the "Fixes: ..." line should
contain the commit id followed by the commit summary within parentheses,
at least based on how I can see this style of annotation used in other
commits in the tree.

Johan

Re: [PATCH v2 2/2] Bluetooth: A2MP: Fix not setting request ID

[Luiz Augusto von Dentz]

Hi Johan,

On Thu, Oct 22, 2020 at 2:00 PM Johan Hedberg <johan.hedberg@gmail.com> wrote:
>
> Hi Luiz,
>
> On Mon, Oct 19, 2020, Luiz Augusto von Dentz wrote:
> > This fixes not resetting of the request ID when sending
> > A2MP_GETAMPASSOC_RSP.
> >
> > Fixes: Bluetooth: A2MP: Fix not initializing all members
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >  net/bluetooth/a2mp.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
>
> The fix itself looks fine, but I think the "Fixes: ..." line should
> contain the commit id followed by the commit summary within parentheses,
> at least based on how I can see this style of annotation used in other
> commits in the tree.

I did not add a commit id because it did not reached the Linus tree
yet and we cannot use the one from bluetooth-next since that tree is
rebased then the commit id might change so I thought the only way to
do it is by commit message.

-- 
Luiz Augusto von Dentz

Re: [PATCH v2 2/2] Bluetooth: A2MP: Fix not setting request ID

[Johan Hedberg]

Hi Luiz,

On Thu, Oct 22, 2020, Luiz Augusto von Dentz wrote:
> > On Mon, Oct 19, 2020, Luiz Augusto von Dentz wrote:
> > > This fixes not resetting of the request ID when sending
> > > A2MP_GETAMPASSOC_RSP.
> > >
> > > Fixes: Bluetooth: A2MP: Fix not initializing all members
> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > ---
> > >  net/bluetooth/a2mp.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > The fix itself looks fine, but I think the "Fixes: ..." line should
> > contain the commit id followed by the commit summary within parentheses,
> > at least based on how I can see this style of annotation used in other
> > commits in the tree.
> 
> I did not add a commit id because it did not reached the Linus tree
> yet

Yes it did:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eddb7732119d53400f48a02536a84c509692faa8

Johan

Re: [PATCH v2 2/2] Bluetooth: A2MP: Fix not setting request ID

[Luiz Augusto von Dentz]

Hi Johan,

On Fri, Oct 23, 2020 at 1:54 AM Johan Hedberg <johan.hedberg@gmail.com> wrote:
>
> Hi Luiz,
>
> On Thu, Oct 22, 2020, Luiz Augusto von Dentz wrote:
> > > On Mon, Oct 19, 2020, Luiz Augusto von Dentz wrote:
> > > > This fixes not resetting of the request ID when sending
> > > > A2MP_GETAMPASSOC_RSP.
> > > >
> > > > Fixes: Bluetooth: A2MP: Fix not initializing all members
> > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > ---
> > > >  net/bluetooth/a2mp.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > The fix itself looks fine, but I think the "Fixes: ..." line should
> > > contain the commit id followed by the commit summary within parentheses,
> > > at least based on how I can see this style of annotation used in other
> > > commits in the tree.
> >
> > I did not add a commit id because it did not reached the Linus tree
> > yet
>
> Yes it did:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eddb7732119d53400f48a02536a84c509692faa8

Somehow I missed that, or git.kernel.org was playing tricks on me, Ive
now updated the v3 to include it.

> Johan



-- 
Luiz Augusto von Dentz