Page MenuHomeFreeBSD

Add a helper function for validating VA ranges.

Authored by markj on Jun 17 2020, 6:30 PM.



Functions which take untrusted user ranges must validate against the
bounds of the map, and also check for wraparound. Instead of having the
same logic duplicated in a number of places, add a function to check.

This somewhat duplicates the VM_MAP_RANGE_CHECK macro, which does not in
fact do any checking but rather clips the range based on the map bounds.
Some functions (like kern_mprotect()) rely on this, while others
(kern_madvise()) do not. As a future step I think we should add such
checking to all syscall paths, and convert VM_MAP_RANGE_CHECK to
assert the validity of the range.

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Jun 17 2020, 6:30 PM
markj created this revision.
262 ↗(On Diff #73232)

At least the code in vm_map.c does <= check there.

I think it visibly changes the user error code.

262 ↗(On Diff #73232)

That case is weird. Note it checks addr + length <= length, not <= addr. So it is just checking for overflow, not for length == 0. I'm not sure if it is intended or not.

This revision is now accepted and ready to land.Jun 18 2020, 5:12 PM
1619 ↗(On Diff #73232)

Why is this test different from the others?

1619 ↗(On Diff #73232)

I just missed it.

See also vm_fault_quick_hold_pages in vm_fault.c.

This revision now requires review to proceed.Jun 18 2020, 9:40 PM

See also vm_fault_quick_hold_pages in vm_fault.c.

Thanks. I also converted a similar check in the LinuxKPI.

This revision is now accepted and ready to land.Jun 18 2020, 10:05 PM
dougm added inline comments.
260 ↗(On Diff #73295)

vm_map_valid_range sounds more boolean. A non-blocking suggestion.

260 ↗(On Diff #73295)

I think that sounds better. vm_map_range_valid() seems a bit more natural to me, so I will go with that.

This revision was automatically updated to reflect the committed changes.

This assignment is pointless. See below.