kern/rman: fix overflow in rman_reserve_resource_bound()

[ehem_freebsd_m5p.com]

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.

[ehem_freebsd_m5p.com]

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.

[ehem_freebsd_m5p.com]

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]

In D30280#680286, @ehem_freebsd_m5p.com wrote:

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.

[jhb]

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.

[ehem_freebsd_m5p.com]

@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.