[PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[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 <dan.carpenter@oracle.com>
---
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(+)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 609fd6871c5a..ee945b3d12e1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -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);
 
@@ -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;
 		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) {
-- 
2.17.1

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Tomas Bortoli]

Hi Dan,

On 3/30/19 8:25 AM, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
Reviewed-By: Tomas Bortoli <tomasbortoli@gmail.com>

> ---
> 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(+)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 609fd6871c5a..ee945b3d12e1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -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);
>  
> @@ -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!


>  		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

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Tomas Bortoli]

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

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().

Let me know your opinion,
Best regards,
Tomas

On 3/30/19 10:20 AM, Tomas Bortoli wrote:
> Hi Dan,
> 
> On 3/30/19 8:25 AM, Dan Carpenter wrote:
>> 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 <dan.carpenter@oracle.com>
> Reviewed-By: Tomas Bortoli <tomasbortoli@gmail.com>
> 
>> ---
>> 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(+)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 609fd6871c5a..ee945b3d12e1 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -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);
>>  
>> @@ -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!
> 
> 
>>  		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
> 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: adv.patch --]
[-- Type: text/x-patch; name="adv.patch", Size: 1348 bytes --]

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 609fd6871c5a..275926e0753e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -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];
 
 	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)
+			break;
+
 		if (ev->length <= HCI_MAX_AD_LENGTH) {
 			rssi = ev->data[ev->length];
 			process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
@@ -5417,6 +5421,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];
+	u8 *end = &skb->data[skb->len - 1];
 
 	hci_dev_lock(hdev);
 
@@ -5425,6 +5430,9 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		u8 legacy_evt_type;
 		u16 evt_type;
 
+		if (ev->data + ev->length > end)
+			break;
+
 		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) {

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
> 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.

> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 609fd6871c5a..275926e0753e 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -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];
                             ^^^^^^^^^^^^
>  
>  	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:

	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...

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Tomas Bortoli]

On 4/1/19 8:32 AM, Dan Carpenter wrote:
> On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
>> 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.
> 
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 609fd6871c5a..275926e0753e 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -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];
>                              ^^^^^^^^^^^^
>>  
>>  	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:
> 
> 	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?)

> 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 ?

Cheers,
Tomas

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Mon, Apr 01, 2019 at 07:24:47PM +0200, Tomas Bortoli wrote:
> On 4/1/19 8:32 AM, Dan Carpenter wrote:
> > On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
> >> 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.
> > 
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index 609fd6871c5a..275926e0753e 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -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];
> >                              ^^^^^^^^^^^^
> >>  
> >>  	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:
> > 
> > 	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?)
> 

You have:

	ptr + length > &skb->data[skb->len - 1];

Imagine we "ptr = &skb->data[skb->len - 1]" that means we can read one
more byte.  But in that case "if (ptr + 1 > &skb->data[skb->len - 1])"
is true so we break instead of allowing the read.  Idiomatically > is
for length and >= is for indexes...

Btw, unrelated but in hci_le_adv_report_evt() if we hit the
HCI_MAX_AD_LENGTH condition we should just break as well.  Everything
after that is going to be guess work and garbage.  No point in trying to
parse it.  IOW:

		if (ptr + sizeof(*ev) + ev->length + 1 > end ||
		    ev->length > HCI_MAX_AD_LENGTH)
			break;

I was planning to resend my patch and the fixes to hci_le_adv_report_evt()
with your Reported-by tonight as two separate patches.  It just makes it
easier to backport if we have a different Fixes tag for both functions.
But then I decided to wait until tomorrow to see if anyone knew what the
+ 1 was about...

regards,
dan carpenter
 

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Jaganath K]

Hi,

On Mon, Apr 1, 2019 at 10:54 PM Tomas Bortoli <tomasbortoli@gmail.com> wrote:
>
> On 4/1/19 8:32 AM, Dan Carpenter wrote:
> > On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
> >> 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.
> >
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index 609fd6871c5a..275926e0753e 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -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];
> >                              ^^^^^^^^^^^^
> >>
> >>      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:
> >
> >       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?)
>
> > 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 ?
>
"+ 1" is required in adv_report_evt since there is one more field
"rssi" after data
so you need + 1 to point to next report, where as it is not required
in ext_adv_report_evt
since rssi is present before data. I have already raised a patch to
fix it the in ML.

Thanks,
Jaganath

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Cong Wang]

Hi,

On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <tomasbortoli@gmail.com> wrote:
>
> Hi Dan,
>
> On 3/30/19 8:25 AM, Dan Carpenter wrote:
> > 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 <dan.carpenter@oracle.com>
> Reviewed-By: Tomas Bortoli <tomasbortoli@gmail.com>

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

Unfortunately I get no response...

Does any of you mind to look at them?

Thanks.

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Mon, Apr 01, 2019 at 11:03:53AM -0700, Cong Wang wrote:
> Hi,
> 
> On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <tomasbortoli@gmail.com> wrote:
> >
> > Hi Dan,
> >
> > On 3/30/19 8:25 AM, Dan Carpenter wrote:
> > > 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 <dan.carpenter@oracle.com>
> > Reviewed-By: Tomas Bortoli <tomasbortoli@gmail.com>
> 
> I sent a patchset to fix all of this kind of OOB:
> https://marc.info/?l=linux-netdev&m=155314874622831&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:

	struct hci_ev_pin_code_req *ev = (void *)skb->data;
	struct hci_conn *conn;

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

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Cong Wang]

On Mon, Apr 1, 2019 at 11:33 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Apr 01, 2019 at 11:03:53AM -0700, Cong Wang wrote:
> > Hi,
> >
> > On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <tomasbortoli@gmail.com> wrote:
> > >
> > > Hi Dan,
> > >
> > > On 3/30/19 8:25 AM, Dan Carpenter wrote:
> > > > 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 <dan.carpenter@oracle.com>
> > > Reviewed-By: Tomas Bortoli <tomasbortoli@gmail.com>
> >
> > I sent a patchset to fix all of this kind of OOB:
> > https://marc.info/?l=linux-netdev&m=155314874622831&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;


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.


>
> 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.

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Tomas Bortoli]

Hi Cong,

On 4/2/19 7:42 PM, Cong Wang wrote:
> On Mon, Apr 1, 2019 at 11:33 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>
>> On Mon, Apr 01, 2019 at 11:03:53AM -0700, Cong Wang wrote:
>>> Hi,
>>>
>>> On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <tomasbortoli@gmail.com> wrote:
>>>>
>>>> Hi Dan,
>>>>
>>>> On 3/30/19 8:25 AM, Dan Carpenter wrote:
>>>>> 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 <dan.carpenter@oracle.com>
>>>> Reviewed-By: Tomas Bortoli <tomasbortoli@gmail.com>
>>>
>>> I sent a patchset to fix all of this kind of OOB:
>>> https://marc.info/?l=linux-netdev&m=155314874622831&w=2

Reviewed-by: Tomas Bortoli <tomasbortoli@gmail.com>

All 3 looks good to me, nice work! Overall, it seems that with these 3
all the RX paths in hci_event.c are bound checked.

>>>
>>> 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;
> 
> 
> 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.
> 
> 
>>
>> 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.
> 

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> 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)?
And it's better style to write things as simply as possible.

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Cong Wang]

On Tue, Apr 2, 2019 at 1:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > 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)?

That is not a requirement.

Unless you have a strong argument to believe likely()/unlikely()
doesn't help in this specific case (ill-formatted packets), we should
by default use it.

Coding style is not a strong argument, it is purely a taste. At least,
does CodingStyle forbid to use it in this case? I tried checkpatch.pl,
it has no such a complain.

Thanks.

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Wed, Apr 03, 2019 at 03:55:20PM -0700, Cong Wang wrote:
> 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.

So in this case, we are not adding likely and unlikely for their
intended purpose of improving benchmarks.  I guess we are instead adding
them just for the human readers?  But most people can immediately see
that this is an error path so they don't need the annotations.  In fact
the annotations obscure what the error condition is so they hurt
readability.  Now there are just too many parentheses to keep track of.

	if (unlikely(!pskb_may_pull(skb, sizeof(*rp))))
		return;

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

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.

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Cong Wang]

On Thu, Apr 4, 2019 at 1:06 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Apr 03, 2019 at 03:55:20PM -0700, Cong Wang wrote:
> > I tried checkpatch.pl, it has no such a complain.
>
> Huh?

Huh +1?

>
> 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.

Sure, you said coding style, then I brought up checkpatch.pl.
If it hurts you, then I am sorry. How do you check your coding
style without checkpatch.pl? I am happy to learn. Personally I
just trust checkpatch.pl, because I don't want to waste my time
on coding styles, I never say I like it or not, I just need a tool.

Thanks for teaching me what checkpatch.pl is, although I have
been using it for so many years, it is really really helpful.


>
> 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.


Personally I disagree with you on this point. You don't have to measure,
you just have to understand the code. In this case, bluetooth has linear
and sane skb's in _normal_ case, you don't disagree, do you?

So even _if_ likely()/unlikely() is only for telling which is the normal
case, it is still helpful. In this case, we want to tell non-linear skb's are
unlikely in bluetooth, you know this is true, as linear is always the case;
and, we want to tell ill-formatted skb's are unlikely too, and you know
this is also true.


>
> 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.

Same. It is not about benchmarking.

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

I'd be surprised if the compiler could know skb's are always linear
in this particular case.


>    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.


If you are suggesting we should remove all likely()/unlikely()
from pskb_may_pull() callers, please go ahead to submit a patch
to remove them all, and see if David accepts it.


>
> 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 don't know why you want to make the topic as broad as all
likely()/unlikely(), we are discussing likely()/unlikely() with
pskb_may_pull() together, and particularly in bluetooth.


Thanks.

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

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

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Tomas Bortoli]

I disagree,

they differ for me.

with unlikely:
f86cf90c9c1ef14063796f4d07ce64bf6952b536e9728d21ba86d3be2e7472d7
net/bluetooth/hci_event.o

without:
6c75194b981294cb933900100448ec335065a35ff9120333558d09aa3dd8b2da
net/bluetooth/hci_event.o


On 4/5/19 10:48 PM, Dan Carpenter wrote:
> 
> 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
> 

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Fri, Apr 05, 2019 at 11:05:53PM +0200, Tomas Bortoli wrote:
> I disagree,
> 
> they differ for me.
> 
> with unlikely:
> f86cf90c9c1ef14063796f4d07ce64bf6952b536e9728d21ba86d3be2e7472d7
> net/bluetooth/hci_event.o
> 
> without:
> 6c75194b981294cb933900100448ec335065a35ff9120333558d09aa3dd8b2da
> net/bluetooth/hci_event.o
> 

Are you sure you didn't change any line numbers or something?

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

I only deleted one unlikely() from around an unlikely(!pskb_may_pull())
check.  I made sure that the line numbers and debug symbols all stayed
exactly the same...  I just re-ran my experiment with the same results.

It's weird that you're getting different object code.  This stuff isn't
a new feature in GCC, it's at least 10 years old.

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > 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().
> 

It harms readability.

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Cong Wang]

On Tue, Apr 2, 2019 at 1:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > > 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().
> >
>
> 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.

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Dan Carpenter]

On Wed, Apr 03, 2019 at 03:51:18PM -0700, Cong Wang wrote:
> On Tue, Apr 2, 2019 at 1:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > > > 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().
> > >
> >
> > 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...  :/

regards,
dan carpenter

Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Cong Wang]

On Wed, Apr 3, 2019 at 11:35 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Apr 03, 2019 at 03:51:18PM -0700, Cong Wang wrote:
> > On Tue, Apr 2, 2019 at 1:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > > > > 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().
> > > >
> > >
> > > 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...  :/

I am not pretending it, I just want to use a standard networking API
which covers all cases.