[PATCH BlueZ] monitor: Improbe decoding of ATT Read by Group Type Respose

[PATCH BlueZ] monitor: Improbe decoding of ATT Read by Group Type Respose

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The group data is as follow:

Attribute Handle (2 octets) End Group Handle (2 octets) Attribute Value
(Length - 4) octets

But since only primary and secondary can be used in ATT Read by Group
Type Request the value is garanteed to be a UUID:

< ACL Data TX: Handle 3585 flags 0x00 dlen 24
      ATT: Read By Group Type Response (0x11) len 19
        Attribute data length: 6
        Attribute group list: 3 entries
        Handle range: 0x0001-0x0005
        UUID: Generic Access Profile (0x1800)
        Handle range: 0x0006-0x000d
        UUID: Heart Rate (0x180d)
        Handle range: 0x000e-0x0011
        UUID: Battery Service (0x180f)
---
 monitor/l2cap.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/monitor/l2cap.c b/monitor/l2cap.c
index 55efa54..409f5f4 100644
--- a/monitor/l2cap.c
+++ b/monitor/l2cap.c
@@ -2217,12 +2217,35 @@ static void att_read_group_type_req(const struct l2cap_frame *frame)
 	print_uuid("Attribute group type", frame->data + 4, frame->size - 4);
 }
 
+static void print_group_list(const char *label, uint8_t length,
+					const void *data, uint16_t size)
+{
+	uint8_t count;
+
+	if (length == 0)
+		return;
+
+	count = size / length;
+
+	print_field("%s: %u entr%s", label, count, count == 1 ? "y" : "ies");
+
+	while (size >= length) {
+		print_handle_range("Handle range", data);
+		print_uuid("UUID", data + 4, length - 4);
+
+		data += length;
+		size -= length;
+	}
+
+	packet_hexdump(data, size);
+}
+
 static void att_read_group_type_rsp(const struct l2cap_frame *frame)
 {
 	const struct bt_l2cap_att_read_group_type_rsp *pdu = frame->data;
 
 	print_field("Attribute data length: %d", pdu->length);
-	print_data_list("Attribute data list", pdu->length,
+	print_group_list("Attribute group list", pdu->length,
 					frame->data + 1, frame->size - 1);
 }
 
-- 
2.1.0

Re: [PATCH BlueZ] monitor: Improbe decoding of ATT Read by Group Type Respose

[Johan Hedberg]

Hi Luiz,

On Sun, May 24, 2015, Luiz Augusto von Dentz wrote:
> +static void print_group_list(const char *label, uint8_t length,
> +					const void *data, uint16_t size)
> +{
> +	uint8_t count;
> +
> +	if (length == 0)
> +		return;
> +
> +	count = size / length;
> +
> +	print_field("%s: %u entr%s", label, count, count == 1 ? "y" : "ies");
> +
> +	while (size >= length) {
> +		print_handle_range("Handle range", data);
> +		print_uuid("UUID", data + 4, length - 4);
> +
> +		data += length;
> +		size -= length;
> +	}
> +
> +	packet_hexdump(data, size);
> +}

Looks good, but to avoid btmon crashes (e.g. when analyzing codenomicon
logs) might be good to add check for 'length >= 6' and jump to
packet_hexdump if you get something smaller than that.

Johan

Re: [PATCH BlueZ] monitor: Improbe decoding of ATT Read by Group Type Respose

[Luiz Augusto von Dentz]

Hi Johan,

On Sun, May 24, 2015 at 4:58 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Luiz,
>
> On Sun, May 24, 2015, Luiz Augusto von Dentz wrote:
>> +static void print_group_list(const char *label, uint8_t length,
>> +                                     const void *data, uint16_t size)
>> +{
>> +     uint8_t count;
>> +
>> +     if (length == 0)
>> +             return;
>> +
>> +     count = size / length;
>> +
>> +     print_field("%s: %u entr%s", label, count, count == 1 ? "y" : "ies");
>> +
>> +     while (size >= length) {
>> +             print_handle_range("Handle range", data);
>> +             print_uuid("UUID", data + 4, length - 4);
>> +
>> +             data += length;
>> +             size -= length;
>> +     }
>> +
>> +     packet_hexdump(data, size);
>> +}
>
> Looks good, but to avoid btmon crashes (e.g. when analyzing codenomicon
> logs) might be good to add check for 'length >= 6' and jump to
> packet_hexdump if you get something smaller than that.

print_uuid already does that and we have a check for minimal size of 4
for att_read_group_type_rsp, what I think we really want to check
field by field like is done in avctp.c for example, so we get exactly
where the pdu is breaking, but to get to that we will have to fix much
more than print_group_list


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] monitor: Improbe decoding of ATT Read by Group Type Respose

[Johan Hedberg]

Hi Luiz,

On Sun, May 24, 2015, Luiz Augusto von Dentz wrote:
> On Sun, May 24, 2015 at 4:58 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi Luiz,
> >
> > On Sun, May 24, 2015, Luiz Augusto von Dentz wrote:
> >> +static void print_group_list(const char *label, uint8_t length,
> >> +                                     const void *data, uint16_t size)
> >> +{
> >> +     uint8_t count;
> >> +
> >> +     if (length == 0)
> >> +             return;
> >> +
> >> +     count = size / length;
> >> +
> >> +     print_field("%s: %u entr%s", label, count, count == 1 ? "y" : "ies");
> >> +
> >> +     while (size >= length) {
> >> +             print_handle_range("Handle range", data);
> >> +             print_uuid("UUID", data + 4, length - 4);
> >> +
> >> +             data += length;
> >> +             size -= length;
> >> +     }
> >> +
> >> +     packet_hexdump(data, size);
> >> +}
> >
> > Looks good, but to avoid btmon crashes (e.g. when analyzing codenomicon
> > logs) might be good to add check for 'length >= 6' and jump to
> > packet_hexdump if you get something smaller than that.
> 
> print_uuid already does that and we have a check for minimal size of 4
> for att_read_group_type_rsp,

I was aware of the checks in print_uuid() but not of the minimum size 4
check that has already happened before entering the print_group_list
function. That should indeed be enough protection for buffer overflows.

> what I think we really want to check field by field like is done in
> avctp.c for example, so we get exactly where the pdu is breaking, but
> to get to that we will have to fix much more than print_group_list

Sounds reasonable to me.

Johan

Re: [PATCH BlueZ] monitor: Improbe decoding of ATT Read by Group Type Respose

[Johan Hedberg]

Hi Luiz,

On Sun, May 24, 2015, Luiz Augusto von Dentz wrote:
> The group data is as follow:
> 
> Attribute Handle (2 octets) End Group Handle (2 octets) Attribute Value
> (Length - 4) octets
> 
> But since only primary and secondary can be used in ATT Read by Group
> Type Request the value is garanteed to be a UUID:
> 
> < ACL Data TX: Handle 3585 flags 0x00 dlen 24
>       ATT: Read By Group Type Response (0x11) len 19
>         Attribute data length: 6
>         Attribute group list: 3 entries
>         Handle range: 0x0001-0x0005
>         UUID: Generic Access Profile (0x1800)
>         Handle range: 0x0006-0x000d
>         UUID: Heart Rate (0x180d)
>         Handle range: 0x000e-0x0011
>         UUID: Battery Service (0x180f)
> ---
>  monitor/l2cap.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)

Applied now (with s/Improbe/Improve/ to the subject). Thanks.

Johan