Re: [PATCH v2 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] Calling vfree() on a PMD-aligned sub-chunk frees the entire vmalloc allocation, causing a Use-After-Free for the remaining active portions.
- [High] Ignored error return from mas_store_gfp() in execmem_cache_clean() leaves dangling pointers in the free list.
- [High] Unchecked maple tree operations in execmem_cache_alloc_locked() cause double allocations or memory leaks.
--

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

mm/execmem: use VM_FLUSH_RESET_PERMS for ROX cache allocations

This commit removes the custom wrappers for set_direct_map APIs in
execmem by utilizing VM_FLUSH_RESET_PERMS for vmalloc allocations. It
simplifies the handling of the ROX cache when allocating and freeing
PMD-aligned chunks.

> 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)) {
> -			struct vm_struct *vm = find_vm_area(area);
> -
> -			execmem_set_direct_map_valid(vm, true);
>  			mas_store_gfp(&mas, NULL, GFP_KERNEL);
>  			vfree(area);

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

The return value of mas_store_gfp() is ignored right before calling
vfree(area). If the maple tree operation fails with -ENOMEM during
rebalancing, the free chunk remains in the tree while the memory is freed
via vfree().

Could this lead to a use-after-free if the allocator hands out the freed
memory to a new caller?

[Severity: Critical]
This is a pre-existing issue, but does calling vfree() on a PMD-aligned
sub-chunk here free the entire vmalloc allocation?

If a caller initially allocates a large region (e.g., 4MB) which is later
partially allocated by smaller requests, it can leave behind a free chunk
starting at a PMD-aligned index (like p + 2MB).

When execmem_cache_clean() runs, it sees that both the size and index are
PMD-aligned and calls vfree() on that sub-chunk. Since vfree() uses an
interval tree lookup to resolve inner pointers, it will find and free the
entire original 4MB vmap_area.

Could this leave the other 2MB of actively executing memory in a
use-after-free state?

>  		}

[ ... ]

> @@ -312,18 +288,15 @@ static void *execmem_cache_populate_alloc(struct execmem_range *range, size_t si
>  	 */
>  	mutex_lock(mutex);
>  	err = execmem_cache_add_locked(p, alloc_size, GFP_KERNEL);
> -	if (err)
> -		goto err_reset_direct_map;
> -
> -	p = execmem_cache_alloc_locked(range, size);
> -
> +	if (!err)
> +		p = execmem_cache_alloc_locked(range, size);

[Severity: High]
This is a pre-existing issue, but does execmem_cache_alloc_locked() handle
maple tree operation failures correctly?

Inside execmem_cache_alloc_locked(), mas_store_gfp() is used to remove a
chunk from free_areas, but the return value is ignored. If it fails, the
memory is given to the caller but remains in free_areas, which seems like
it could cause a double allocation.

Furthermore, if the free area was split and re-inserting the remainder
fails:

mm/execmem.c:execmem_cache_alloc_locked() {
    ...
	mas_set_range(&mas_free, addr + size, last);
	err = mas_store_gfp(&mas_free, ptr, GFP_KERNEL);
	if (err) {
		mas_store_gfp(&mas_busy, NULL, GFP_KERNEL);
		return NULL;
	}
    ...
}

The allocation is aborted and the entry is cleared from busy_areas, but the
original free chunk is not restored back into free_areas. Does this
permanently leak the entire chunk?

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