Add guarded memcpy

[jvoisin]

snmalloc has a cool low overhead check in memcpy to detect cross-zones copies, is this something that we might want to add to hardened_malloc?

[thestinger]

It's already supported via the malloc_object_size APi and it's up to libc if it wants to use it.

[jvoisin]

snmalloc provides it natively, without having to have the libc support it.

[qua3k]

Would such a mitigation prevent any more attacks than the slab canaries already do?

[jvoisin]

This would catch corruption before it happens, while slab canaries are only detecting it maybe at some point.

But with MTE, it's kinda useless I think.

[thestinger]

@jvoisin Yeah, it's useless with MTE, but we support platforms without MTE so it would make sense to address as a low priority feature.

[jvoisin]

It adds a fair share a complexity:

memcpy_guarded>:
    mov    rax,QWORD PTR [rip+0xbfa]        # Load Chunk map base
    test   rax,rax                          # Check if chunk map is initialised
    je     DONE                             #  |
    mov    rcx,rdi                          # Get chunk map entry
    shr    rcx,0xa                          #  |
    and    rcx,0xfffffffffffffff0           #  |
    mov    rax,QWORD PTR [rax+rcx*1+0x8]    # Load sizeclass
    and    eax,0x7f                         #  |
    shl    rax,0x5                          #  |
    lea    r8,[sizeclass_meta_data]         #  |
    mov    rcx,QWORD PTR [rax+r8*1]         # Load object size
    mov    r9,QWORD PTR [rax+r8*1+0x8]      # Load slab mask
    and    r9,rdi                           # Offset within slab
    mov    rax,QWORD PTR [rax+r8*1+0x10]    # Load modulus constant
    imul   rax,r9                           # Perform recripocal modulus
    shr    rax,0x36                         #  |
    imul   rax,rcx                          #  |
    sub    rcx,r9                           # Find distance to end of object.
    add    rcx,rax                          #  |
    cmp    rax,rdx                          # Compare to length of memcpy.
    jb     ERROR                            #  |
DONE:
    jmp    <memcpy>
ERROR:
    ud2                                     # Trap

I'm not sure it's worth the hassle.

[SkewedZeppelin]

Here are some Android examples that this could benefit:
A-194105348 CVE-2021-39623 EoP/High
A-246932269 CVE-2023-20945 EoP/High
A-258652631 CVE-2023-20951 RCE/Critical
A-261867748 CVE-2023-20954 RCE/Critical
A-275418191 CVE-2023-21127 RCE/Critical
A-276898739 CVE-2024-0030 ID/High

[thestinger]

There are guaranteed guard pages around everything above 16k with the default guard slab internal of 1, so that does provide protection against a lot of attacks even without memory tagging. We did used to implement a feature like this for a lot of libc functions but that has been gone for years now. The infrastructure for it is partially there is hardened_malloc already via the object size functions.

[SkewedZeppelin]

I made memcpy use malloc_object_size and can run a whole system with it but I'm seeing spurious crashes from some gnome apps:

Mar 21 07:32:59 localhost org.gnome.clocks.desktop[3080]: fatal allocator error: invalid malloc_object_size
Mar 21 07:33:31 localhost org.gnome.Calculator.desktop[3833]: fatal allocator error: invalid malloc_object_size
Mar 21 07:35:04 localhost org.gnome.baobab.desktop[5046]: fatal allocator error: invalid malloc_object_size
Mar 21 07:35:16 localhost org.gnome.FileRoller.desktop[5299]: fatal allocator error: invalid malloc_object_size
Mar 21 07:35:25 localhost org.gnome.Logs.desktop[5491]: fatal allocator error: invalid malloc_object_size

[mjp41]

Is that a memcpy to the inside of an object. snmalloc's metadata can look up meta data on any interior pointer of an allocation. This is required as memcpy doesn't have to be to the start of the object.

[thestinger]

@SkewedZeppelin That error is when the slot has already been freed. That could be a use-after-free bug in GNOME.

        if (unlikely(!is_used_slot(metadata, slot))) {
            fatal_error("invalid malloc_object_size");
        }

#if SLAB_QUARANTINE
        if (unlikely(is_quarantine_slot(metadata, slot))) {
            fatal_error("invalid malloc_object_size (quarantine)");
        }
#endif

[thestinger]

@mjp41 It can always return either a size or SIZE_MAX but for malloc_object_size as opposed to malloc_object_size_fast, it checks whether the slot is free or quarantined in the bitmap and considers it a fatal error to check the size of a free slot. It doesn't appear to be an incompatibility between the implementation and how it's expected to be used. It looks like a use-after-free bug.

We haven't written the code to always return a size for large allocations above the max slab allocation size since we weren't using this functionality ourselves anymore. Large allocations always have at least 1 page guards at the start and end which are randomly sized up to 50% of the allocation size. In the default non-lite configuration, slabs have guard slabs between them. There are random canaries with a guaranteed leading zero for slab allocations which don't stop large or linear overflows but they do help. On the Pixel 8 and later, and as one of our baseline requirements for GrapheneOS, we require hardware memory tagging. You can see we guarantee that adjacent slab allocations have distinct tags by dynamically excluding the current (or previous, when free) random tags for the allocations below and above along with excluding the previous tag for the current allocation and the reserved tag used for free. This means we are always guaranteed to catch small overflows and linear overflows for both large / slab allocations along with non-linear overflows into the next slab allocation. Therefore, we don't have much interest in adding back what we used to do in libc for GrapheneOS where all of the system calls like read, write, etc. checked for overflow.