rtnetlink: Align RTA_MULTIPATH length validation in nlattr_get
ClosedPublic
Actions

Authored by pouria on Tue, May 12, 11:47 AM.

Details

Reviewers

markj
glebius
melifaro

Group Reviewers

network

Commits

rG44be923a2b6b: rtnetlink: Align RTA_MULTIPATH length validation in nlattr_get
rG2c6617658f0c: rtnetlink: Align RTA_MULTIPATH length validation in nlattr_get
rG4329663a861e: rtnetlink: Align RTA_MULTIPATH length validation in nlattr_get

Summary

Fix length validation of RTA_MULTIPATH attributes in
nlattr_get_multipath() by making sure the user request is align.

PR: 295102
Reported by: Robert Morris <rtm@lcs.mit.edu>
Fixes: 7e5bf68495cc ("netlink: add netlink support")
MFC after: 3 days

Test Plan

See the attached patch in PR295102.
https://bugs.freebsd.org/bugzilla/attachment.cgi?id=270516

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 73060
Build 69943: arc lint + arc unit

Event Timeline

pouria created this revision.Tue, May 12, 11:47 AM

Herald added a subscriber: imp. · View Herald TranscriptTue, May 12, 11:47 AM

pouria requested review of this revision.Tue, May 12, 11:47 AM

Harbormaster completed remote builds in B73011: Diff 177633.Tue, May 12, 11:47 AM

markj added inline comments.Tue, May 12, 4:04 PM

sys/netlink/route/rt.c
484	Are you sure that NL_ITEM_ALIGN() won't cause the value to wrap around, e.g., if `rtnh->rtnh_len == 0xffff`? I suspect we should also check `NL_ITEM_ALIGN(rtnh->rtnh_len) >= rtnh->rtnh_len`.
490	Here I guess we're assuming that nl_parse_header() isn't going to modify `*rtnh`, even though the first parameter is not a pointer to const? It would be nice if this code actually used the `const` qualifier.
497–498	It might be a bit cleaner to assign `len` at the beginning of the loop.

@markj you're absolutely right. my mistake.
Thank you!

Harbormaster completed remote builds in B73060: Diff 177718.Tue, May 12, 9:56 PM

pouria added inline comments.Tue, May 12, 9:56 PM

sys/netlink/route/rt.c
490	I'll work on it in another revision.

It would be great to add another regression test for this. I suspect that a KASAN kernel would panic if given Robert's original test case, but I haven't tried it.