Page MenuHomeFreeBSD

kern/rman: fix overflow in rman_reserve_resource_bound()
ClosedPublic

Authored by ehem_freebsd_m5p.com on May 15 2021, 7:38 PM.

Details

Summary

If the range [0, ~0] is given, then (0 - ~0) + 1 == 0. This in turn
will cause any allocation of non-zero size to fail.

History indicates it is part of the original rman code. This bug may in
fact be older than some contributors.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There is actually a trade-off between two issues here. Is it valuable for rman_reserve_resource_bound() to handle the case of being given a count of 0? Is it valuable for rman_reserve_resource_bound() to handle the situation of count == 0? Both are a bit funky and a bit valuable to handle.

Having rstart=0 and rend=~0 is actually likely to hint at issues underneath. Along this line looking at sys/arm64/arm64/nexus.c, I'm pretty sure what it is doing isn't plausible. Perhaps handling count=0 has some value and this should be abandoned. The issues in sys/arm64/arm64/nexus.c are going to need to be fixed.

I'm actually tempted to suggest adding:

if (RMAN_IS_DEFAULT_RANGE(start, end))
        panic("resource manager initialized with illegal range");

to rman_manage_region(). Too many architectures initialize their address and interrupt rmans to that range, which means they aren't performing a useful function.

mhorne added reviewers: imp, jhb.
mhorne added a subscriber: mhorne.

There is actually a trade-off between two issues here. Is it valuable for rman_reserve_resource_bound() to handle the case of being given a count of 0? Is it valuable for rman_reserve_resource_bound() to handle the situation of count == 0? Both are a bit funky and a bit valuable to handle.

Having rstart=0 and rend=~0 is actually likely to hint at issues underneath. Along this line looking at sys/arm64/arm64/nexus.c, I'm pretty sure what it is doing isn't plausible. Perhaps handling count=0 has some value and this should be abandoned. The issues in sys/arm64/arm64/nexus.c are going to need to be fixed.

I think it's worth addressing here too, since the overflow happens with the default values assigned by rman_init().

This function makes use of count - 1 in several places already, so we need not go out of our way to accommodate count == 0. The change as-is seems correct to me.

This revision is now accepted and ready to land.Jun 18 2021, 3:44 PM

Hmmm, are there really valid use cases at this point in the API for a count of 0? We use a count of 0 for devices to let the parent bus driver determine the size (for when the size is known, e.g. from a PCI BAR or an ACPI or FDT resource entry), but a count of 0 at this layer makes no sense and should likely be an error? Hmm, I think the description of 'bound' in the rman(9) manpage is also missing a "not".

All the places doing 'count - 1' are assuming a count > 0 (or assuming -fwrapv which isn't super ideal), so I really do think a count of 0 to rman_* is an error that should fail.

@jhb on the line of count of 0 being questionable, I propose D31312. D31312 targets both sides of the issue, count of 0 is questionable, but having a resource manager with the range [0, ~0] is also rather questionable. The latter though is actually rather common right now.