Page MenuHomeFreeBSD

mmap(2): fix MAP_32BIT when ASLR is disabled
ClosedPublic

Authored by kib on Jul 23 2023, 9:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 5:47 AM
Unknown Object (File)
Fri, Nov 15, 7:38 PM
Unknown Object (File)
Fri, Nov 15, 5:49 PM
Unknown Object (File)
Wed, Oct 30, 12:22 PM
Unknown Object (File)
Tue, Oct 22, 11:20 AM
Unknown Object (File)
Oct 18 2024, 10:17 AM
Unknown Object (File)
Oct 1 2024, 4:57 AM
Unknown Object (File)
Sep 23 2024, 8:40 PM
Subscribers

Details

Summary
After the specified revision, vm_map_object() with fitit == true passes
min_addr equal to the end of the data segment, to vm_map_find_min().
This breaks MAP_32BIT since min_addr > max_addr. Make MAP_32BIT a
special case for kern_mmap() by setting min_addr to vm_map_min().

Reported by:    dchagin
Fixes:  d8e6f4946cec0b84a6997d62e791b8cf993741b2

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.Jul 23 2023, 9:46 PM
markj added inline comments.
sys/vm/vm_mmap.c
1628 ↗(On Diff #125057)

This computation is thrown away in the MAP_32BIT case. You could add an else clause to avoid that.

This revision is now accepted and ready to land.Jul 24 2023, 1:52 PM
This revision now requires review to proceed.Jul 24 2023, 7:43 PM

I'd like to suggest this patch instead:

diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index 444e09986d4e..eb607d519247 100644
--- a/sys/vm/vm_map.c
+++ b/sys/vm/vm_map.c
@@ -2255,10 +2255,10 @@ vm_map_find_min(vm_map_t map, vm_object_t object, vm_ooffset_t offset,
        int rv;
 
        hint = *addr;
-       if (hint == 0)
+       if (hint == 0) {
                cow |= MAP_NO_HINT;
-       if (hint < min_addr)
                *addr = hint = min_addr;
+       }
        for (;;) {
                rv = vm_map_find(map, object, offset, addr, length, max_addr,
                    find_space, prot, max, cow);

This patch also restores the ability of non-MAP_32BIT mmap()s to allocate starting from hints below the text segment.

In D41159#937487, @alc wrote:

I'd like to suggest this patch instead:

diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index 444e09986d4e..eb607d519247 100644
--- a/sys/vm/vm_map.c
+++ b/sys/vm/vm_map.c
@@ -2255,10 +2255,10 @@ vm_map_find_min(vm_map_t map, vm_object_t object, vm_ooffset_t offset,
        int rv;
 
        hint = *addr;
-       if (hint == 0)
+       if (hint == 0) {
                cow |= MAP_NO_HINT;
-       if (hint < min_addr)
                *addr = hint = min_addr;
+       }
        for (;;) {
                rv = vm_map_find(map, object, offset, addr, length, max_addr,
                    find_space, prot, max, cow);

This patch also restores the ability of non-MAP_32BIT mmap()s to allocate starting from hints below the text segment.

Please go ahead with your fix.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 26 2023, 5:26 AM
Closed by commit rG50d663b14b31: vm: Fix vm_map_find_min() (authored by alc). · Explain Why
This revision was automatically updated to reflect the committed changes.
sys/vm/vm_mmap.c
362 ↗(On Diff #125091)

Shouldn't this expression include the size? Otherwise, if the given address is close to vm_taddr, such that there isn't enough space between the given address and vm_taddr, the find space code will select an unused address range within the legacy heap region.

sys/vm/vm_mmap.c
362 ↗(On Diff #125091)

Isn't it the reverse? If the hint falls into the legacy heap, we must move it.

If addr + size is compared with the text address, instead of add, we might end up allowing allocation below text, for sufficiently large size.

sys/vm/vm_mmap.c
362 ↗(On Diff #125091)

No, I don't think that I have it backwards. Setting aside the possibility of overflow by addr + size for the moment, if addr > vm_taddr, then so is addr + size. However, suppose that vm_taddr is 0x200000, addr is 0x1ff000, and size is 8192. Today, I believe that would result in the allocation of an address range in the legacy heap.

sys/vm/vm_mmap.c
362 ↗(On Diff #125091)

I agree.