Re: [Devel] [PATCH hci-8.1] mm: allow MADV_DOEXEC on shared PFN mappings of device memory

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: MADV_DOEXEC dups any shared PFNMAP VMA, sharing vm_private_data across mms
- [Medium] mm: vma_dup() links pfnmap VMA into i_mmap before PTE copy, so zap can miss them
- [Low] mm: vma_dup() keeps inherited VM_PAT on PFNMAP copy, unlike dup_mmap()
--

commit 2e5fc626303801168dc124d9f0c079510fb5d4fd
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

mm: allow MADV_DOEXEC on shared PFN mappings of device memory

This patch lets MADV_DOEXEC also preserve shared VM_PFNMAP file mappings
such as VFIO BARs. vma_dup() duplicates them the way fork does for a shared
file mapping: it takes a file reference, links the VMA into i_mmap, calls
vm_ops->open() and copies the PTEs.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-103860

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1018,6 +1018,18 @@ static inline bool vma_is_anonymous(struct vm_area_struct *vma)
>  	return !vma->vm_ops;
>  }
>  
> +/*
> + * Shared mapping of memory without struct pages, such as a device BAR
> + * mapped through VFIO.  Such a VMA may still have no vm_ops (when the
> + * driver populates it with remap_pfn_range() at mmap time), so it must
> + * be tested before vma_is_anonymous().
> + */
> +static inline bool vma_is_shared_pfnmap(struct vm_area_struct *vma)
> +{
> +	return vma->vm_file &&
> +	       (vma->vm_flags & (VM_SHARED | VM_PFNMAP)) == (VM_SHARED | VM_PFNMAP);
> +}
> +

[Severity: High]
The eligibility test here and in madvise_vma_behavior() is purely
structural: a file plus VM_SHARED|VM_PFNMAP. There is no driver opt-in, and
VM_DONTCOPY and VM_IO are not excluded. Can this let unprivileged userspace
opt any driver PFN mapping into exec-time duplication?

vma_dup() copies the VMA with vm_area_dup(), so vm_private_data is shared
verbatim between the old and new VMA. Only vm_ops->open() is called, never
->mmap(). Drivers that free per-mapping state in ->close() without a
refcounting ->open() then get two VMAs and two ->close() calls.

For example, gru_file_mmap() sets VM_IO|VM_PFNMAP|VM_DONTCOPY and stores
vdata in vm_private_data. gru_vma_close() does:

    vdata = vma->vm_private_data;
    vma->vm_private_data = NULL;
    ...
    kfree(vdata);

There is no ->open(). The duplicated VMA keeps the stale vdata pointer, so
can its later close double free vdata?

genwqe_mmap() uses remap_pfn_range(), and genwqe_vma_open() is an empty
stub. genwqe_vma_close() runs at exec teardown of the old mm:

    __genwqe_del_mapping(cfile, dma_map);
    __genwqe_free_consistent(cd, dma_map->size, dma_map->k_vaddr,
                             dma_map->dma_addr);
    kfree(dma_map);

Can this free the coherent DMA buffer while the duplicated PFNMAP VMA in the
new mm still maps it, leaving userspace with access to freed memory?

uacce sets VM_DONTCOPY|VM_WIPEONFORK and kfree()s its qfr on close. Its
->mmap() uniqueness check is bypassed by the duplication, so can the
hardware mapping outlive its qfr?

Drivers set VM_DONTCOPY to say the VMA should not be duplicated.
MADV_DOFORK rejects VM_IO. Should this path require an explicit driver
opt-in, or at least reject VM_DONTCOPY and VM_IO VMAs, or require ->open()
to be defined?

> diff --git a/mm/mmap.c b/mm/mmap.c
> --- a/mm/mmap.c
> +++ b/mm/mmap.c

[ ... ]

>  int vma_dup(struct vm_area_struct *old_vma, struct mm_struct *mm)
>  {
>  	struct vm_area_struct *vma;
>  	unsigned long npages;
> +	bool pfnmap = vma_is_shared_pfnmap(old_vma);
>  	int ret = -ENOMEM;

[ ... ]

>  	vma->vm_mm = mm;
> -	ret = anon_vma_fork(vma, old_vma);
> -	if (ret)
> -		goto fail_nomem_anon_vma_fork;
> +	if (pfnmap) {
> +		get_file(vma->vm_file);
> +	} else {

[Severity: Low]
In dup_mmap(), a VM_PFNMAP VMA gets this before copy_page_range():

    /* track_pfn_copy() will later take care of copying internal state. */
    if (unlikely(tmp->vm_flags & VM_PFNMAP))
            untrack_pfn_clear(tmp);

The pfnmap branch here has no equivalent, so the vm_area_dup() copy keeps
the inherited VM_PAT flag without owning a reservation. Can this leave an
inconsistent PAT state on the destination VMA?

Consider copy_page_range() failing in track_pfn_copy() (in get_pat_info()
or reserve_pfn_range()) before it reaches vm_flags_set(dst_vma, VM_PAT).
The exec then fails and the new mm is torn down, and unmap_single_vma()
calls untrack_pfn() on a VMA that still has the inherited VM_PAT.
get_pat_info() runs on empty destination page tables. Can this hit
WARN_ON_ONCE(1) on the exec error path, and trigger a panic with
panic_on_warn? Should vma_dup() call untrack_pfn_clear() on the new VMA
before copy_page_range() as dup_mmap() does?

[ ... ]

> +	/* Also links a file VMA into the file's i_mmap, for unmap_mapping_range() */
>  	ret = vma_link(mm, vma);
>  	if (ret)
>  		goto fail_nomem_vma_link;
>  
> +	if (pfnmap && vma->vm_ops && vma->vm_ops->open)
> +		vma->vm_ops->open(vma);
> +

[ ... ]

> +	/* For PFN maps this copies the device PTEs and the PAT memtype */
> +	if (pfnmap)
> +		ret = copy_page_range(vma, old_vma);
> +	else

[Severity: Medium]
vma_link() makes the new VMA visible in i_mmap via vma_link_file(), which
takes i_mmap_rwsem for write only around the insert. copy_page_range() then
runs with no i_mmap_rwsem held. Can unmap_mapping_range() miss the copied
PTEs?

unmap_mapping_pages() takes only i_mmap_rwsem for read and no mmap_lock.
One possible interleaving:

    exec thread                        unmap_mapping_range()
    vma_link()  (new VMA inserted)
                                       walks i_mmap, zaps empty new VMA
    copy_page_range()  (copies PTEs
      from old_vma, not yet zapped)
                                       zaps old_vma only

The new mm would then keep live device MMIO PTEs after the revoke. fork()
avoids this with:

    vma_interval_tree_insert_after(tmp, mpnt, &mapping->i_mmap);

so the walker sees the parent before the child. vma_link_file() uses a
plain insert with no such ordering. Is there something that closes this
window, such as keeping i_mmap_rwsem held across the PTE copy or ordering
the new VMA after old_vma?

>  #ifdef CONFIG_PADATA
> -	ret = copy_page_range_mt(vma, old_vma);
> +		ret = copy_page_range_mt(vma, old_vma);
>  #else
> -	ret = copy_page_range(vma, old_vma);
> +		ret = copy_page_range(vma, old_vma);
>  #endif

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009143227.492150-1-andrey.drobyshev@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel