Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLE: Add HCI_LE_Meta_Extended_Advertising_Report event #4686

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tryger
Copy link

@tryger tryger commented Mar 10, 2025

Bluetooth Core Specification v6.0 | Vol 4, Part E, Section 7.7.65.13

Example packet show()

###[ HCI header ]###
  type      = Event
###[ HCI Event header ]###
     code      = 0x3e
     len       = 80
###[ HCI_LE_Meta ]###
        event     = extended_advertising_report
###[ Extended Advertising Reports ]###
           num_reports= 2
           \reports   \
            |###[ Extended Advertising Report ]###
            |  type      = 18
            |  atype     = random
            |  addr      = a1:b2:c3:d4:e5:f6
            |  pri_phy   = le_1m
            |  sec_phy   = 0
            |  adv_sid   = 255
            |  tx_pwr    = 127
            |  rssi      = -85
            |  interval  = 0
            |  datype    = public
            |  daddr     = 00:00:00:00:00:00
            |  len       = 16
            |  \data      \
            |   |###[ EIR Header ]###
            |   |  len       = 3
            |   |  type      = complete_list_16_bit_svc_uuids
            |   |###[ Complete list of 16-bit service UUIDs ]###
            |   |     svc_uuids = [0xffff]
            |   |###[ EIR Header ]###
            |   |  len       = 11
            |   |  type      = svc_data_16_bit_uuid
            |   |###[ EIR Service Data - 16-bit UUID ]###
            |   |     svc_uuid  = 0xffff
            |   |###[ Raw ]###
            |   |        load      = b'scapy\x00\x00\x00'
            |###[ Extended Advertising Report ]###
            |  type      = 26
            |  atype     = random
            |  addr      = a1:b2:c3:d4:e5:f6
            |  pri_phy   = le_1m
            |  sec_phy   = 0
            |  adv_sid   = 255
            |  tx_pwr    = 127
            |  rssi      = -85
            |  interval  = 0
            |  datype    = public
            |  daddr     = 00:00:00:00:00:00
            |  len       = 14
            |  \data      \
            |   |###[ EIR Header ]###
            |   |  len       = 13
            |   |  type      = mfg_specific_data
            |   |###[ EIR Manufacturer Specific Data ]###
            |   |     company_id= 0xffff
            |   |###[ Raw ]###
            |   |        load      = b'scapy\x00\x01\x02\x03\x04'

Copy link

codecov bot commented Mar 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.59%. Comparing base (874abdc) to head (699abad).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4686      +/-   ##
==========================================
- Coverage   82.13%   78.59%   -3.55%     
==========================================
  Files         361      336      -25     
  Lines       86829    81635    -5194     
==========================================
- Hits        71321    64158    -7163     
- Misses      15508    17477    +1969     
Files with missing lines Coverage Δ
scapy/layers/bluetooth.py 90.46% <100.00%> (+0.06%) ⬆️

... and 287 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@antoniovazquezblanco
Copy link
Contributor

Extract of the Bluetooth Core Spec
imagen

@antoniovazquezblanco
Copy link
Contributor

One last comment, this may be quite important:

You have implemented an array of a structure with 10+ fields but as I understand from the documentation, it seems that it should be a struct of 10+ arrays...

I am not sure this is explanation is easy to understand, please, let me know if I can clarify this comment. Thanks

@tryger
Copy link
Author

tryger commented Mar 10, 2025

You are right that in the Spec is not really clear how this is implemented. Based on empirical observations from captured communications of this event, I concluded this event is implemented as my commit does: It contains num_reports report structures with the extended advertisement format, specified in the spec.

It is implemented similarly to HCI_LE_Advertising_Report (Section 7.7.65.2), which is already implemented in Scapy.

Hope this clarifies your doubt.

@antoniovazquezblanco
Copy link
Contributor

Hope this clarifies your doubt.

Yes! It also clarifies that the HCI_LE_Advertising_Report packet may be ok, wich I had my doubts...

Thanks! :D

@tryger
Copy link
Author

tryger commented Mar 10, 2025

I renamed fields to Spec names; that makes more sense to me, despite the fact that for other events the field names are shortened.

Can somebody clarify if there is a reason for keeping some field names not matching the Specification, or maybe it is legacy?

class HCI_LE_Meta_Extended_Advertising_Report(Packet):
name = "Extended Advertising Report"
fields_desc = [
LEShortField("type", 0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field would ideally be a bitfield or a bitenumfield. I understand this may not be fun to write and could possibly be added in future improvements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

name = "Extended Advertising Report"
fields_desc = [
LEShortField("type", 0),
ByteEnumField("atype", 0, {0: "public", 1: "random"}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum is missing a couple of possible values
imagen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor

@antoniovazquezblanco antoniovazquezblanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments more... Sorry I think i wrongly approved before the last two comments where added...

@tryger
Copy link
Author

tryger commented Mar 10, 2025

Thanks for your revision. I pushed a commit including your proposed changes.

However, I am not completely sure independent BitFields for event_type is the best option; I'd rather prefer implementing this as something like a named BitField struct. I think this Field is not implemented and would be a nice feature to include.

@antoniovazquezblanco
Copy link
Contributor

AFAIK there is no way to create a composable field in the field list.

If you want to group that as a field, you may create a Packet for the type. That would allow you to compose it. It is not a bad solution from my point of view but maybe other maintainers have different ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants