automotive, j1939: scanning for CAs - #5164
BenGardiner wants to merge 6 commits into
Conversation
|
thanks @polybassa for the review. I can do almost all of that right now. There's a couple things that are either answering your questions or require me to ask you questions first. |
fbcb5a9 to
4757249
Compare
|
I noticed that in the rebase of the scanner code to your replacement soft socket the scanners were no longer cleanly relying on sr() / sr1() via answers() logic. I'll work on bringing that back, fixing the things above I didn't have questions about and then refactoring the scanners to use the answers() logic... |
97acab8 to
f664c8a
Compare
|
Hi @polybassa while I think this is ready for your next review, it might not be merged in this form. There are 'Feature' commits and then 'fixes' on them. e.g. FFfffFffFfffffFfff. To merge you would probably want the fixes squashed into the features. You may even want one squashed commit -- in which case you might consider merging the first commit separately since it is implementing missing sr1() functionality in the current J1939SoftSocket on master. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5164 +/- ##
==========================================
+ Coverage 80.37% 80.55% +0.17%
==========================================
Files 375 380 +5
Lines 97683 98788 +1105
==========================================
+ Hits 78516 79580 +1064
- Misses 19167 19208 +41
🚀 New features to boost your workflow:
|
3b5967c to
4d7cb29
Compare
|
I'm going to look closer at https://github.com/secdev/scapy/actions/runs/35222531093/job/105206012569?pr=5164 -- that seems like there could be something wrong with the soft socket... |
yep that was a race in close() of the soft socket -- just like we had in isotp soft socket. I have a fix |
35d5188 to
625c334
Compare
|
sorry the checks may be skipped now due to a rate limit... I think I resolved them but pretty hard to tell locally without waiting for the github runners |
625c334 to
58adda4
Compare
|
no sorry. skipped due to a typo. |
58adda4 to
811dfd9
Compare
|
ok... ok well that was a journey... over to you then @polybassa |
811dfd9 to
2aee690
Compare
|
Two of the failed CI tests are related. Could you please have a look |
2aee690 to
e2061c3
Compare
| log_j1939, | ||
| ) | ||
| from scapy.contrib.automotive.j1939.j1939_scanner import ( # noqa: F401 | ||
| _j1939_can_id, |
There was a problem hiding this comment.
Since all these definitions are used in multiple files, I recommend to use the leading "_"
e229234 to
888200f
Compare
Add guidelines regarding protocol prefixes, shared module symbols, dataclass usage, import ordering, and bindings. AI-Assisted: yes (Gemini 3.8 Flash)
AI-Assisted: Yes Kimi 2.7 / GPT 5.4 codex / Copilot
…allback (heuristic) Implement answers(), clone_with(), and copy() on J1939 to support sr1() for directed and broadcast requests with session tracking and fallback heuristics. Add identifier property and setter to J1939_CAN to satisfy the CAN interface for python-can backends, and add a defensive fallback in _can_send() converting to CAN on AttributeError. AI-Assisted: yes (Gemini 3.8 Flash)
Adds a scanner to identify Controller Applications in a J1939 network, various scanning techniques are provided including both broadcast and unicast. AI-Assisted: yes (Gemini 3.8 Flash)
AI-Assisted: yes (Gemini 3.8 Flash)
…939-81) AI-Assisted: yes (Gemini 3.8 Flash)
888200f to
4a87d80
Compare
yes they sadly were. should be fixed now |
|
|
||
| try: | ||
| from scapy.contrib.cansocket import CANSocket | ||
| except ImportError: |
| stop_event=None, # type: Optional[Event] | ||
| bitrate=J1939_DEFAULT_BITRATE, # type: int | ||
| busload=J1939_DEFAULT_BUSLOAD, # type: float | ||
| reconnect=None, # type: Optional[Callable[[], SuperSocket]] |
There was a problem hiding this comment.
Maybe rename to reconnect_handler
| bitrate=J1939_DEFAULT_BITRATE, # type: int | ||
| busload=J1939_DEFAULT_BUSLOAD, # type: float | ||
| reset_handler=None, # type: Optional[Callable[[], None]] | ||
| reconnect_handler=None, # type: Optional[Callable[[], SuperSocket]] |
There was a problem hiding this comment.
What's the difference between reconnect and reconnect handler?
polybassa
left a comment
polybassa
left a comment
There was a problem hiding this comment.
Focus on simplicity and Scapy-likeness: prefer packet fields/answers()/sr1() over parallel decoders, opaque request bytes, and scanner-side reimplementation of correlation.
Inline notes cover NAME, Request/post_build, answers(), the DM scanner, and scanner result/sr1 handling.
This review was written with the help of AI (ChatGPT).
| # Scapy Packet Class for J1939 64-bit NAME | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| class J1939_NAME(Packet): |
There was a problem hiding this comment.
Could we make J1939_NAME the single representation of a NAME and remove J1939NameDecoder / J1939NameResult?
At the moment the bit layout is implemented twice: once manually using shifts in J1939NameDecoder.decode() and once again through J1939_NAME.fields_desc. J1939_NAME.decode() then serializes itself back to bytes just to run the other decoder.
For example, I would expect this to look more like:
class J1939_NAME(Packet):
fields_desc = [
BitField("arbitrary_address_capable", 0, 1, tot_size=-8),
BitField("industry_group", 0, 3),
BitField("vehicle_system_instance", 0, 4),
BitField("vehicle_system", 0, 7),
BitField("reserved", 0, 1),
BitField("function", 0, 8),
BitField("function_instance", 0, 5),
BitField("ecu_instance", 0, 3),
BitField("manufacturer_code", 0, 11),
BitField("identity_number", 0, 21, end_tot_size=-8),
]
@property
def manufacturer_name(self):
return J1939_MANUFACTURERS.get(
self.manufacturer_code,
"Unknown Manufacturer (%d)" % self.manufacturer_code,
)
@property
def function_description(self):
if self.function < 128:
return J1939_PRE_ASSIGNED_FUNCTIONS.get(self.function, "Reserved")
return J1939_INDUSTRY_SPECIFIC_FUNCTIONS.get(
self.industry_group, {}
).get(self.function, "Industry-specific")Then decoding naturally becomes:
name = J1939_NAME(raw_bytes)
name.manufacturer_code
name.function
name.show()
name.summary()This would let us delete a considerable amount of parallel parsing/reporting code and feels much closer to how Scapy normally represents protocol structures.
| def extract_padding(self, s: bytes) -> Tuple[bytes, bytes]: | ||
| return b"", s | ||
|
|
||
| def decode(self) -> J1939NameResult: |
There was a problem hiding this comment.
This method is a good example of why I think the separate decoder should disappear:
def decode(self):
return J1939NameDecoder.decode(bytes(self))The packet has already decoded these bits into fields, so converting it back into bytes and decoding the same fields again seems backwards.
For example, callers currently doing:
info = name.decode()
print(info.manufacturer_code)should simply be able to do:
print(name.manufacturer_code)and descriptive information can be exposed as properties:
print(name.manufacturer_name)
print(name.function_description)That would make J1939_NAME the source of truth instead of maintaining both a Packet representation and a dataclass representation.
| ) | ||
|
|
||
|
|
||
| class J1939Request(J1939): |
There was a problem hiding this comment.
Could we represent the requested PGN as an actual Scapy field instead of putting it into the generic data field manually?
There is already an XLE3BytesField, which matches the three-byte little-endian PGN representation. A much simpler implementation could therefore be approximately:
class J1939Request(J1939):
name = "J1939Request"
fields_desc = [
XLE3BytesField("req_pgn", 0),
]
def __init__(self, *args, **kwargs):
kwargs.setdefault("pgn", J1939_PGN_REQUEST)
super(J1939Request, self).__init__(*args, **kwargs)Then:
J1939Request(req_pgn=0xFECA, dst=0x10)naturally produces the correct three bytes and dissection gives us:
pkt.req_pgn == 0xFECAThis should allow us to remove the special struct.pack("<I", req_pgn)[:3] constructor handling and most importantly simplify answers() from several fallback representations to something like:
if isinstance(other, J1939Request):
return self.pgn == other.req_pgnIt should also make the Request-specific post_build() handling unnecessary.
| ps = self.pgn & 0xFF | ||
| return j1939_to_can_id(self.priority, 0, dp, pf, ps, self.src) | ||
|
|
||
| def post_build(self, p, pay): |
There was a problem hiding this comment.
Related to the Request comment above: I don't think the generic J1939 packet should need to know that PGN 0xEA00 means "inspect my payload for an attribute named PGN or pgn, then manually encode that attribute as three bytes".
Currently we have roughly:
if self.pgn == 0xEA00 and not self.data and self.payload:
target_pgn = getattr(
self.payload, "PGN", getattr(self.payload, "pgn", None)
)
...If Request is represented as:
J1939Request(req_pgn=J1939_PGN_DM1)then Scapy can build the three bytes through XLE3BytesField and this whole post_build() special case can disappear.
I would prefer that over supporting three interchangeable request representations:
J1939Request(req_pgn=...)
J1939Request(data=b"...")
J1939Request() / SomePacketWithAPGNAttribute()One explicit wire representation seems both simpler and more Scapy-like.
| return p + struct.pack("<I", target_pgn)[:3] | ||
| return p + pay | ||
|
|
||
| def answers(self, other): |
There was a problem hiding this comment.
Could we keep J1939.answers() limited to generic J1939 semantics?
Right now this method knows about several unrelated application interactions:
- DM14 -> DM15
- Diagnostic B -> Diagnostic A/B
- Diagnostic A -> Diagnostic A
- TP.CM RTS -> CTS/ABORT
- ECU-ID-specific BAM
- Command ACKs
- generic same-PGN fallback
For example, once DM15 is represented as a packet, the DM-specific relationship could live beside the DM packets instead:
class J1939_DM15(Packet):
PGN = J1939_PGN_DM15
def answers(self, other):
return isinstance(other, J1939_DM14)Likewise TP control-message correlation belongs naturally to the TP packet representation.
I would expect the generic layer to become closer to:
def answers(self, other):
if not isinstance(other, J1939):
return 0
if directed_request_does_not_match_addresses(self, other):
return 0
if isinstance(other, J1939Request):
return self.pgn == other.req_pgn
return 0The exact split can differ, but I would avoid turning J1939.answers() into a registry of every request/response pair that a future scanner might need.
| # --- Technique: unicast DM PGN probe | ||
|
|
||
|
|
||
| def j1939_scan_dm_pgn( |
There was a problem hiding this comment.
Could this scanner use J1939Request + sr1() instead of dropping back to raw CAN?
The PR already adds answers() support specifically so J1939 requests work through Scapy's send/receive machinery, but this function currently reconstructs the same behavior manually:
can_id = j1939_can_id(...)
payload = struct.pack(...)
...
def _rx(pkt):
...
j1939_decode_can_id(...)
...
parse TP.CM
...
parse ACK/NACKI would expect the core of this function to be closer to:
req = J1939Request(
req_pgn=pgn,
src=src_addr,
dst=target_da,
)
with j1939_get_sock(
sock,
reconnect=reconnect,
target_sa=target_da,
) as j_sock:
rcv = j_sock.sr1(
req,
timeout=sniff_time,
verbose=False,
)
if rcv is None:
return DmScanResult(dm_name, pgn, False, error="Timeout")
if rcv.pgn == J1939_PGN_ACK:
# Interpret ACK/NACK here.
...
return DmScanResult(dm_name, pgn, True, packet=rcv)There may still need to be a very small special case after reception for aborting an RTS transfer, but CAN-ID decoding, sniff callback management, PGN matching, source matching, and TP response correlation should ideally come from the J1939 layer/socket.
Otherwise we end up maintaining request/response logic both in answers() and again in every scanner.
| req = J1939Request(req_pgn=J1939_PGN_ADDRESS_CLAIMED, dst=_da, src=_sa) | ||
| j1939_log.debug("unicast: probing DA=0x%02X from SA=0x%02X", _da, _sa) | ||
| rcv = j_sock.sr1(req, timeout=sniff_time, verbose=False) | ||
| if ( |
There was a problem hiding this comment.
Here sr1() has already selected a packet because rcv.answers(req) returned true, but immediately afterwards we repeat most of that matching:
rcv = j_sock.sr1(req, ...)
if (
rcv is not None
and rcv.src == _da
and rcv.pgn == J1939_PGN_ADDRESS_CLAIMED
and rcv.src != _sa
and (rcv.dst == _sa or rcv.dst == J1939_GLOBAL_ADDRESS)
):Could we decide which layer owns these rules?
If these conditions define whether the packet answers the request, I think they should be in answers() and this should simply become:
rcv = j_sock.sr1(req, timeout=sniff_time, verbose=False)
if rcv is not None:
found.setdefault(_da, []).append(rcv)Application-specific validation after sr1() is fine, e.g. checking that an XCP response has a positive-response byte. But SA/DA/PGN correlation should preferably not be implemented once in answers() and then repeated in the caller.
| if sa not in found: | ||
| found[sa] = [] | ||
| # Record which scanner SA elicited this broadcast | ||
| setattr(rcv, "src_addrs", [_sa]) |
There was a problem hiding this comment.
Could we avoid adding scanner-specific attributes to received protocol packets?
For example:
setattr(rcv, "src_addrs", [_sa])means the same J1939 packet type now has an undocumented attribute depending on which scanner produced it.
A very small result object would make this explicit without changing the packet:
@dataclass
class J1939ScanResult:
packet: J1939
scanner_src: Optional[int]Then a technique can return:
found[sa].append(
J1939ScanResult(
packet=rcv,
scanner_src=_sa,
)
)This also avoids having the top-level scanner maintain parallel structures such as:
methods[i]
packets[i]
src_addrs[i]where the meaning depends on the indexes staying synchronized.
I would keep this to one tiny result type rather than creating a larger result hierarchy.
Description
This adds j1939 scanning for Controller Applications on-top-of the soft socket support.
The changes aim to introduce only those J1939 value enumeration definitions which can be sourced from freely available locations on the internet. As such, there is not a complete list of the values.
I don't intend any impacts on other parts of the libraries.
fixes missing sr1() functionality in J1939 soft socket on master
LLM coding tools were used in the development of this PR: copilot and gemini, various models.