Hi Marcel,
On Tuesday 19 May 2015 21:17:40 Marcel Holtmann wrote:
> Hi Szymon,
>
> > Some remote devices (ie Gigaset G-Tag) misbehave with ADV data length.
> > This can lead to incorrect EIR format in device found event when
> > ADV_DATA and SCAN_RSP are merged (terminator field before SCAN_RSP
> > part).
> >
> > Fix this by inspecting ADV_DATA and correct its length if terminator
> > is found.
> >
> >> HCI Event: LE Meta Event (0x3e) plen 42 [hci0] 32.172182
> >>
> > LE Advertising Report (0x02)
> >
> > Num reports: 1
> > Event type: Connectable undirected - ADV_IND (0x00)
> > Address type: Public (0x00)
> > Address: 7C:2F:80:94:97:5A (Gigaset Communications GmbH)
> > Data length: 30
> > Flags: 0x06
> >
> > LE General Discoverable Mode
> > BR/EDR Not Supported
> >
> > Company: Gigaset Communications GmbH (384)
> >
> > Data: 021512348094975abbc5
> >
> > 16-bit Service UUIDs (partial): 1 entry
> >
> > Battery Service (0x180f)
> >
> > RSSI: -65 dBm (0xbf)
> >>
> >> HCI Event: LE Meta Event (0x3e) plen 27 [hci0] 32.172191
> >>
> > LE Advertising Report (0x02)
> >
> > Num reports: 1
> > Event type: Scan response - SCAN_RSP (0x04)
> > Address type: Public (0x00)
> > Address: 7C:2F:80:94:97:5A (Gigaset Communications GmbH)
> > Data length: 15
> > Name (complete): Gigaset G-tag
> > RSSI: -59 dBm (0xc5)
> >
> > Note "Data length: 30" in ADV_DATA which results in 9 extra zero bytes
> > after Battery Service UUID. Terminator field present in the middle of
> > EIR in Device Found event resulted in userspace stop parsing EIR and
> > skipping device name.
> >
> > @ Device Found: 7C:2F:80:94:97:5A (1) rssi -59 flags 0x0000
> >
> > 02 01 06 0d ff 80 01 02 15 12 34 80 94 97 5a bb ..........4...Z.
> > c5 03 02 0f 18 00 00 00 00 00 00 00 00 00 0e 09 ................
> > 47 69 67 61 73 65 74 20 47 2d 74 61 67 Gigaset G-tag
> >
> > With this fix EIR with merged ADV_DATA and SCAN_RSP in device found
> > event is properly formatted:
> >
> > @ Device Found: 7C:2F:80:94:97:5A (1) rssi -59 flags 0x0000
> >
> > 02 01 06 0d ff 80 01 02 15 12 34 80 94 97 5a bb ..........4...Z.
> > c5 03 02 0f 18 0e 09 47 69 67 61 73 65 74 20 47 .......Gigaset G
> > 2d 74 61 67 -tag
> >
> > Signed-off-by: Szymon Janc <szymon.janc@xxxxxxxxx>
> > ---
> > v2: fixed build error due to use of non-upstream BT_ERR_RATELIMITED
> >
> > net/bluetooth/hci_event.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 7b61be7..0407324 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4711,6 +4711,23 @@ static void process_adv_report(struct hci_dev
> > *hdev, u8 type, bdaddr_t *bdaddr,>
> > struct hci_conn *conn;
> > bool match;
> > u32 flags;
> >
> > + u8 *ptr, real_len;
> > +
> > + /* Find the 'real' end of the data in case remote incorrectly
> > + * set significant part length.
>
> I do not understand this sentence. I assume what you want to say find the
> end of the data in case the report contains padded zero bytes at the end
> causing an invalid length value.
Spec is says about significant and non-significant part of advertising (where
length is length of significant part) but I can rephrase that.
> > + */
> > + for (ptr = data; ptr < data + len && *ptr; ptr += *ptr + 1) {
> > + if (ptr + 1 + *ptr > data + len)
> > + break;
> > + }
>
> I am worried about the unchecked usage of *ptr here. That might let you jump
> into some memory area. How are we protecting this against malicious
> advertising data. And we should have mgnt-tester test cases for malicious
> fields that are not valid data and would make you jump outside of the 31
> bytes.
OK, I see now that data can be NULL in case of direct advertising report. I'll
add check for that and provide mgmt-tester cases for this and for bogus
advertising data as well.
> > +
> > + real_len = ptr - data;
> > +
> > + /* Adjust for actual length */
> > + if (len != real_len) {
> > + BT_ERR("Corrected invalid ADV_DATA length”);
>
> I would spell advertising report data length here. There really is no
> ADV_DATA anywhere in the specification or used in BlueZ like that.
OK. Yet, I still wonder about ratelimit here. With 10 or more G-Tags around
this can become quite spammy...
--
Szymon K. Janc
szymon.janc@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
