Re: BUG in binder_vma_close() at mmap_assert_locked() in stable v5.15

[Suren Baghdasaryan <surenb@google.com>]

On Fri, Sep 23, 2022 at 1:50 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Tue, Sep 13, 2022 at 06:36:33AM +0000, Liam Howlett wrote:
> >
> > It sounds like the binder_alloc vma_vm_mm is being used unsafely as
> > well?  I'd actually go the other way with this and try to add more
> > validation that are optimized out on production builds.  Since binder is
> > saving a pointer to the mm struct and was saving the vma ponter, we
> > should be very careful around how we use them. Is the mutex in
> > binder_alloc protection enough for the vma binder buffers uses?  How is
> > the close() not being called before the exit_mmap() path?
>
> The alloc->mutex is top-level so it can't be used under vm_ops or we
> risk a possible deadlock with mmap_lock unfortunately.
>
> >
> > When you look at the mmget_not_zero() stuff, have a look at
> > binder_alloc_new_buf_locked().  I think it is unsafely using the
> > vma_vm_mm pointer without calling mmget_not_zero(), but the calling
> > function is rather large so I'm not sure.
>
> We had used mm safely in places like binder_update_page_range() but not
> so in the recent changes to switch over to vma_lookup(). It seems this
> can be an issue if ->release() races with binder_alloc_print_allocated()
> so I'll have a closer look at this.
>
>
> So a fix for the initial BUG concern has landed in v5.15.70:
> https://git.kernel.org/stable/c/899f4160b140
>
> However, after doing a deeper investigation it seems there is still an
> underlying problem. This requires a bit of context so please bear with
> me while I try to explain.
>
> It started with the maple tree patchset in linux-next which added a
> late allocation in mmap_region() in commit ("mm: start tracking VMAs
> with maple tree"). Syzbot failed this allocation and so mmap_region()
> unwinds, munmaps and frees the vma. This error path makes the cached
> alloc->vma in binder a dangling pointer.
>
> Liam explains the scenario here:
> https://lore.kernel.org/all/20220621020814.sjszxp3uz43rka62@revolver/
>
> Also, Liam correctly points out that is safer to lookup the vma instead
> of caching a pointer to it. Such change is what eventually is proposed
> as the fix to the fuzzer report.
>
> However, I wonder why isn't ->mmap() being undone for this exit path in
> mmap_region()? If anything fails _after_ call_mmap() it seems we
> silently unmap and free the vma. What about allocations, atomic_incs,
> and anything done inside ->mmap()?

I think if ->mmap() fails it is expected to undo its own changes
before returning the error. mmap_region() has no idea what kind of
changes ->mmap() has done before it failed, therefore it can't undo
them. At least that's how I understand it.

> Shouldn't a ->close() be needed here
> to undo these things as such:
> --
> @@ -1872,6 +1889,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>
>         return addr;
>
> +undo_mmap:
> +       if (vma->vm_ops && vma->vm_ops->close)
> +               vma->vm_ops->close(vma);
>  unmap_and_free_vma:
>         fput(vma->vm_file);
>         vma->vm_file = NULL;
> --
>

I don't see mmap_region() calling vma->vm_ops->open() anywhere. So why
would it have to call vma->vm_ops->close()?

> I managed to reproduce the same syzbot issue in v5.15.41 by failing the
> arch_validate_flags() check by simply passing PROT_MTE flag to mmap().
> I ran this in a "qemu-system-aarch64 -M virt,mte=on" system.
>
> Am I missing something? It looks scary to me all the memleaks, corrupt
> ref counts, etc. that could follow from this simple path.
>
> --
> Carlos Llamas