Re: [PATCH 5/6] mm/execmem: use VM_FLUSH_RESET_PERMS for ROX cache allocations

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] execmem_cache_clean() calls vfree() on partial or interior chunks of a larger vmalloc allocation, causing a use-after-free for actively used regions.
- [High] Unchecked return value of mas_store_gfp() during maple tree deletion can leave a dangling pointer in the free tree if deletion fails.
- [Medium] Cache memory may never be freed because execmem_vmalloc() does not guarantee PMD_SIZE alignment on all architectures, preventing execmem_cache_clean() from matching its alignment checks.
--

--- Patch [5]: [PATCH 5/6] mm/execmem: use VM_FLUSH_RESET_PERMS for ROX cache allocations ---
commit 32d82345ceeccc2881319ef4ff8f3be90e5de6df
Author: Mike Rapoport (Microsoft) <[email protected]>

mm/execmem: use VM_FLUSH_RESET_PERMS for ROX cache allocations
    
Initially execmem removed direct map aliases for ROX cache allocations and
restored them manually. This patch leverages vmalloc's VM_FLUSH_RESET_PERMS
to handle these updates without splitting the direct map for PMD_SIZE chunks,
removing the need for custom set_direct_map wrappers.

> diff --git a/mm/execmem.c b/mm/execmem.c
> index 74a178a87e758..d35f1d0ea54a4 100644
> --- a/mm/execmem.c
> +++ b/mm/execmem.c
[ ... ]
> @@ -169,9 +148,6 @@ static void execmem_cache_clean(struct work_struct *work)
>  
>  		if (IS_ALIGNED(size, PMD_SIZE) &&
>  		    IS_ALIGNED(mas.index, PMD_SIZE)) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
This is a pre-existing issue, but does this strict alignment check prevent
unaligned vmalloc allocations from ever being cleaned up?

execmem_cache_populate_alloc() rounds the allocation size up to PMD_SIZE,
but calls execmem_vmalloc() with vm_flags |= VM_ALLOW_HUGE_VMAP. If the
architecture does not support arch_vmap_pmd_supported(PAGE_KERNEL),
__vmalloc_node_range() falls back to PAGE_SIZE alignment.

If the returned base address is not PMD_SIZE aligned, the free block's
index will never be PMD-aligned, potentially permanently leaking the cache
block even when it is completely empty.

> -			struct vm_struct *vm = find_vm_area(area);
> -
> -			execmem_set_direct_map_valid(vm, true);
>  			mas_store_gfp(&mas, NULL, GFP_KERNEL);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but what happens if
mas_store_gfp() fails here?

Modifying a maple tree under RCU can require allocating a new node. If this
node allocation fails and returns -ENOMEM, the entry is not removed from
free_areas. 

Since the return value is ignored, the memory is subsequently freed via
vfree(area), leaving a dangling pointer in the free tree that could be
returned by a subsequent allocation.

>  			vfree(area);
                        ^^^^^^^^^^^

[Severity: Critical]
This is a pre-existing issue, but does calling vfree() on a partial or
interior chunk of a larger vmalloc allocation cause a use-after-free for
actively used regions?

If a multi-PMD block is allocated (e.g., 4MB) and an interior PMD_SIZE free
block is formed (e.g., area = p + 2MB), execmem_cache_clean() blindly calls
vfree(area) on it.

However, vfree() relies on find_unlink_vmap_area(), which matches any
interior pointer to the containing vmap_area:

mm/vmalloc.c:find_unlink_vmap_area() {
    ...
    va = __find_vmap_area(addr, &vn->busy.root);
    ...
}

Won't this completely unmap and free the entire original 4MB block,
destroying the actively used portions alongside the free chunk? This could
potentially be triggered by userspace loading and unloading BPF programs
or kernel modules of specific sizes.

>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5