PR: 267334 Reported by: Robert Morris <rtm@lcs.mit.edu> Sponsored by: The FreeBSD Foundation
Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Looking at 267334. IMHO, we should return the error for invalid message as close as possible to the userland. Don't let the invalid data travel this down.
Of course we should also check for num nonnegative and too large; there's probably more to be covered here too.
Can we return unsigned value from ng_get_composite_len() instead?
We should, but I think there are ABI considerations to take into account and we really need someone who understands netgraph well.
I'm going to commit this as an interim improvement. We can revisit ng_get_composite_len's return type and error handling later on.
I agree in principle with “as close as possible to userland”, but I don't see how you could move this check any closer to userland without either duplicating a non-trivial amount of code in several places or completely refactoring ng_parse.c.
More importantly though, I'm concerned by the lack of an upper bound on num. I wouldn't be surprised if it's possible to pass in a value that causes the multiplication in the malloc() call to overflow to a small positive number, allowing the allocation to succeed but leading to a buffer overflow further down the line.
I also dislike hiding the call to ng_get_composite_len() in the initialization of num. I would prefer to see it immediately before the range check.