scapy icon indicating copy to clipboard operation
scapy copied to clipboard

Add additional RoCE extended transport headers (such as DETH and ImmDt)

Open haggaie opened this issue 4 years ago • 6 comments

Allow parsing unreliable datagram headers. The code adds guess_payload_class functions to find the correct sub-header according to BTH.opcode, and also automatic selection of the opcode when possible.

haggaie avatar Oct 16 '21 15:10 haggaie

Codecov Report

Merging #3408 (e47caf2) into master (1de61c7) will increase coverage by 0.00%. The diff coverage is 97.29%.

:exclamation: Current head e47caf2 differs from pull request most recent head 215e36a. Consider uploading reports for the commit 215e36a to get more accurate results

@@           Coverage Diff           @@
##           master    #3408   +/-   ##
=======================================
  Coverage   85.95%   85.96%           
=======================================
  Files         275      275           
  Lines       56834    56870   +36     
=======================================
+ Hits        48854    48886   +32     
- Misses       7980     7984    +4     
Impacted Files Coverage Δ
scapy/contrib/roce.py 95.09% <97.29%> (+1.15%) :arrow_up:
scapy/arch/windows/__init__.py 67.73% <0.00%> (-0.57%) :arrow_down:
scapy/layers/inet.py 65.92% <0.00%> (-0.08%) :arrow_down:
scapy/contrib/automotive/gm/gmlan_scanner.py 86.42% <0.00%> (+0.27%) :arrow_up:

codecov[bot] avatar Oct 16 '21 15:10 codecov[bot]

Very sorry about the delay regarding this PR !

gpotter2 avatar Nov 08 '21 23:11 gpotter2

Very sorry about the delay regarding this PR !

Don't worry about it 😄

haggaie avatar Nov 14 '21 07:11 haggaie

is this PR going to be merged?

albydnc avatar Nov 23 '23 15:11 albydnc

Not as-is, no. A lot feedback has not been addressed.

gpotter2 avatar Nov 24 '23 16:11 gpotter2

Hi @gpotter2, I would be happy to fix this pull request (and continue with other parts of the RoCE spec), but I wanted to consult with you on how to best represent the RoCE protocol stack within scapy. I believe the main issue with the pull request was how the code examines upper layers in order to determine the next layer. With RoCE, a single opcode field within the BTH header can decide several subsequent layers. For example, an opcode of UD_SEND_ONLY_WITH_IMMEDIATE should tell the parser that the next header after BTH is the DETH header, and the one after that is the ImmDt. I'm not sure what's the best way to represent this in scapy. I can see three alternatives:

  1. Using logic in guess_payload and post_build to describe the layers that are expected (as this patch tried to do).
  2. Using conditional fields and storing the full set of alternative headers as one big layer. E.g., we could have all the DETH fields and ImmDt fields moved into BTH under conditions that test the opcode for UD_SEND_ONLY or UD_SEND_ONLY_WITH_IMMEDIATE.
  3. Creating a separate layer for every combination under BTH and use bind_layers to link each opcode with the appropriate layer. For example, we could have a DETHImmDt layer for the combination of DETH and ImmDt and bind it to BTH when the opcode is UD_SEND_ONLY_WITH_IMMEDIATE.

I can see there are pros and cons to each method, so I wonder if the scapy project has a preferred approach, or if you have any other alternative I haven't considered.

Thanks, Haggai

haggaie avatar Feb 04 '24 07:02 haggaie