[PATCH 0/2] Fix crash in profile descriptor list parsing

[PATCH 0/2] Fix crash in profile descriptor list parsing

[Frédéric Dalleau]

Hi,

I've tried to connect the HFP audio gateway on a Mac running Lion.
bluetoothd crashed. So did sdptool browse.

I guess the whole device has passed qualification, and is widely available, so:
* First patch fixes the crash.
* Second one makes sure profile version can be read.

Hope this helps!

Regards,
Frédéric

Output after running in GDB:

Service Name: Hands Free Audio Gateway
Service RecHandle: 0x10003
Service Class ID List:
  "Handsfree Audio Gateway" (0x111f)
  "Generic Audio" (0x1203)
Protocol Descriptor List:
  "L2CAP" (0x0100)
  "RFCOMM" (0x0003)
    Channel: 2
Language Base Attr List:
  code_ISO639: 0x656e
  encoding:    0x6a
  base_offset: 0x100

Program received signal SIGSEGV, Segmentation fault.
sdp_get_profile_descs (rec=0x80039520, profDescSeq=0xbfffef38) at lib/sdp.c:2070
2070				sdp_data_t *pVnum = seq->val.dataseq->next;
(gdb) bt
#0  sdp_get_profile_descs (rec=0x80039520, profDescSeq=0xbfffef38) at lib/sdp.c:2070
#1  0x80003f2e in print_service_attr (rec=0x80039520) at tools/sdptool.c:1129
#2  0x80005210 in do_search (bdaddr=0xbffff186, context=0xbffff164) at tools/sdptool.c:3803
#3  0x80005627 in cmd_browse (argc=1, argv=<optimized out>) at tools/sdptool.c:3898
#4  0x800028f4 in main (argc=2, argv=<optimized out>) at tools/sdptool.c:4277
(gdb) l
2065	
2066			if (SDP_IS_UUID(seq->dtd)) {
2067				uuid = &seq->val.uuid;
2068			} else {
2069				sdp_data_t *puuid = seq->val.dataseq;
2070				sdp_data_t *pVnum = seq->val.dataseq->next;
2071				if (puuid && pVnum) {
2072					uuid = &puuid->val.uuid;
2073					version = pVnum->val.uint16;
2074				}
(gdb) p *puuid
Cannot access memory at address 0x105

The following is an extract of hcidump of record wich caused crash:
              aid 0x0009 (BTProfileDescList)
                 < uuid-16 0x111e (Handsfree) uint 0x105 >

by contrast, other profile version looks like this and are fine with BlueZ:
              aid 0x0009 (BTProfileDescList)
                 < < uuid-16 0x110e (AVRemote) uint 0x103 > >

              aid 0x0009 (BTProfileDescList)
                 < < uuid-16 0x1108 (Headset) uint 0x102 > >

Frédéric Dalleau (2):
  sdp: Check type of sdp data before dereferencing
  sdp: Fix sdp_get_profile_descs for Mac Os X Lion

 lib/sdp.c |    7 ++++++-
 lib/sdp.h |    1 +
 2 files changed, 7 insertions(+), 1 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] sdp: Check type of sdp data before dereferencing

[Frédéric Dalleau]

---
 lib/sdp.c |    2 +-
 lib/sdp.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index a48ee14..57f630a 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -2065,7 +2065,7 @@ int sdp_get_profile_descs(const sdp_record_t *rec, sdp_list_t **profDescSeq)
 
 		if (SDP_IS_UUID(seq->dtd)) {
 			uuid = &seq->val.uuid;
-		} else {
+		} else if (SDP_IS_SEQ(seq->dtd)) {
 			sdp_data_t *puuid = seq->val.dataseq;
 			sdp_data_t *pVnum = seq->val.dataseq->next;
 			if (puuid && pVnum) {
diff --git a/lib/sdp.h b/lib/sdp.h
index 5f7d271..2fe74d5 100644
--- a/lib/sdp.h
+++ b/lib/sdp.h
@@ -432,6 +432,7 @@ typedef struct {
 } uuid_t;
 
 #define SDP_IS_UUID(x) ((x) == SDP_UUID16 || (x) == SDP_UUID32 || (x) ==SDP_UUID128)
+#define SDP_IS_SEQ(x)  ((x) == SDP_SEQ8 || (x) == SDP_SEQ16 || (x) == SDP_SEQ32)
 
 typedef struct _sdp_list sdp_list_t;
 struct _sdp_list {
-- 
1.7.5.4

[PATCH 2/2] sdp: Fix sdp_get_profile_descs for Mac Os X Lion

[Frédéric Dalleau]

---
 lib/sdp.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index 57f630a..97c0a08 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -2064,7 +2064,12 @@ int sdp_get_profile_descs(const sdp_record_t *rec, sdp_list_t **profDescSeq)
 		uint16_t version = 0x100;
 
 		if (SDP_IS_UUID(seq->dtd)) {
+			sdp_data_t *next = seq->next;
 			uuid = &seq->val.uuid;
+			if (next && next->dtd == SDP_UINT16) {
+				version = next->val.uint16;
+				seq = next;
+			}
 		} else if (SDP_IS_SEQ(seq->dtd)) {
 			sdp_data_t *puuid = seq->val.dataseq;
 			sdp_data_t *pVnum = seq->val.dataseq->next;
-- 
1.7.5.4

Re: [PATCH 0/2] Fix crash in profile descriptor list parsing

[Dalleau, Frederic]

Hi,

> I've tried to connect the HFP audio gateway on a Mac running Lion.
> bluetoothd crashed. So did sdptool browse.

Any updates please?

Thanks,
Frédéric

Re: [PATCH 2/2] sdp: Fix sdp_get_profile_descs for Mac Os X Lion

[Johan Hedberg]

Hi Frédéric,

On Mon, Feb 20, 2012, Frédéric Dalleau wrote:
> ---
>  lib/sdp.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/sdp.c b/lib/sdp.c
> index 57f630a..97c0a08 100644
> --- a/lib/sdp.c
> +++ b/lib/sdp.c
> @@ -2064,7 +2064,12 @@ int sdp_get_profile_descs(const sdp_record_t *rec, sdp_list_t **profDescSeq)
>  		uint16_t version = 0x100;
>  
>  		if (SDP_IS_UUID(seq->dtd)) {
> +			sdp_data_t *next = seq->next;
>  			uuid = &seq->val.uuid;
> +			if (next && next->dtd == SDP_UINT16) {
> +				version = next->val.uint16;
> +				seq = next;
> +			}
>  		} else if (SDP_IS_SEQ(seq->dtd)) {
>  			sdp_data_t *puuid = seq->val.dataseq;
>  			sdp_data_t *pVnum = seq->val.dataseq->next;

I've applied the first patch since that's quite straight forward but I'm
struggling a bit with this second one. Firstly you should include the
hcidump into the commit message as well as a proper explanation. An
empty commit message (just summary line) is only acceptable for extremely
trivial patches (and this is far from it).

I *think* I get the main part of the patch, but one thing that's unclear
is why do you assign next to seq instead of letting the for-loop do it.
The second branch of the if-statement doesn't do it either so why does
the first need it? Again, you could have avoided these questions from me
if you had included an appropriate code comment. Whenever there's
something counterintuitive in the code it's a good idea to have such a
comment.

Johan

Re: [PATCH 2/2] sdp: Fix sdp_get_profile_descs for Mac Os X Lion

[Dalleau, Frederic]

Hi Johan,

> I've applied the first patch since that's quite straight forward but I'm
> struggling a bit with this second one. Firstly you should include the
> hcidump into the commit message as well as a proper explanation. An
> empty commit message (just summary line) is only acceptable for extremely
> trivial patches (and this is far from it).
>
> I *think* I get the main part of the patch, but one thing that's unclear
> is why do you assign next to seq instead of letting the for-loop do it.
> The second branch of the if-statement doesn't do it either so why does
> the first need it? Again, you could have avoided these questions from me
> if you had included an appropriate code comment. Whenever there's
> something counterintuitive in the code it's a good idea to have such a
> comment.
>

This patch just makes sure the correct profile version number is retrieved and
is specific to Mac Os X Lion HFP AG record. SDP is a very sensible piece of
code, and this needs double checking. I'm not even sure that the record itself
is valid so I would surely understand that in order to enforce the standard this
one is not applied.

>From my understanding, the SDP profile descriptor is a list. The for loop
iterates that list. It is unclear in the HFP specification what is contained.
Until now, bluez expected a collection of either uuid or lists with a uuid and a
version number. For example list=(uuid, uuid, (uuid, version), (uuid, version))

The patch fixes the situation where the profile descriptor list contains a uuid
AND a version number both on the root level and not enclosed in a sublist. If
the next item after uuid is uint16, then this is likely to be the
version number of the profile which uuid is given.
list=(uuid, uuid, version, uuid, (uuid, version))

Very easy to reproduce : just run sdptool browse and check the version number of
HFP AG is not 0x105.

However, I'll provide you an updated version with hcidump and a more intuitive
 commit message as soon as I can put my hands on one of these machines again.

Regards,
Frédéric