Bluetooth: hci_event: Mask data status from LE ext adv reports

The Event_Type field in an LE Extended Advertising Report uses bits 5 and 6 for data status (e.g. fragmentation), not the PDU type itself.

The ext_evt_type_to_legacy() function fails to mask these status bits before evaluation. This causes valid advertisements with status bits set (e.g. a fragmented non-connectable advertisement, which ends up showing as PDU type 0x40) to be misclassified as unknown and subsequently dropped. This is okay for most checks which use bitwise AND on the relevant event type bits, but it doesn't work for non-connectable types, which are checked with '== LE_EXT_ADV_NON_CONN_IND' (that is, zero).

This patch introduces a PDU type mask to ensure only the relevant bits are evaluated, allowing for the correct translation of all valid extended advertising packets.

Signed-off-by Chris Down <[email protected]>

Cc [email protected]

---
 net/bluetooth/hci_event.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

net/bluetooth/hci_event.c +7 -5

index c0eb03e5cbf8..077c93b5fae0 100644
@@ -6237,10 +6237,14 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, void *data,
        hci_dev_unlock(hdev);
 }
 
+#define LE_EXT_ADV_DATA_STATUS_MASK GENMASK(6, 5)
 
 
 
 static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type)
 {
-       if (evt_type & LE_EXT_ADV_LEGACY_PDU) {
-               switch (evt_type) {
+       u16 pdu_type = evt_type & ~LE_EXT_ADV_DATA_STATUS_MASK;
+
+       if (pdu_type & LE_EXT_ADV_LEGACY_PDU) {
+               switch (pdu_type) {
                case LE_LEGACY_ADV_IND:
                        return LE_ADV_IND;
                case LE_LEGACY_ADV_DIRECT_IND:

@@ -6257,21 +6261,21 @@ static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type)
                goto invalid;
        }
 
-       if (evt_type & LE_EXT_ADV_CONN_IND) {
-               if (evt_type & LE_EXT_ADV_DIRECT_IND)
+       if (pdu_type & LE_EXT_ADV_CONN_IND) {
+               if (pdu_type & LE_EXT_ADV_DIRECT_IND)
                        return LE_ADV_DIRECT_IND;
 
                return LE_ADV_IND;
        }
 
-       if (evt_type & LE_EXT_ADV_SCAN_RSP)

net/bluetooth/hci_event.c +1 -1

+           pdu_type & LE_EXT_ADV_DIRECT_IND)
                return LE_ADV_NONCONN_IND;
 
 invalid:
--

2.49.0

Can you include a sample trace of the above?

Also it would be great to have a mgmt-tester for example that attempts to generate an advertisement like that to exercise such change.

net/bluetooth/hci_event.c static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type) · line 6277

Quoted 3 lines

net/bluetooth/hci_event.c +1 -2

-       if (evt_type == LE_EXT_ADV_NON_CONN_IND ||
-           evt_type & LE_EXT_ADV_DIRECT_IND)
+       if (pdu_type == LE_EXT_ADV_NON_CONN_IND ||

I'm not sure I would keep checking for LE_EXT_ADV_NON_CONN_IND, maybe just return LE_ADV_NONCONN_IND, LE_EXT_ADV_NON_CONN_IND is not actually a bit it is the absence of any bits being set, so I guess the only invalid adv are the ones for legacy which seem to require a bit to be set.

Hi Luiz,

Thanks for the review!

Quoted 1 line · Luiz Augusto von Dentz

Can you include a sample trace of the above?

Is that with btmon or similar? Sorry, I'm not a regular contributor to this subsystem :-)

I mostly have a personal desire to get this merged because it's a particularly noisy case where I happen to live :-) These are all with 0x40:

% dmesg | wc -l
3815
% dmesg | grep -c 'Unknown advertising'
3227

Quoted 2 lines

Also it would be great to have a mgmt-tester for example that attempts to generate an advertisement like that to exercise such change.

Looks like that's in Bluez userspace code right, so what's the order of doing these things?

Quoted 9 lines

net/bluetooth/hci_event.c static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type) · line 6277

Quoted 3 lines

net/bluetooth/hci_event.c +1 -2

-       if (evt_type == LE_EXT_ADV_NON_CONN_IND ||
-           evt_type & LE_EXT_ADV_DIRECT_IND)
+       if (pdu_type == LE_EXT_ADV_NON_CONN_IND ||

I'm not sure I would keep checking for LE_EXT_ADV_NON_CONN_IND, maybe just return LE_ADV_NONCONN_IND, LE_EXT_ADV_NON_CONN_IND is not actually a bit it is the absence of any bits being set, so I guess the only invalid adv are the ones for legacy which seem to require a bit to be set.

So are you thinking of doing this?

   if (!(pdu_type & ~(LE_EXT_ADV_DIRECT_IND)))
           return LE_ADV_NONCONN_IND;

Thanks for your help!

Chris

Try to capture one of them using btmon and then add to the patch description.

Quoted 10 lines

Quoted 9 lines

net/bluetooth/hci_event.c static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type) · line 6277

Quoted 3 lines

net/bluetooth/hci_event.c +1 -2

-       if (evt_type == LE_EXT_ADV_NON_CONN_IND ||
-           evt_type & LE_EXT_ADV_DIRECT_IND)
+       if (pdu_type == LE_EXT_ADV_NON_CONN_IND ||

I'm not sure I would keep checking for LE_EXT_ADV_NON_CONN_IND, maybe just return LE_ADV_NONCONN_IND, LE_EXT_ADV_NON_CONN_IND is not actually a bit it is the absence of any bits being set, so I guess the only invalid adv are the ones for legacy which seem to require a bit to be set.

Quoted 4 lines

So are you thinking of doing this?

   if (!(pdu_type & ~(LE_EXT_ADV_DIRECT_IND)))
           return LE_ADV_NONCONN_IND;

We can probably return early on if (!evt_type) return LE_ADV_NONCONN_IND since there is no point in evaluating it if it is zero.

Hi Luiz,

Quoted 1 line

Try to capture one of them using btmon and then add to the patch description.

Thanks, I have one now and will add for v2.

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

net/bluetooth/hci_event.c static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type) · line 6277

Quoted 3 lines

net/bluetooth/hci_event.c +1 -2

-       if (evt_type == LE_EXT_ADV_NON_CONN_IND ||
-           evt_type & LE_EXT_ADV_DIRECT_IND)
+       if (pdu_type == LE_EXT_ADV_NON_CONN_IND ||

I'm not sure I would keep checking for LE_EXT_ADV_NON_CONN_IND, maybe just return LE_ADV_NONCONN_IND, LE_EXT_ADV_NON_CONN_IND is not actually a bit it is the absence of any bits being set, so I guess the only invalid adv are the ones for legacy which seem to require a bit to be set.

So are you thinking of doing this?

   if (!(pdu_type & ~(LE_EXT_ADV_DIRECT_IND)))
           return LE_ADV_NONCONN_IND;

Quoted 3 lines

We can probably return early on if (!evt_type) return LE_ADV_NONCONN_IND since there is no point in evaluating it if it is zero.

I guess you meant if (!pdu_type)? That correctly handles the 0x40 case (where pdu_type becomes 0), but it would miss non-connectable directed advertisements (PDU type 0x04), right? Or maybe you meant something else?