On Sat, 11 Oct 2025 at 21:20, Matthew Lugg <[email protected]> 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).
I don't really understand this code, I'm just looking at
it fresh, so my comment below might be wrong.
> - /*
> - * If guest pages remain on the first or last host pages,
> - * adjust the deallocation to retain those guest pages.
> - * The single page special case is required for the last page,
> - * lest real_start overflow to zero.
> - */
This comment says we need the special case for
"real_last - real_start < host_page_size" to avoid an overflow.
> - if (real_last - real_start < host_page_size) {
> - prot = 0;
We delete the special case...
> - for (a = real_start; a < start; a += TARGET_PAGE_SIZE) {
> - prot |= page_get_flags(a);
> - }
> - for (a = last; a < real_last; a += TARGET_PAGE_SIZE) {
> - prot |= page_get_flags(a + 1);
> - }
> - if (prot != 0) {
> - return 0;
> - }
> - } else {
> - for (prot = 0, a = real_start; a < start; a += TARGET_PAGE_SIZE) {
> - prot |= page_get_flags(a);
> - }
> - if (prot != 0) {
> - real_start += host_page_size;
> - }
> + /* end or real_end may have overflowed to 0, but that's okay. */
>
> - for (prot = 0, a = last; a < real_last; a += TARGET_PAGE_SIZE) {
> - prot |= page_get_flags(a + 1);
> - }
> - if (prot != 0) {
> - real_last -= host_page_size;
> - }
> + /* If [real_start,start) contains a mapped guest page, retain the first
> page. */
> + for (prot = 0, off = 0; off < start - real_start; off +=
> TARGET_PAGE_SIZE) {
> + prot |= page_get_flags(real_start + off);
> + }
> + if (prot != 0) {
> + real_start += host_page_size;
...and now if real_start was the last page in the
address space, this addition will overflow it to zero.
> + }
>
> - if (real_last < real_start) {
> - return 0;
> - }
> + /* If [end,real_end) contains a mapped guest page, retain the last page.
> */
> + for (prot = 0, off = 0; off < real_end - end; off += TARGET_PAGE_SIZE) {
> + prot |= page_get_flags(end + off);
> + }
> + if (prot != 0) {
> + real_end -= host_page_size;
> }
>
> - real_len = real_last - real_start + 1;
> host_start = g2h_untagged(real_start);
> -
> - return do_munmap(host_start, real_len);
> + return do_munmap(host_start, real_end - real_start);
> }
>
> int target_munmap(abi_ulong start, abi_ulong len)
thanks
-- PMM
