Re: [PATCH hotfix 6.12 1/8] mm: avoid unsafe VMA hook invocation when error arises on mmap hook

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [241022 16:41]:
> After an attempted mmap() fails, we are no longer in a situation where we
> can safely interact with VMA hooks. This is currently not enforced, meaning
> that we need complicated handling to ensure we do not incorrectly call
> these hooks.
> 
> We can avoid the whole issue by treating the VMA as suspect the moment that
> the file->f_ops->mmap() function reports an error by replacing whatever VMA
> operations were installed with a dummy empty set of VMA operations.
> 
> We do so through a new helper function internal to mm - mmap_file() - which
> is both more logically named than the existing call_mmap() function and
> correctly isolates handling of the vm_op reassignment to mm.
> 
> All the existing invocations of call_mmap() outside of mm are ultimately
> nested within the call_mmap() from mm, which we now replace.
> 
> It is therefore safe to leave call_mmap() in place as a convenience
> function (and to avoid churn). The invokers are:
> 
>      ovl_file_operations -> mmap -> ovl_mmap() -> backing_file_mmap()
>     coda_file_operations -> mmap -> coda_file_mmap()
>      shm_file_operations -> shm_mmap()
> shm_file_operations_huge -> shm_mmap()
>             dma_buf_fops -> dma_buf_mmap_internal -> i915_dmabuf_ops
> 	                    -> i915_gem_dmabuf_mmap()
> 
> None of these callers interact with vm_ops or mappings in a problematic way
> on error, quickly exiting out.
> 
> Reported-by: Jann Horn <jannh@google.com>
> Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  mm/internal.h | 27 +++++++++++++++++++++++++++
>  mm/mmap.c     |  6 +++---
>  mm/nommu.c    |  4 ++--
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 508f7802dd2b..af032e76dfd4 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -108,6 +108,33 @@ static inline void *folio_raw_mapping(const struct folio *folio)
>  	return (void *)(mapping & ~PAGE_MAPPING_FLAGS);
>  }
> 
> +/*
> + * This is a file-backed mapping, and is about to be memory mapped - invoke its
> + * mmap hook and safely handle error conditions. On error, VMA hooks will be
> + * mutated.
> + *
> + * @file: File which backs the mapping.
> + * @vma:  VMA which we are mapping.
> + *
> + * Returns: 0 if success, error otherwise.
> + */
> +static inline int mmap_file(struct file *file, struct vm_area_struct *vma)
> +{
> +	int err = call_mmap(file, vma);
> +
> +	if (likely(!err))
> +		return 0;
> +
> +	/*
> +	 * OK, we tried to call the file hook for mmap(), but an error
> +	 * arose. The mapping is in an inconsistent state and we most not invoke
> +	 * any further hooks on it.
> +	 */
> +	vma->vm_ops = &vma_dummy_vm_ops;
> +
> +	return err;
> +}
> +
>  #ifdef CONFIG_MMU
> 
>  /* Flags for folio_pte_batch(). */
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 1ba0878bbc30..10f4ccaf491b 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1425,7 +1425,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	/*
>  	 * clear PTEs while the vma is still in the tree so that rmap
>  	 * cannot race with the freeing later in the truncate scenario.
> -	 * This is also needed for call_mmap(), which is why vm_ops
> +	 * This is also needed for mmap_file(), which is why vm_ops
>  	 * close function is called.
>  	 */
>  	vms_clean_up_area(&vms, &mas_detach);
> @@ -1450,7 +1450,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> 
>  	if (file) {
>  		vma->vm_file = get_file(file);
> -		error = call_mmap(file, vma);
> +		error = mmap_file(file, vma);
>  		if (error)
>  			goto unmap_and_free_vma;
> 
> @@ -1473,7 +1473,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> 
>  		vma_iter_config(&vmi, addr, end);
>  		/*
> -		 * If vm_flags changed after call_mmap(), we should try merge
> +		 * If vm_flags changed after mmap_file(), we should try merge
>  		 * vma again as we may succeed this time.
>  		 */
>  		if (unlikely(vm_flags != vma->vm_flags && vmg.prev)) {
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 385b0c15add8..f9ccc02458ec 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -885,7 +885,7 @@ static int do_mmap_shared_file(struct vm_area_struct *vma)
>  {
>  	int ret;
> 
> -	ret = call_mmap(vma->vm_file, vma);
> +	ret = mmap_file(vma->vm_file, vma);
>  	if (ret == 0) {
>  		vma->vm_region->vm_top = vma->vm_region->vm_end;
>  		return 0;
> @@ -918,7 +918,7 @@ static int do_mmap_private(struct vm_area_struct *vma,
>  	 * happy.
>  	 */
>  	if (capabilities & NOMMU_MAP_DIRECT) {
> -		ret = call_mmap(vma->vm_file, vma);
> +		ret = mmap_file(vma->vm_file, vma);
>  		/* shouldn't return success if we're not sharing */
>  		if (WARN_ON_ONCE(!is_nommu_shared_mapping(vma->vm_flags)))
>  			ret = -ENOSYS;
> --
> 2.47.0