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

cdc_ecm: Truncate frames at max ethernet size #17200

Merged

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented Nov 15, 2021

Contribution description

This truncates the incomming frames to ETHERNET_FRAME_LEN and silently
discards the rest of the frame until the end of the frame. This should
be modified to an endpoint halt condition after #17090 is merged, but
for now this should be good enough.

Stalling the endpoint with the current stall implementation could cause
a ping of death scenario, so for now the data is truncated until the
above solution can be implemented.

(I've also removed an duplicate USBOPT_EP_AVAILABLE call)

Testing procedure

The tests/usbus_cdc_ecm application should still work on a platform of choice.

Issues/PRs references

Reported in detail by @szymonh

This truncates the incomming frames to ETHERNET_FRAME_LEN and silently
discards the rest of the frame until the end of the frame. This should
be modified to an endpoint halt condition after RIOT-OS#17090 is merged, but
for now this should be good enough.

Stalling the endpoint with the current stall implementation could cause
a ping of death scenario, so for now the data is truncated until the
above solution can be implemented.
@bergzand bergzand added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: USB Area: Universal Serial Bus labels Nov 15, 2021
@bergzand bergzand requested a review from dylad November 15, 2021 15:08
@github-actions github-actions bot added the Area: sys Area: System label Nov 15, 2021
@dylad dylad added this to the Release 2022.01 milestone Nov 15, 2021
@dylad dylad self-assigned this Nov 15, 2021
@dylad dylad added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Nov 15, 2021
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

LGTM.
Tested it and there is no regression so far with tests/usbus_cdc_ecm

@dylad dylad added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Nov 15, 2021
@fjmolinas fjmolinas merged commit 5f119e3 into RIOT-OS:master Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants