Re: [PATCH v2] fork: avoid inappropriate uprobe access to invalid mm

[David Hildenbrand]

On 10.12.24 18:24, Lorenzo Stoakes wrote:
> If dup_mmap() encounters an issue, currently uprobe is able to access the
> relevant mm via the reverse mapping (in build_map_info()), and if we are
> very unlucky with a race window, observe invalid XA_ZERO_ENTRY state which
> we establish as part of the fork error path.
>
> This occurs because uprobe_write_opcode() invokes anon_vma_prepare() which
> in turn invokes find_mergeable_anon_vma() that uses a VMA iterator,
> invoking vma_iter_load() which uses the advanced maple tree API and thus is
> able to observe XA_ZERO_ENTRY entries added to dup_mmap() in commit
> d24062914837 ("fork: use __mt_dup() to duplicate maple tree in
> dup_mmap()").
>
> This change was made on the assumption that only process tear-down code
> would actually observe (and make use of) these values. However this very
> unlikely but still possible edge case with uprobes exists and unfortunately
> does make these observable.
>
> The uprobe operation prevents races against the dup_mmap() operation via
> the dup_mmap_sem semaphore, which is acquired via uprobe_start_dup_mmap()
> and dropped via uprobe_end_dup_mmap(), and held across
> register_for_each_vma() prior to invoking build_map_info() which does the
> reverse mapping lookup.
>
> Currently these are acquired and dropped within dup_mmap(), which exposes
> the race window prior to error handling in the invoking dup_mm() which
> tears down the mm.
>
> We can avoid all this by just moving the invocation of
> uprobe_start_dup_mmap() and uprobe_end_dup_mmap() up a level to dup_mm()
> and only release this lock once the dup_mmap() operation succeeds or clean
> up is done.

What I understand is: we need to perform the uprobe_end_dup_mmap() after 
the mmput().

I assume/hope that we cannot see another mmget() before we return here. 
In that case, this LGTM.

--
Cheers,

David / dhildenb