Bluetooth: hci_event: potential out of bounds parsing ADV events

There is a potential out of bounds if "ev->length" is too high or if the number of reports are too many.

Fixes c215e9397b00 ("Bluetooth: Process extended ADV report event")

Signed-off-by Dan Carpenter <[email protected]>

---
Not tested. I suck at pointer math, and I don't know why the protocol requires a "+ 1". Please review carefully.

 net/bluetooth/hci_event.c | 3 +++
 1 file changed, 3 insertions(+)

net/bluetooth/hci_event.c +1 -0

index 609fd6871c5a..ee945b3d12e1 100644
@@ -5417,6 +5417,7 @@ 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];
+	void *end = &skb->data[skb->len];
 
 	hci_dev_lock(hdev);
 

Hi Dan,

Quoted 5 lines · Dan Carpenter

There is a potential out of bounds if "ev->length" is too high or if the number of reports are too many.

Fixes c215e9397b00 ("Bluetooth: Process extended ADV report event")

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Tomas Bortoli <[email protected]>

Quoted 20 lines

---
Not tested. I suck at pointer math, and I don't know why the protocol requires a "+ 1". Please review carefully.

 net/bluetooth/hci_event.c | 3 +++
 1 file changed, 3 insertions(+)

net/bluetooth/hci_event.c +1 -0

index 609fd6871c5a..ee945b3d12e1 100644
@@ -5417,6 +5417,7 @@ 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];
+	void *end = &skb->data[skb->len];
 
 	hci_dev_lock(hdev);
 

net/bluetooth/hci_event.c static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5430

Quoted 5 lines

net/bluetooth/hci_event.c +2 -0

@@ -5425,6 +5426,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		u8 legacy_evt_type;
 		u16 evt_type;
 
+		if (ptr + sizeof(*ev) + ev->length + 1 > end)
+			break;

Assuming that per each iteration, the while cycle, including the call to process_adv_report() read up to (sizeof(*ev) + ev->length + 1) bytes,

(I don't understand why the +1, but, anyway..)

If the assumption is correct, then the if condition should be:

if (ptr + sizeof(*ev) + ev->length + 1 >= end)

Because as soon as we try to read from the end pointer on, we are out-of-bound.. the valid skb bytes end at end-1 (included)

Note that also hci_le_adv_report_evt() is likely to need the same fix!

net/bluetooth/hci_event.c static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5433

Quoted 4 lines

net/bluetooth/hci_event.c +0 -0

 		evt_type = __le16_to_cpu(ev->evt_type);
 		legacy_evt_type = ext_evt_type_to_legacy(evt_type);
 		if (legacy_evt_type != LE_ADV_INVALID) {

Kind regards,
Tomas

Hi,

sorry for the multiple emails but I have checked again the code and looks like process_adv_report() reads from ev->data for a size of ev->length.

I attach a patch that applies the bound check to both hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().

Quoted 9 lines · Tomas Bortoli

Hi,

sorry for the multiple emails but I have checked again the code and looks like process_adv_report() reads from ev->data for a size of ev->length.

I attach a patch that applies the bound check to both hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().

You're right that both need to be fixed.

net/bluetooth/hci_event.c static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5348

Quoted 9 lines

net/bluetooth/hci_event.c +1 -0

index 609fd6871c5a..275926e0753e 100644
@@ -5345,6 +5345,7 @@ 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];
+	u8 *end = &skb->data[skb->len - 1];

                             ^^^^^^^^^^^^

Quoted 7 lines

net/bluetooth/hci_event.c +0 -0

 	hci_dev_lock(hdev);
 

@@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		struct hci_ev_le_advertising_info *ev = ptr;
 		s8 rssi;
 

net/bluetooth/hci_event.c static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5356

Quoted 1 line

net/bluetooth/hci_event.c +1 -0

+		if (ev->data + ev->length > end)

No, this isn't right. You've removed the + 1 and you've introduced an additional "sbk->len - 1" so we're off by two... The test is supposed to be:

start + length_read > start + length_of_buffer

So the end has to be &skb->data[skb->len]. The "+ 1" comes from later in the function when we do:

		ptr += sizeof(*ev) + ev->length + 1;
                                               ^^^^

I don't where the "+ 1" comes from, but I know the condition and the increment should match. We could use ev->data instead of "ptr + sizeof(*ev)" but to me, because the mysterious "+ 1" then it seems more readable to match the increment exactly...

Quoted 36 lines · Dan Carpenter

Quoted 9 lines · Tomas Bortoli

Hi,

sorry for the multiple emails but I have checked again the code and looks like process_adv_report() reads from ev->data for a size of ev->length.

I attach a patch that applies the bound check to both hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().

You're right that both need to be fixed.

net/bluetooth/hci_event.c static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5348

Quoted 9 lines

net/bluetooth/hci_event.c +1 -0

index 609fd6871c5a..275926e0753e 100644
@@ -5345,6 +5345,7 @@ 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];
+	u8 *end = &skb->data[skb->len - 1];

                             ^^^^^^^^^^^^

net/bluetooth/hci_event.c static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5356

Quoted 8 lines

net/bluetooth/hci_event.c +1 -0

 	hci_dev_lock(hdev);
 

@@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		struct hci_ev_le_advertising_info *ev = ptr;
 		s8 rssi;
 
+		if (ev->data + ev->length > end)

No, this isn't right. You've removed the + 1 and you've introduced an additional "sbk->len - 1" so we're off by two... The test is supposed to be:

Quoted 2 lines

start + length_read > start + length_of_buffer

afaict: ev->data = start and length_read = ev->length
and the right side of the condition is the upper limit. "end" as defined in my patch is the last readable byte of skb->data (or am I wrong on this too?)

Quoted 30 lines · Dan Carpenter

Quoted 9 lines · Tomas Bortoli

Hi,

sorry for the multiple emails but I have checked again the code and looks like process_adv_report() reads from ev->data for a size of ev->length.

I attach a patch that applies the bound check to both hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().

You're right that both need to be fixed.

net/bluetooth/hci_event.c static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5348

Quoted 9 lines

net/bluetooth/hci_event.c +1 -0

index 609fd6871c5a..275926e0753e 100644
@@ -5345,6 +5345,7 @@ 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];
+    u8 *end = &skb->data[skb->len - 1];

                             ^^^^^^^^^^^^

Quoted 7 lines

net/bluetooth/hci_event.c +0 -0

     hci_dev_lock(hdev);
 

@@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
             struct hci_ev_le_advertising_info *ev = ptr;
             s8 rssi;
 

Quoted 8 lines

net/bluetooth/hci_event.c static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb) · line 5356

Quoted 1 line

net/bluetooth/hci_event.c +1 -0

+            if (ev->data + ev->length > end)

No, this isn't right. You've removed the + 1 and you've introduced an additional "sbk->len - 1" so we're off by two... The test is supposed to be:

start + length_read > start + length_of_buffer

afaict: ev->data = start and length_read = ev->length
and the right side of the condition is the upper limit. "end" as defined in my patch is the last readable byte of skb->data (or am I wrong on this too?)

Quoted 10 lines

So the end has to be &skb->data[skb->len]. The "+ 1" comes from later in the function when we do:

              ptr += sizeof(*ev) + ev->length + 1;
                                               ^^^^

I don't where the "+ 1" comes from, but I know the condition and the increment should match. We could use ev->data instead of "ptr + sizeof(*ev)" but to me, because the mysterious "+ 1" then it seems more readable to match the increment exactly...

We really have to first understand why there is that + 1 there. I agree that the condition and the increment should match, otherwise or there is a mistake in the error condition or the increment just skips 1 byte, not reading the last per each cycle, for no reason (very unlikely).

Reading process_adv_report() I spotted some memcpy() and other reads of the memory area that begins at data (ev->data) and ends at (ev->data + length).

Could anybody clarify the logic of that inc ?

Hi Dan,

Quoted 5 lines · Dan Carpenter

There is a potential out of bounds if "ev->length" is too high or if the number of reports are too many.

Fixes c215e9397b00 ("Bluetooth: Process extended ADV report event")

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Tomas Bortoli <[email protected]>

Hi,

Quoted 9 lines · Tomas Bortoli

Hi Dan,

Quoted 5 lines · Dan Carpenter

There is a potential out of bounds if "ev->length" is too high or if the number of reports are too many.

Fixes c215e9397b00 ("Bluetooth: Process extended ADV report event")

Signed-off-by Dan Carpenter <[email protected]>

Quoted 1 line

Reviewed-by Tomas Bortoli <[email protected]>

I sent a patchset to fix all of this kind of OOB:
https://marc.info/?l=linux-netdev&m5314874622831&w=2

Unfortunately I get no response...

Does any of you mind to look at them?

Quoted 12 lines · Cong Wang

Hi,

Quoted 9 lines · Tomas Bortoli

Hi Dan,

Quoted 5 lines · Dan Carpenter

There is a potential out of bounds if "ev->length" is too high or if the number of reports are too many.

Fixes c215e9397b00 ("Bluetooth: Process extended ADV report event")

Signed-off-by Dan Carpenter <[email protected]>

Quoted 9 lines

Quoted 1 line

Reviewed-by Tomas Bortoli <[email protected]>

I sent a patchset to fix all of this kind of OOB:
https://marc.info/?l=linux-netdev&m5314874622831&w=2

Unfortunately I get no response...

Does any of you mind to look at them?

I don't know the rules... When is it ok say:

        if (skb->len < sizeof(*ev))
                return;

and when must we say:

        if (!pskb_may_pull(skb, sizeof(*ev)))
                return;

Btw, get rid of all the likely/unlikely() macros. Then the other style comment would be don't move the "ev = (void *)skb->data;" assignments around. It's ok to say:

Quoted 12 lines · Dan Carpenter

Quoted 10 lines · Cong Wang

Hi,

Quoted 7 lines · Tomas Bortoli

Hi Dan,

Quoted 3 lines · Dan Carpenter

There is a potential out of bounds if "ev->length" is too high or if the number of reports are too many.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

Fixes c215e9397b00 ("Bluetooth: Process extended ADV report event")

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Tomas Bortoli <[email protected]>

I sent a patchset to fix all of this kind of OOB:
https://marc.info/?l=linux-netdev&m5314874622831&w=2

Quoted 8 lines

Quoted 5 lines

Unfortunately I get no response...

Does any of you mind to look at them?

I don't know the rules... When is it ok say:

Quoted 7 lines

        if (skb->len < sizeof(*ev))
                return;

and when must we say:

        if (!pskb_may_pull(skb, sizeof(*ev)))
                return;

The rule is simple: pskb_may_pull() always knows better. In bluetooth case, they are actually equivalent, as the skb's in bluetooth are linear.

Quoted 4 lines

Btw, get rid of all the likely/unlikely() macros. Then the other style comment would be don't move the "ev = (void *)skb->data;" assignments around. It's ok to say:

Similarly, pskb_may_pull() may reallocate skb's, although very unlikely for bluetooth case (skb's are linear). At least it doesn't harm anything we move the skb->data dereference after pskb_may_pull().

I think these likely()/unlikely() are reasonable, ill-formatted packets are rare cases, normal packets deserve such a special care. We use likely()/unlikely() with pskb_may_pull() in many places in networking subsystem, at least.

Anyway, if you don't mind, I can resend my patchset with you Cc'ed.

Thanks.

I think these likely()/unlikely() are reasonable, ill-formatted packets are rare cases, normal packets deserve such a special care. We use likely()/unlikely() with pskb_may_pull() in many places in networking subsystem, at least.

Quoted 4 lines · Cong Wang

I think these likely()/unlikely() are reasonable, ill-formatted packets are rare cases, normal packets deserve such a special care. We use likely()/unlikely() with pskb_may_pull() in many places in networking subsystem, at least.

The likely()/unlikely() annotations are to help the compiler optimize the fast path. They are not there just for decorating the code. We should only use likely()/unlikely() where it makes a difference in benchmarking. Otherwise it's just a style question, right (obviously)?

I tried checkpatch.pl, it has no such a complain.

Quoted 1 line · Cong Wang

I tried checkpatch.pl, it has no such a complain.

Huh?

I sorry, I must have been very unclear if you're asking about checkpatch. Checkpatch is a Perl script. It's basically like grep. It has no idea about fast paths or benchmarking. Let me try explain better.

The likely/unlikely annotations are there to help optimize branch prediction and make the code run faster. In the real world if you can't benchmark or measure or detect a speed improvement then it's not a real speed improvement.

1) HCI events don't happen often enough to where the speed can be easily benchmarked. In that situation, we just write the code as readably as possible instead of trying to micro optimize it.

2) The compiler already does common sense branch prediction. These conditions look straight forward so it probably gets it right. You

should take a look at the object code. The compiler probably gets it right. I bet that these annotations don't even affect the compiled code let alone the benchmarking output.

I know this isn't explained in CodingStyle but some things are assumed. In drivers/ about 1.5% of if statements have likely/unlikely annotations. It really is not normal to add it to everything willy-nilly for no reason.

I'm don't have a lot of time for these discussions. The object code is exactly the same regardless of whether you add the annotations or not.

32c307d27583a624baa3a24eec00402e net/bluetooth/hci_event.o.no_annotation 32c307d27583a624baa3a24eec00402e net/bluetooth/hci_event.o.w_annotation

regards,
dan carpenter

I disagree,

they differ for me.

with unlikely:
f86cf90c9c1ef14063796f4d07ce64bf6952b536e9728d21ba86d3be2e7472d7 net/bluetooth/hci_event.o

without:
6c75194b981294cb933900100448ec335065a35ff9120333558d09aa3dd8b2da net/bluetooth/hci_event.o

Quoted 3 lines

Btw, get rid of all the likely/unlikely() macros. Then the other style comment would be don't move the "ev = (void *)skb->data;" assignments around. It's ok to say:

Similarly, pskb_may_pull() may reallocate skb's, although very unlikely for bluetooth case (skb's are linear). At least it doesn't harm anything we move the skb->data dereference after pskb_may_pull().

Quoted 5 lines · Cong Wang

Quoted 3 lines

Btw, get rid of all the likely/unlikely() macros. Then the other style comment would be don't move the "ev = (void *)skb->data;" assignments around. It's ok to say:

Quoted 4 lines

Similarly, pskb_may_pull() may reallocate skb's, although very unlikely for bluetooth case (skb's are linear). At least it doesn't harm anything we move the skb->data dereference after pskb_may_pull().

It harms readability.

Quoted 12 lines · Dan Carpenter

Quoted 9 lines · Cong Wang

Quoted 3 lines

Btw, get rid of all the likely/unlikely() macros. Then the other style comment would be don't move the "ev = (void *)skb->data;" assignments around. It's ok to say:

Similarly, pskb_may_pull() may reallocate skb's, although very unlikely for bluetooth case (skb's are linear). At least it doesn't harm anything we move the skb->data dereference after pskb_may_pull().

Quoted 1 line

It harms readability.

Why? I can't see how it harms readability if you have pskb_may_pull() in mind that it potentially reallocates skb->data.

Quoted 13 lines · Cong Wang

Quoted 12 lines · Dan Carpenter

Quoted 9 lines · Cong Wang

Quoted 3 lines

Btw, get rid of all the likely/unlikely() macros. Then the other style comment would be don't move the "ev = (void *)skb->data;" assignments around. It's ok to say:

Similarly, pskb_may_pull() may reallocate skb's, although very unlikely for bluetooth case (skb's are linear). At least it doesn't harm anything we move the skb->data dereference after pskb_may_pull().

Quoted 4 lines

Quoted 1 line

It harms readability.

Why? I can't see how it harms readability if you have pskb_may_pull() in mind that it potentially reallocates skb->data.

You're making the code more complicated because you're pretending that we didn't linearize the skb data already... :/