Re: [PATCH 03/39] mm: consistently validate VMA state after mmap[_prepare] hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Error cleanup path unmaps arbitrary memory if a driver illegally alters vma->vm_start.
- [High] Driver resources and file reference are leaked when mmap_prepare_validate() fails after a successful hook.
--

commit 7871b5bece83247c9fe3efaac4b928710f478390
Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>

mm: consistently validate VMA state after mmap[_prepare] hooks

This commit introduces mmap_prepare_validate() and mmap_hook_validate() to
verify that mmap hooks do not perform unauthorized operations, such as
modifying the VMA start address. It applies these checks uniformly across
VMA creation paths.

> diff --git a/mm/internal.h b/mm/internal.h
> --- a/mm/internal.h
> +++ b/mm/internal.h
[ ... ]
> @@ -238,26 +259,14 @@ static inline int mmap_file(struct file *file, struct vm_area_struct *vma)
>  	 */
>  	if (unlikely(err || !vma->vm_ops))
>  		vma->vm_ops = &vma_dummy_vm_ops;
> +	if (unlikely(err))
> +		return err;
>  
> +	err = mmap_hook_validate(prev_start, &prev_flags, vma);
> +	if (unlikely(err))
> +		vma_close(vma);
>  
> +	return err;
>  }

[Severity: High]
If a driver illegally alters vma->vm_start, mmap_hook_validate() will
correctly catch the violation and return -EINVAL. However, does this leave
the corrupted vma->vm_start in place when returning to the caller?

Looking at __mmap_new_file_vma(), the error handling path relies on
vma->vm_start:

	error = mmap_file(vma->vm_file, vma);
	if (error) {
		UNMAP_STATE(unmap, vmi, vma, vma->vm_start, vma->vm_end,
			    map->prev, map->next);
		...
		unmap_region(&unmap);
		return error;
	}

If mmap_file() fails because the driver altered vma->vm_start, could
unmap_region() accidentally unmap arbitrary user memory using the corrupted
start address?

> diff --git a/mm/util.c b/mm/util.c
> --- a/mm/util.c
> +++ b/mm/util.c
[ ... ]
> @@ -1224,10 +1224,17 @@ EXPORT_SYMBOL(compat_set_desc_from_vma);
>  int __compat_vma_mmap(struct vm_area_desc *desc,
>  		      struct vm_area_struct *vma)
>  {
> +	struct vm_area_desc prev_desc;
>  	int err;
>  
> +	/* Derive state prior to mmap_prepare hook. */
> +	compat_set_desc_from_vma(&prev_desc, desc->file, vma);
>  	/* Perform any preparatory tasks for mmap action. */
>  	err = mmap_action_prepare(desc);
> +	if (err)
> +		return err;
> +	/* Check the caller did nothing crazy. */
> +	err = mmap_prepare_validate(&prev_desc, desc);
>  	if (err)
>  		return err;

[Severity: High]
If mmap_prepare_validate() detects an invariant violation and returns an
error here, does this early return leak driver resources and the file
reference?

If the driver's mmap_prepare() hook succeeded, it may have allocated
resources (expecting them to be freed later via vm_ops->close) or placed
a new file reference in desc->vm_file. Since vma_close() is never called
and the local desc structure is discarded, it appears these resources
might be leaked.

> diff --git a/mm/vma.c b/mm/vma.c
> --- a/mm/vma.c
> +++ b/mm/vma.c
[ ... ]
> @@ -2800,10 +2855,16 @@ static int call_mmap_prepare(struct mmap_state *map,
>  	if (!desc->vm_ops)
>  		return -EINVAL;
>  
> +	/* Perform any preparatory tasks for mmap action. */
>  	err = call_action_prepare(map, desc);
>  	if (err)
>  		return err;
>  
> +	/* Check the caller did nothing crazy. */
> +	err = mmap_prepare_validate(&prev_desc, desc);
> +	if (err)
> +		return err;
> +
>  	/* Update fields permitted to be changed. */
>  	map->pgoff = desc->pgoff;

[Severity: High]
This early return appears to have the same resource leak behavior as
__compat_vma_mmap() above. Would a validation failure here also skip
releasing driver resources or a newly assigned desc->vm_file before the VMA
is fully established?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-b4-mmap-prepare-vma-flag-sanify-v1-0-dacf19cce22b@kernel.org?part=3