Re: [PATCH v2 04/13] mm: update generic_get_unmapped_area[_topdown]() to use vma_flags_t

Re: [PATCH v2 04/13] mm: update generic_get_unmapped_area[_topdown]() to use vma_flags_t

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the x86-specific bottom-up mmap fallback path silently discards VMA flags, bypassing the shadow stack guard gap allocation.
--

--- Patch [4]: [PATCH v2 04/13] mm: update generic_get_unmapped_area[_topdown]() to use vma_flags_t ---
commit 00b351f4aaa23a000490d96bd47f04d6f2e0f5d1
Author: Lorenzo Stoakes <ljs@kernel.org>

mm: update generic_get_unmapped_area[_topdown]() to use vma_flags_t

This commit updates generic_get_unmapped_area() and
generic_get_unmapped_area_topdown() to accept vma_flags_t instead of
vm_flags_t. It also updates stack_guard_placement() to use the new type.
The architecture-specific wrappers are retained but use legacy_to_vma_flags()
for compatibility.

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 4a88b2a233f7f..39d33f3a55c2e 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c

[ ... ]

> @@ -807,7 +807,7 @@ arch_get_unmapped_area_topdown(struct file *filp, unsigned long addr,
>  			       unsigned long flags, vm_flags_t vm_flags)
>  {
>  	return generic_get_unmapped_area_topdown(filp, addr, len, pgoff, flags,
> -						 vm_flags);
> +						 legacy_to_vma_flags(vm_flags));

[Severity: High]
This isn't a bug introduced by this patch, but the x86-specific bottom-up mmap
fallback path silently discards VMA flags, bypassing the shadow stack guard
gap allocation.

Is it possible for an attacker to intentionally exhaust the top-down allocation
space (e.g., via high stack limits) to force the shadow stack allocation (using
the map_shadow_stack syscall) into the fallback bottom-up path in
arch_get_unmapped_area_topdown()?

In arch/x86/kernel/sys_x86_64.c, the fallback path looks like this:

bottomup:
	/*
	 * A failed mmap() very likely causes application failure,
	 * so fall back to the bottom-up function here. This scenario
	 * can happen with large stack limits and large mmap()
	 * allocations.
	 */
	return arch_get_unmapped_area(filp, addr0, len, pgoff, flags, 0);

By passing 0 for vm_flags, does this completely drop the VM_SHADOW_STACK flag
before the guard gap allocation logic checks for it?

Could this bypass allow shadow stacks to be allocated immediately adjacent to
other mappings, meaning if a shadow stack overflows, it corrupts adjacent data
instead of hitting a guard page?

>  }
>  #endif
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-vma-flags-mm-v2-0-0fa2357d5431@kernel.org?part=4

Re: [PATCH v2 11/13] mm/mlock: convert mlock code to use vma_flags_t

[sashiko-bot]

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

Pre-existing issues:
- [High] Page table walk callbacks trigger an infinite loop when pte_offset_map_lock() fails.
--

--- Patch [11]: [PATCH v2 11/13] mm/mlock: convert mlock code to use vma_flags_t ---
commit 4497d6e0c6929dd585b156beb8ad3252bc077a68
Author: Lorenzo Stoakes <ljs@kernel.org>

mm/mlock: convert mlock code to use vma_flags_t

Replace use of the legacy vm_flags_t flags with vma_flags_t values
throughout the mlock logic. No functional change intended.

> diff --git a/mm/mlock.c b/mm/mlock.c
> index 34ffa954006fb..efa6716e4dfbd 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c

[ ... ]

> @@ -368,7 +368,7 @@ static int mlock_pte_range(pmd_t *pmd, unsigned long addr,
>  		folio = pmd_folio(*pmd);
>  		if (folio_is_zone_device(folio))
>  			goto out;
> -		if (vma->vm_flags & VM_LOCKED)
> +		if (vma_test(vma, VMA_LOCKED_BIT))
>  			mlock_folio(folio);
>  		else
>  			munlock_folio(folio);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this function, I
noticed a potential infinite loop in mlock_pte_range(). 

When pte_offset_map_lock() returns NULL (for example, when encountering a
non-present PMD migration entry during concurrent THP migration), the callback
sets ACTION_AGAIN and returns 0:

	start_pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
	if (!start_pte) {
		walk->action = ACTION_AGAIN;
		return 0;
	}

Does this cause walk_pmd_range() to continuously retry without yielding the
CPU? 

Because the function returns 0, it entirely bypasses the cond_resched()
call at the end of mlock_pte_range(). Meanwhile, ACTION_AGAIN instructs
walk_pmd_range() to jump to its retry label and re-process the exact same
PMD. 

Could an unprivileged user space process trigger a soft or hard CPU lockup
by calling mlock() on a memory range during a concurrent migration?

I also noticed this exact same pattern appears in other page walk callbacks
that return 0 with ACTION_AGAIN when pte_offset_map_lock() fails, such as
queue_folios_pmd(), mincore_pte_range(), smaps_pte_range(),
pagemap_pmd_range(), gather_pte_stats(), clear_refs_pte_range(), and
pagemap2_pte_range().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-vma-flags-mm-v2-0-0fa2357d5431@kernel.org?part=11

Re: [PATCH v2 13/13] mm/mremap: convert mremap code to use vma_flags_t

[sashiko-bot]

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

Pre-existing issues:
- [High] Permanent leak of `vm_committed_as` committed memory when `do_vmi_munmap()` fails in `unmap_source_vma()`.
- [High] Permanent leak of `vm_committed_as` (delta size) in the `move_vma()` error path when page table moves fail.
- [High] Permanent leak of `mm->locked_vm` when `MREMAP_DONTUNMAP` is called on a locked VMA.
- [High] `mremap_to()` with `MREMAP_SHRINK` permanently inflates committed memory by failing to reset `vrm->delta`.
--

--- Patch [13]: [PATCH v2 13/13] mm/mremap: convert mremap code to use vma_flags_t ---
commit 2acb878e611c682f5c4e70b5f73085e003532a49
Author: Lorenzo Stoakes <ljs@kernel.org>

mm/mremap: convert mremap code to use vma_flags_t

This commit replaces legacy vm_flags_t with vma_flags_t bitwise values
across the mremap codebase. It updates flag modification functions to their
corresponding vma_* counterparts. No functional changes are intended.

> diff --git a/mm/mremap.c b/mm/mremap.c
> index 384ef4cc2195b..b64aa1f6e07ed 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c

[ ... ]

> @@ -985,7 +985,7 @@ static bool vrm_calc_charge(struct vma_remap_struct *vrm)
>  {
>  	unsigned long charged;
>  
> -	if (!(vrm->vma->vm_flags & VM_ACCOUNT))
> +	if (!vma_test(vrm->vma, VMA_ACCOUNT_BIT))
>  		return true;

[Severity: High]
This isn't a bug introduced by this patch, but since we're updating the
accounting logic here, could this lead to a committed memory leak when
handling a shrink operation?

When mremap_to() processes MREMAP_SHRINK, it sets vrm->old_len to
vrm->new_len but appears to leave vrm->delta at its original value. When
vrm_calc_charge() is subsequently called during move_vma(), it uses
vrm->delta to charge new committed memory, effectively re-charging the
memory that was just shrunk.

Does this permanently inflate vm_committed_as under strict overcommit?

[ ... ]

> @@ -1195,7 +1195,7 @@ static void unmap_source_vma(struct vma_remap_struct *vrm)
>  	 * portions of the original VMA that remain.
>  	 */
>  	if (accountable_move) {
> -		vm_flags_clear(vma, VM_ACCOUNT);
> +		vma_clear_flags(vma, VMA_ACCOUNT_BIT);
>  		/* We are about to split vma, so store the start/end. */
>  		vm_start = vma->vm_start;
>  		vm_end = vma->vm_end;

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak of committed
memory here during error handling in unmap_source_vma().

If do_vmi_munmap() fails shortly after this block (e.g., due to OOM when
splitting an adjacent VMA), the error path calls vm_acct_memory() but
returns early without restoring VMA_ACCOUNT_BIT on the surviving VMA:

	if (err) {
		/* OOM: unable to split vma, just get accounts right */
		vm_acct_memory(len >> PAGE_SHIFT);
		return;
	}

Because the bit is stripped, will the system permanently leak this charge
since do_vmi_munmap() won't unaccount it when the VMA is eventually torn
down?

[Severity: High]
This is also a pre-existing issue on the same path. Can this lead to a
similar leak if move_page_tables() fails inside copy_vma_and_data()?

In that scenario, copy_vma_and_data() assigns new_vma to vrm->vma and
returns an error. Then, move_vma() invokes unmap_source_vma() to clean up
new_vma.

This block will strip VMA_ACCOUNT_BIT from new_vma before unmapping
it, which seems to prevent do_vmi_munmap() from uncharging the delta added
earlier by vrm_calc_charge().

[ ... ]

> @@ -1330,8 +1332,8 @@ static void dontunmap_complete(struct vma_remap_struct *vrm,
>  	unsigned long old_start = vrm->vma->vm_start;
>  	unsigned long old_end = vrm->vma->vm_end;
>  
> -	/* We always clear VM_LOCKED[ONFAULT] on the old VMA. */
> -	vm_flags_clear(vrm->vma, VM_LOCKED_MASK);
> +	/* We always clear VMA_LOCKED[ONFAULT]_BIT on the old VMA. */
> +	vma_clear_flags_mask(vrm->vma, VMA_LOCKED_MASK);

[Severity: High]
This is a pre-existing issue, but could stripping the locked mask here leak
the mm->locked_vm count?

By clearing VMA_LOCKED_MASK unconditionally from the old VMA without
decrementing mm->locked_vm or unlocking the pages, it seems the old VMA
will no longer account for its locked pages when it is eventually unmapped.

Since move_vma() already added the new VMA's size to mm->locked_vm, does
this permanently inflate the process's locked memory count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-vma-flags-mm-v2-0-0fa2357d5431@kernel.org?part=13

Re: [PATCH v2 08/13] mm: introduce vma_get_page_prot() and use it

[sashiko-bot]

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

Pre-existing issues:
- [High] Modifying `vma->vm_page_prot` after `vm_mmap()` without `vma_start_write(vma)` causes data races with per-VMA lock readers and allows cache aliasing via concurrent page faults.
- [High] Multiple DRM drivers call `drm_gem_vm_close(vma)` on their internal `mmap` error paths, causing a double-put and Use-After-Free (UAF).
- [High] `i915_gem_mmap_ioctl()` leaks a user-space memory mapping (VMA) if operations fail after `vm_mmap()` succeeds.
- [High] `exynos_drm_gem_mmap()` leaks a GEM object reference when delegating mapping of an imported dma-buf.
--

--- Patch [8]: [PATCH v2 08/13] mm: introduce vma_get_page_prot() and use it ---
commit 02617eb8c6f24873696c9debadc114509af28be5
Author: Lorenzo Stoakes <ljs@kernel.org>

mm: introduce vma_get_page_prot() and use it

This commit introduces a new helper vma_get_page_prot() to simplify
retrieving the page protection from a VMA. It replaces numerous direct
calls to vm_get_page_prot() across various DRM and core mm files.

> diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c
> index 1c00a71ab3c9d..7d9612075d310 100644
> --- a/drivers/gpu/drm/drm_gem_dma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_dma_helper.c
> @@ -540,7 +540,7 @@ int drm_gem_dma_mmap(struct drm_gem_dma_object *dma_obj, struct vm_area_struct *
>  	vm_flags_mod(vma, VM_DONTDUMP | VM_DONTEXPAND, VM_PFNMAP);
>  
>  	if (dma_obj->map_noncoherent) {
> -		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +		vma->vm_page_prot = vma_get_page_prot(vma);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential reference
count underflow here.

In drm_gem_dma_mmap(), if the dma_mmap_pages() or dma_mmap_wc() mapping
fails, we call drm_gem_vm_close(vma) which drops the GEM object reference.
However, the caller drm_gem_mmap_obj() will also hit its own error path
and call drm_gem_object_put(obj) again on the same object. Could this
lead to a double-put and use-after-free? I see a similar pattern in
__tegra_gem_mmap() in the tegra driver as well.

>  
>  		ret = dma_mmap_pages(drm_dev_dma_dev(dma_obj->base.dev),
>  				     vma, vma->vm_end - vma->vm_start,

> [ ... ]

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 9a6270f3dca6b..0208c92595725 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -377,13 +377,13 @@ static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct
>  
>  	/* non-cachable as default. */
>  	if (exynos_gem->flags & EXYNOS_BO_CACHABLE)
> -		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +		vma->vm_page_prot = vma_get_page_prot(vma);

[Severity: High]
This is a pre-existing issue, but does this function leak a GEM object
reference when mapping an imported dma-buf?

When exynos_drm_gem_mmap() delegates an imported dma-buf to dma_buf_mmap()
early in the function, the dma_buf_mmap() routine replaces
vma->vm_private_data and vma->vm_ops with the exporter's state. When the
VMA is later unmapped, the exporter's close handler is invoked instead of
drm_gem_vm_close(). Doesn't this mean the reference originally taken by
drm_gem_mmap_obj() right before calling this callback is never dropped?

>  	else if (exynos_gem->flags & EXYNOS_BO_WC)
>  		vma->vm_page_prot =
> -			pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +			pgprot_writecombine(vma_get_page_prot(vma));

> [ ... ]

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 0644f85c6c8e5..9ca90c1bb5b42 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -112,7 +112,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,

[Severity: High]
This isn't a bug introduced by this patch, but could this code leak a VMA?

If vm_mmap() succeeds earlier in i915_gem_mmap_ioctl(), but we then hit
the mmap_write_lock_killable() failure path:

i915_gem_mmap_ioctl()
    addr = vm_mmap(...);
    ...
    if (mmap_write_lock_killable(mm)) {
        addr = -EINTR;
        goto err;
    }
    
Does this overwrite the successfully mapped address and jump to the error
handling without unmapping the VMA, permanently leaking it?

>  		vma = find_vma(mm, addr);
>  		if (vma && __vma_matches(vma, obj->base.filp, addr, args->size))
>  			vma->vm_page_prot =
> -				pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +				pgprot_writecombine(vma_get_page_prot(vma));

[Severity: High]
This is also a pre-existing issue, but is it safe to modify the VMA
page protection properties here without holding the per-VMA write lock?

We hold the mmap_write_lock, but since vma_start_write() is not called
first, per-VMA lock readers are not drained. Could a concurrent page
fault handle install Write-Back PTEs while we change the VMA to
Write-Combine, leading to cache aliasing?

>  		else
>  			addr = -ENOMEM;
>  		mmap_write_unlock(mm);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-vma-flags-mm-v2-0-0fa2357d5431@kernel.org?part=8