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.

Sign up for GitHub

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

Jump to bottom

nrf802154: Validate frame length before dst filter #11191

Closed
wants to merge 4 commits into from
Closed

nrf802154: Validate frame length before dst filter #11191

wants to merge 4 commits into from

Conversation

bergzand
Copy link
Member

Contribution description

This PR adds a length check to the received frame. This prevents the destination filter check in the driver from reading beyond the received frame length. This increases the time spent in the receive part of the interrupt by 0.875µs (from 6.75µs to 7.625µs).

Also requesting a review from @miri64 and @haukepetersen as they probably also have an opinion on the additional check here.

Testing procedure

Basic receive validation should be enough.
Before this PR it should have been possible to trigger a netif receive operation on an incorrect frame after a valid frame was received.

Issues/PRs references

None

@bergzand bergzand added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 14, 2019
@bergzand bergzand added this to the Release 2019.04 milestone Mar 14, 2019
miri64
miri64 reviewed Mar 14, 2019
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

My opinion is that any incoming data should be checked for its length ;-). However, I'm not sure that your solution guarantees that.

cpu/nrf52/radio/nrf802154/nrf802154.c Outdated Show resolved Hide resolved
SemjonWilke
SemjonWilke suggested changes Apr 5, 2019
View reviewed changes
Copy link
Member

@SemjonWilke SemjonWilke left a comment

Choose a reason for hiding this comment

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

Just some suggestions.
But I have a more basic question: Could this filtering go into /drivers/netdev_ieee802154?

cpu/nrf52/radio/nrf802154/nrf802154.c Outdated Show resolved Hide resolved
cpu/nrf52/radio/nrf802154/nrf802154.c Outdated
return false;
}
size_t expected_len = ieee802154_get_frame_hdr_len(&rxbuf[1]);
if (!expected_len) {
Copy link
Member

Choose a reason for hiding this comment

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

Could go into the last return.
Also, I'd prefer comparison with zero since its not a bool, but its fine to me if you like writing it this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I agree with your suggestion to compare with zero to increase readability. I'll also try to make the set of checks a bit more readable with some comments :)

@SemjonWilke SemjonWilke 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: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Apr 5, 2019
@bergzand
Copy link
Member Author

bergzand commented Apr 5, 2019

But I have a more basic question: Could this filtering go into /drivers/netdev_ieee802154?

I think so, I have to check if I can simply extend the functions or have to change the API slightly.

@SemjonWilke
Copy link
Member

I discussed this with Martine before, and she mentioned, that rxbuf[0] or anything similar might not be available for other drivers. I think this could be investigated and the decision should be based on that.

Anyway, just tested sending, obviously still works. I did not try to break it, because i had no idea how to easily send an invalid frame.

@SemjonWilke SemjonWilke added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Apr 5, 2019
@SemjonWilke
Copy link
Member

@bergzand small reminder

@SemjonWilke
Copy link
Member

Hey, @bergzand do you still work on this PR?

@bergzand
Copy link
Member Author

bergzand commented May 6, 2019

Hey @SemjonKerner, sorry for the delays. I don't really have time to work on this at the moment but I haven't forgotten about this PR.

@bergzand
Copy link
Member Author

@SemjonKerner Thanks for the patience here. I've refactored this PR a bit to a generic data frame validation function called by the nrf802514 radio code. Let me know what you think.

@SemjonWilke
Copy link
Member

Oh, can you pls rebase. Throws some gcc changes that are fixed in master.

@bergzand bergzand force-pushed the pr/nrf802154/recv_len_check branch from e216a1f to 8e8b011 Compare May 29, 2019 12:31
@bergzand
Copy link
Member Author

Rebased!

@SemjonWilke SemjonWilke added Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines and removed Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels May 31, 2019
@SemjonWilke
Copy link
Member

LGTM, but i can't test it today. I'd like to run it once, before merging. I guess it's alright for you to wait a few more days.?

@SemjonWilke
Copy link
Member

SemjonWilke commented Jun 3, 2019 •
edited

So, uhm.. I was just checking out this PR and started a udp server on my nrf52840dk. But I cannot receive packets from samr21-xpro to nrf52840dk. Sending from nrf52840dk to samr21-xpro works. So reception is somehow broken now.

EDIT: On master everything is fine.

SemjonWilke
SemjonWilke suggested changes Jun 3, 2019
View reviewed changes
Copy link
Member

@SemjonWilke SemjonWilke left a comment

Choose a reason for hiding this comment

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

Please make sure the return values of the filter function are correct.

drivers/netdev_ieee802154/netdev_ieee802154.c
(frame_len > expected_len) &&
((*mhr & IEEE802154_FCF_TYPE_MASK) == IEEE802154_FCF_TYPE_DATA) &&
(netdev_ieee802154_dst_filter(dev, mhr) == 0)) {
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Okay, I think you mixed up the return values.

drivers/netdev_ieee802154/netdev_ieee802154.c
const uint8_t *mhr, size_t frame_len)
{
if (frame_len < IEEE802154_FCF_LEN) {
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be zero?
ae120c5#diff-864e98442504008c9fad4791703b0e35R144

@fjmolinas
Copy link
Contributor

ping @bergzand! :)

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss
Copy link
Contributor

Ping, also #16576 is touching this.

@jia200x jia200x mentioned this pull request Jun 24, 2021
Merged
@SemjonWilke
Copy link
Member

This PR can likely be closed with the same argumentation as in #11150.

@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@SemjonWilke
Copy link
Member

@benpicco this one can be closed as well.
#11150 (comment)

@benpicco benpicco closed this Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@SemjonWilke SemjonWilke SemjonWilke requested changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers 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: 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bergzand @SemjonWilke @fjmolinas @MrKevinWeiss @miri64 @benpicco