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

[Johan Hedberg <johan.hedberg@gmail.com>]

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