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

drivers/slipdev: fix off-by-one error in _recv() #18229

Closed
wants to merge 3 commits into from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jun 18, 2022

Contribution description

If the number of written bytes is greater than the length of the buffer, we have already written out-of bounds memory.

With pktbuf this means we will likely have corrupted the next free list entry.

Testing procedure

Issues/PRs references

alternative to #18066

If the number of written bytes is greater than the length of the
buffer, we have already written out-of bounds memory.

With pktbuf this means we will likely have corrupted the next free
list entry.
@benpicco benpicco requested a review from miri64 as a code owner June 18, 2022 08:24
@github-actions github-actions bot added the Area: drivers Area: Device drivers label Jun 18, 2022
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jun 18, 2022
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I think it would be better to move that if from line 202 to line 193 and replace the return -ENOBUFS;
by

res= -ENOBUFS;
break;

without the move a write may happen to a "valid" pointer to a buffer of 0 length

@miri64
Copy link
Member

miri64 commented Jun 21, 2022

Agreeing with @kfessel.

@benpicco benpicco force-pushed the drivers/slipdev-off-by-one branch from 06504fc to f7bccf0 Compare June 22, 2022 22:28
@benpicco
Copy link
Contributor Author

benpicco commented Jun 22, 2022

Like this?

btw what's up with that _recv function? Why does it have a special case to drop len bytes if buf is NULL? This is the only driver I'm aware of that does that, and I can't think of a use case for when this would be useful.

@benpicco
Copy link
Contributor Author

Hm a

sudo ping -A ff02::1%sl0

still kills it

Comment on lines 195 to 202
if ((unsigned)res == len) {
/* clear out unreceived packet */
while (byte != SLIPDEV_END) {
byte = tsrb_get_one(&dev->inbuf);
}
return -ENOBUFS;
}

Copy link
Contributor

@kfessel kfessel Jun 23, 2022

Choose a reason for hiding this comment

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

Suggested change
if ((unsigned)res == len) {
/* clear out unreceived packet */
while (byte != SLIPDEV_END) {
byte = tsrb_get_one(&dev->inbuf);
}
return -ENOBUFS;
}
if ( (unsigned) res >= len) {
/* the result grew larger than the provided buffer
clear out rest of the current packet, this package is lost */
do {
byte = tsrb_get_one(&dev->inbuf);
} while (byte != SLIPDEV_END);
res = -ENOBUFS;
break;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

do{ } while to not depend on the initialization of byte
res = .. ; break; to avoid multiple returns
(unsigned) res >= len also catches negative res (for any reason) as bigger than len
and some comment cleanup (the old one sound like the package is unreceived (and might be still receivable)

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this helps with the ping issue but if not i don't think the original one helped either

@benpicco
Copy link
Contributor Author

benpicco commented Jun 23, 2022

Ah the adaptive ping issue is unrelated. I was testing this on a nrf52840dk with examples/gnrc_border_router, so gnrc_netif_pktq is used

> ping ff02::1
error: packet buffer full
error: packet buffer full
error: packet buffer full

-> #17924

@benpicco
Copy link
Contributor Author

benpicco commented Nov 1, 2022

closed in favor of #18826

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers 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.

3 participants