[RFC] Bluetooth: don't increment twice in eir_has_data_type()

[RFC] Bluetooth: don't increment twice in eir_has_data_type()

[Dan Carpenter]

I don't have this hardware, and I'm not familiar with this code.  It
just looked suspicious that we move the parsed counter forward faster
than the data pointer.  We do it once in middle the loop and again as
the for loop incrementer.  The effect is that we only search half the
data_len before returning false.

Also I've changed the breaks to just return false directly because it
made the code easier to follow.

I wrote this patch based on a guess of what the data might look like so
it's very likely wrong.  Could you maybe treat it as a bug report and
give me a Reported-by?

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8dc07fa..ff79f41 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -912,22 +912,17 @@ static inline void hci_role_switch_cfm(struct hci_conn *conn, __u8 status,
 static inline bool eir_has_data_type(u8 *data, size_t data_len, u8 type)
 {
 	u8 field_len;
-	size_t parsed;
+	size_t parsed = 0;
 
-	for (parsed = 0; parsed < data_len - 1; parsed += field_len) {
+	while (parsed < data_len - 1) {
 		field_len = data[0];
 
 		if (field_len == 0)
-			break;
-
-		parsed += field_len + 1;
-
-		if (parsed > data_len)
-			break;
-
+			return false;
 		if (data[1] == type)
 			return true;
 
+		parsed += field_len + 1;
 		data += field_len + 1;
 	}
 

Re: [RFC] Bluetooth: don't increment twice in eir_has_data_type()

[Johan Hedberg]

Hi Dan,

On Tue, Mar 20, 2012, Dan Carpenter wrote:
> I don't have this hardware, and I'm not familiar with this code.  It
> just looked suspicious that we move the parsed counter forward faster
> than the data pointer.  We do it once in middle the loop and again as
> the for loop incrementer.  The effect is that we only search half the
> data_len before returning false.
> 
> Also I've changed the breaks to just return false directly because it
> made the code easier to follow.
> 
> I wrote this patch based on a guess of what the data might look like so
> it's very likely wrong.  Could you maybe treat it as a bug report and
> give me a Reported-by?
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 8dc07fa..ff79f41 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -912,22 +912,17 @@ static inline void hci_role_switch_cfm(struct hci_conn *conn, __u8 status,
>  static inline bool eir_has_data_type(u8 *data, size_t data_len, u8 type)
>  {
>  	u8 field_len;
> -	size_t parsed;
> +	size_t parsed = 0;
>  
> -	for (parsed = 0; parsed < data_len - 1; parsed += field_len) {
> +	while (parsed < data_len - 1) {
>  		field_len = data[0];
>  
>  		if (field_len == 0)
> -			break;
> -
> -		parsed += field_len + 1;
> -
> -		if (parsed > data_len)
> -			break;
> -
> +			return false;
>  		if (data[1] == type)
>  			return true;
>  
> +		parsed += field_len + 1;
>  		data += field_len + 1;
>  	}
>  

Looks like the original code is indeed buggy, no idea how I didn't
notice something that obvious. Your patch does however seem to change
the behavior a bit, a valid tag would be detected even though its length
would be invalid (pointing outside of the supplied data). Not sure if
that's so critical though since the important thing is to keep the code
from doing anything nasty when supplied invalid data.

Johan

Re: [RFC] Bluetooth: don't increment twice in eir_has_data_type()

[Dan Carpenter]

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

On Wed, Mar 21, 2012 at 07:06:32PM -0300, Johan Hedberg wrote:
> Looks like the original code is indeed buggy, no idea how I didn't
> notice something that obvious. Your patch does however seem to change
> the behavior a bit, a valid tag would be detected even though its length
> would be invalid (pointing outside of the supplied data). Not sure if
> that's so critical though since the important thing is to keep the code
> from doing anything nasty when supplied invalid data.
> 

We should check the length.  It will just cause headaches if we
don't.

It would be simple enough for me to put back the check I removed
from the middle of the loop.  But the thing is I wasn't sure how all
the + 1 and - 1 things fit together so I didn't feel good about
signing off on this.  Could you send a patch?  That way I get a
reported-by tag but if there are any problems you get blamed while I
deny knowing anything about it.  ;)

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] Bluetooth: don't increment twice in eir_has_data_type()

[Johan Hedberg]

Hi Dan,

On Thu, Mar 22, 2012, Dan Carpenter wrote:
> On Wed, Mar 21, 2012 at 07:06:32PM -0300, Johan Hedberg wrote:
> > Looks like the original code is indeed buggy, no idea how I didn't
> > notice something that obvious. Your patch does however seem to change
> > the behavior a bit, a valid tag would be detected even though its length
> > would be invalid (pointing outside of the supplied data). Not sure if
> > that's so critical though since the important thing is to keep the code
> > from doing anything nasty when supplied invalid data.
> > 
> 
> We should check the length.  It will just cause headaches if we
> don't.
> 
> It would be simple enough for me to put back the check I removed
> from the middle of the loop.  But the thing is I wasn't sure how all
> the + 1 and - 1 things fit together so I didn't feel good about
> signing off on this.  Could you send a patch?  That way I get a
> reported-by tag but if there are any problems you get blamed while I
> deny knowing anything about it.  ;)

Done. I also sent a second patch for another issue with the function.

In case you're interested the EIR data format is quite simple,
consisting of a sequence of data structures with a format of:

	| data_len (1 byte) | data (data_len bytes) |

The first byte of "data" is the type of data structure in question. EIR
data buffers do not need to be completely filled up, i.e. they can
contain a non-significant part at the end, the beginning of which can be
detected by a data_len field with the value of 0.

Johan