Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Omitting VM_ALLOW_HUGE_VMAP breaks the ROX cache shrinker by stripping PMD_SIZE alignment, causing a permanent memory leak of cached module memory. Pre-existing issues: - [High] Fallback allocations in execmem_cache_populate_alloc() leak memory due to failing the PMD_SIZE alignment checks in the cache shrinker. - [High] Ignored return value of execmem_restore_rox() allows memory to be silently returned to the ROX cache with RW permissions. -- --- Patch [2]: [RFC PATCH 2/9] mm: execmem: Allow huge vmappings to be avoided for execmem caches --- commit 3d4b59f7ca74d71807be337626018d273686c686 Author: Ard Biesheuvel <[email protected]> mm: execmem: Allow huge vmappings to be avoided for execmem caches Only recent arm64 systems are guaranteed to be able to manipulate the permissions on live kernel mappings that may use huge mappings. Older ones can only do so on live mappings that are mapped down to pages. In order to make execmem caches work on arm64 despite this distinction, allow huge vmappings to be omitted when allocating the caches. > diff --git a/mm/execmem.c b/mm/execmem.c > index 6db0c1d82a9d6..5790d4a8532af 100644 > --- a/mm/execmem.c > +++ b/mm/execmem.c > @@ -276,7 +276,8 @@ static void *__execmem_cache_alloc(struct execmem_range > *range, size_t size) > > static void *execmem_cache_populate_alloc(struct execmem_range *range, > size_t size) > { > - unsigned long vm_flags = VM_ALLOW_HUGE_VMAP; > + unsigned long vm_flags = (range->flags & EXECMEM_NO_HUGE_VMAP) > + ? 0 : VM_ALLOW_HUGE_VMAP; [Severity: High] Does dropping VM_ALLOW_HUGE_VMAP here cause a permanent memory leak in the ROX cache? When VM_ALLOW_HUGE_VMAP is omitted, the allocation may fall back to the standard alignment (typically PAGE_SIZE) instead of being PMD_SIZE aligned. However, the shrinker in execmem_cache_clean() strictly requires PMD_SIZE alignment to free the memory: mm/execmem.c:execmem_cache_clean() { ... mas_for_each(&mas, area, ULONG_MAX) { size_t size = mas_range_len(&mas); if (IS_ALIGNED(size, PMD_SIZE) && IS_ALIGNED(mas.index, PMD_SIZE)) { struct vm_struct *vm = find_vm_area(area); ... } If the starting address (mas.index) of the chunk allocated without VM_ALLOW_HUGE_VMAP is not PMD_SIZE aligned, the shrinker will skip it, never returning it to the vmalloc subsystem. > struct mutex *mutex = &execmem_cache.mutex; > struct vm_struct *vm; > size_t alloc_size; > int err = -ENOMEM; > void *p; > > alloc_size = round_up(size, PMD_SIZE); > p = execmem_vmalloc(range, alloc_size, PAGE_KERNEL, vm_flags); > if (!p) { > alloc_size = size; > p = execmem_vmalloc(range, alloc_size, PAGE_KERNEL, vm_flags); [Severity: High] This is a pre-existing issue, but does this exact-size fallback allocation leak memory for the same reason? If the PMD_SIZE rounded allocation fails under pressure, the fallback allocates the exact requested size. Since this fallback size is not guaranteed to be PMD_SIZE aligned, it will also permanently fail the IS_ALIGNED(size, PMD_SIZE) check in execmem_cache_clean(), leaking the vmalloc space. [ ... ] > static int __execmem_cache_free(struct ma_state *mas, void *ptr, gfp_t > gfp_mask) > { > size_t size = mas_range_len(mas); > int err; > > err = execmem_force_rw(ptr, size); > if (err) > return err; > > execmem_fill_trapping_insns(ptr, size); > execmem_restore_rox(ptr, size); > > err = execmem_cache_add_locked(ptr, size, gfp_mask); [Severity: High] This isn't a bug introduced by this patch, but is it safe to ignore the return value of execmem_restore_rox() here? If restoring the ROX permissions fails (for example, if set_memory_rox() fails internally), the chunk is still added back to the free_areas tree. Subsequent allocations could then receive memory that is still writable, silently breaking the W^X security guarantee. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
