[PATCH BlueZ 1/4] input: Change extract_hid_record() return type

[PATCH BlueZ 1/4] input: Change extract_hid_record() return type

[Anderson Lizardo]

This will allow returning error values when necessary.
---
 profiles/input/device.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 759603a..9e485cf 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -202,7 +202,7 @@ static void epox_endian_quirk(unsigned char *data, int size)
 	}
 }
 
-static void extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
+static int extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
 {
 	sdp_data_t *pdlist, *pdlist2;
 	uint8_t attr_val;
@@ -255,6 +255,8 @@ static void extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
 			epox_endian_quirk(req->rd_data, req->rd_size);
 		}
 	}
+
+	return 0;
 }
 
 static int ioctl_connadd(struct hidp_connadd_req *req)
@@ -346,8 +348,13 @@ static int hidp_add_connection(struct input_device *idev)
 	rec = record_from_string(str);
 	g_free(str);
 
-	extract_hid_record(rec, req);
+	err = extract_hid_record(rec, req);
 	sdp_record_free(rec);
+	if (err < 0) {
+		error("Could not parse HID SDP record: %s (%d)", strerror(-err),
+									-err);
+		goto cleanup;
+	}
 
 	req->vendor = btd_device_get_vendor(idev->device);
 	req->product = btd_device_get_product(idev->device);
-- 
1.7.9.5

[PATCH BlueZ 2/4] input: Move HID device name creation to separate function

[Anderson Lizardo]

The attributes used for composing the device name are all optional, and
thus need to be properly validated. A separate function will avoid
polluting the caller with variables used only for device name
composition.
---
 profiles/input/device.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 9e485cf..7a7e995 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -202,10 +202,9 @@ static void epox_endian_quirk(unsigned char *data, int size)
 	}
 }
 
-static int extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
+static int create_hid_dev_name(sdp_record_t *rec, struct hidp_connadd_req *req)
 {
 	sdp_data_t *pdlist, *pdlist2;
-	uint8_t attr_val;
 
 	pdlist = sdp_data_get(rec, SDP_ATTR_SVCDESC_PRIMARY);
 	pdlist2 = sdp_data_get(rec, SDP_ATTR_PROVNAME_PRIMARY);
@@ -222,6 +221,19 @@ static int extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
 							pdlist->val.str);
 	}
 
+	return 0;
+}
+
+static int extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
+{
+	sdp_data_t *pdlist;
+	uint8_t attr_val;
+	int err;
+
+	err = create_hid_dev_name(rec, req);
+	if (err < 0)
+		DBG("No valid Service Name or Service Description found");
+
 	pdlist = sdp_data_get(rec, SDP_ATTR_HID_PARSER_VERSION);
 	req->parser = pdlist ? pdlist->val.uint16 : 0x0100;
 
-- 
1.7.9.5

[PATCH BlueZ 3/4] input: Use SDP library functions for reading attributes

[Anderson Lizardo]

These functions do the necessary validation that is lacking from
previous code.
---
 profiles/input/device.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 7a7e995..eaf5681 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -204,21 +204,19 @@ static void epox_endian_quirk(unsigned char *data, int size)
 
 static int create_hid_dev_name(sdp_record_t *rec, struct hidp_connadd_req *req)
 {
-	sdp_data_t *pdlist, *pdlist2;
-
-	pdlist = sdp_data_get(rec, SDP_ATTR_SVCDESC_PRIMARY);
-	pdlist2 = sdp_data_get(rec, SDP_ATTR_PROVNAME_PRIMARY);
-	if (pdlist && pdlist2 &&
-			strncmp(pdlist->val.str, pdlist2->val.str, 5) != 0) {
-		snprintf(req->name, sizeof(req->name), "%s %s",
-					pdlist2->val.str, pdlist->val.str);
-	} else {
-		if (!pdlist)
-			pdlist = sdp_data_get(rec, SDP_ATTR_SVCNAME_PRIMARY);
+	char sdesc[sizeof(req->name)];
+
+	if (sdp_get_service_desc(rec, sdesc, sizeof(sdesc)) == 0) {
+		char pname[sizeof(req->name)];
 
-		if (pdlist)
-			snprintf(req->name, sizeof(req->name), "%s",
-							pdlist->val.str);
+		if (sdp_get_provider_name(rec, pname, sizeof(pname)) == 0 &&
+						strncmp(sdesc, pname, 5) != 0)
+			snprintf(req->name, sizeof(req->name), "%s %s", pname,
+									sdesc);
+		else
+			snprintf(req->name, sizeof(req->name), "%s", sdesc);
+	} else {
+		return sdp_get_service_name(rec, req->name, sizeof(req->name));
 	}
 
 	return 0;
-- 
1.7.9.5

[PATCH BlueZ 4/4] input: Validate SDP HIDDescriptorList subattributes

[Anderson Lizardo]

It should not be assumed that remote SDP attributes are in a compliant
format. This fixes a couple of invalid pointer access on invalid data.
---
 profiles/input/device.c |   60 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index eaf5681..bd32623 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -222,6 +222,49 @@ static int create_hid_dev_name(sdp_record_t *rec, struct hidp_connadd_req *req)
 	return 0;
 }
 
+/* See HID profile specification v1.0, "7.11.6 HIDDescriptorList" for details
+ * on the attribute format. */
+static int extract_hid_desc_data(sdp_record_t *rec,
+						struct hidp_connadd_req *req)
+{
+	sdp_data_t *d;
+
+	d = sdp_data_get(rec, SDP_ATTR_HID_DESCRIPTOR_LIST);
+	if (!d)
+		goto invalid_desc;
+
+	if (d->dtd < SDP_SEQ8 || d->dtd > SDP_SEQ32)
+		goto invalid_desc;
+
+	/* First HIDDescriptor */
+	d = d->val.dataseq;
+	if (d->dtd < SDP_SEQ8 || d->dtd > SDP_SEQ32)
+		goto invalid_desc;
+
+	/* ClassDescriptorType */
+	d = d->val.dataseq;
+	if (d->dtd != SDP_UINT8)
+		goto invalid_desc;
+
+	/* ClassDescriptorData */
+	d = d->next;
+	if (!d || d->dtd < SDP_TEXT_STR8 || d->dtd > SDP_TEXT_STR32)
+		goto invalid_desc;
+
+	req->rd_data = g_try_malloc0(d->unitSize);
+	if (req->rd_data) {
+		memcpy(req->rd_data, d->val.str, d->unitSize);
+		req->rd_size = d->unitSize;
+		epox_endian_quirk(req->rd_data, req->rd_size);
+	}
+
+	return 0;
+
+invalid_desc:
+	error("Missing or invalid HIDDescriptorList SDP attribute");
+	return -EINVAL;
+}
+
 static int extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
 {
 	sdp_data_t *pdlist;
@@ -251,20 +294,9 @@ static int extract_hid_record(sdp_record_t *rec, struct hidp_connadd_req *req)
 	if (attr_val)
 		req->flags |= (1 << HIDP_BOOT_PROTOCOL_MODE);
 
-	pdlist = sdp_data_get(rec, SDP_ATTR_HID_DESCRIPTOR_LIST);
-	if (pdlist) {
-		pdlist = pdlist->val.dataseq;
-		pdlist = pdlist->val.dataseq;
-		pdlist = pdlist->next;
-
-		req->rd_data = g_try_malloc0(pdlist->unitSize);
-		if (req->rd_data) {
-			memcpy(req->rd_data, (unsigned char *) pdlist->val.str,
-								pdlist->unitSize);
-			req->rd_size = pdlist->unitSize;
-			epox_endian_quirk(req->rd_data, req->rd_size);
-		}
-	}
+	err = extract_hid_desc_data(rec, req);
+	if (err < 0)
+		return err;
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH BlueZ 4/4] input: Validate SDP HIDDescriptorList subattributes

[Johan Hedberg]

Hi Lizardo,

On Sun, Jan 06, 2013, Anderson Lizardo wrote:
> It should not be assumed that remote SDP attributes are in a compliant
> format. This fixes a couple of invalid pointer access on invalid data.
> ---
>  profiles/input/device.c |   60 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 14 deletions(-)

I've applied the first three patches, but this one needs a bit of fixing
up:

> +	if (d->dtd < SDP_SEQ8 || d->dtd > SDP_SEQ32)
> +		goto invalid_desc;

Please always be explicit on what values you're checking for instead of
assuming that the reader of the code knows what's contained within some
range. In this case there's already a convenient SDP_IS_SEQ() macro you
could use.

> +	if (d->dtd < SDP_SEQ8 || d->dtd > SDP_SEQ32)
> +		goto invalid_desc;

Same here.

> +	if (!d || d->dtd < SDP_TEXT_STR8 || d->dtd > SDP_TEXT_STR32)
> +		goto invalid_desc;

I suppose the best way to handle this one is to add a SDP_IS_STR() macro
(in a separate patch) to lib/sdp.h and then use it in this patch.

Johan

Re: [PATCH BlueZ 4/4] input: Validate SDP HIDDescriptorList subattributes

[Anderson Lizardo]

Hi Johan,

On Mon, Jan 7, 2013 at 4:27 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> +     if (d->dtd < SDP_SEQ8 || d->dtd > SDP_SEQ32)
>> +             goto invalid_desc;
>
> Please always be explicit on what values you're checking for instead of
> assuming that the reader of the code knows what's contained within some
> range. In this case there's already a convenient SDP_IS_SEQ() macro you
> could use.
>
>> +     if (d->dtd < SDP_SEQ8 || d->dtd > SDP_SEQ32)
>> +             goto invalid_desc;
>
> Same here.
>
>> +     if (!d || d->dtd < SDP_TEXT_STR8 || d->dtd > SDP_TEXT_STR32)
>> +             goto invalid_desc;
>
> I suppose the best way to handle this one is to add a SDP_IS_STR() macro
> (in a separate patch) to lib/sdp.h and then use it in this patch.

Just sent a couple of patches fixing these problems and similar ones
in lib/sdp.c and health plugin source.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil