On 10/11/25 15:03, Matthew Lugg wrote:
The previous logic here had an off-by-one error: assuming 4k pages on
host and guest, if `len == 4097` (indicating to unmap 2 pages), then
`last = start + 4096`, so `real_last = start + 4095`, so ultimately
`real_len = 4096`. I do not believe this could cause any observable bugs
in guests, because `target_munmap` page-aligns the length it passes in.
However, calls to this function in `target_mremap` do not page-align the
length, so those calls could "drop" pages, leading to a part of the
reserved region becoming unmapped. At worst, a host allocation could get
mapped into that hole, then clobbered by a new guest mapping.
A simple fix didn't feel ideal here, because I think this function was
not written as well as it could be. Instead, the logic is simpler if we
use `end = start + len` instead of `last = start + len - 1` (overflow
does not cause any problem here), and use offsets in the loops (avoiding
overflows since the offset is never larger than the host page size).
No, it is not simpler with 'end', because end == 0 is 'valid' in the sense that the range
goes from [start, (abi_ptr)-1]. But having end <= start is awkward in the extreme.
Thus we prefer the inclusive range [start, last] to the exclusive range [start,
end).
Not everything has been converted away from 'end', but we certainly should not regress
existing code.
r~
