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

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

[Carlos Llamas]

Hi,

I've received several reports of a mmap_assert_locked() BUG triggering
at binder_alloc_vma_close() in the v5.15 stable tree. This makes sense
as the following two commits were recently picked for stable:

a43cfc87caaf ("android: binder: stop saving a pointer to the VMA")
b0cab80ecd54 ("android: binder: fix lockdep check on clearing vma")

In such commits mmap_asserts are added to binder_alloc_set_vma() which
is called back from vma->vm_ops->close() and file->f_op->mmap().

However, mmap_locking was only added to the exit_mmap() path in commit
f798a1d4f94d ("mm: fix use-after-free bug when mm->mmap is reused after
being freed") and since this patch doesn't exist in v5.15 stable tree
undoubtedly it leads to the following BUG:

	kernel BUG at include/linux/mmap_lock.h:156!
	Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
	[...]
	Call trace:
	binder_alloc_vma_close+0x14c/0x150
	binder_vma_close+0xa4/0x184
	remove_vma+0xa4/0x108
	exit_mmap+0x320/0x424
	__mmput+0xb4/0x2b4
	mmput+0x8c/0xb0
	exit_mm+0x51c/0x6c8
	do_exit+0x488/0x1bf4
	do_group_exit+0x118/0x270
	[...]

Note that I have removed such asserts from the binder driver here:
https://lore.kernel.org/all/20220829201254.1814484-5-cmllamas@google.com/
as a random driver didn't seem to me like the appropriate place to
validate the mm stack locking. Since this patch is not going to the
stable trees, the asserts still exist in v5.15.

So, what is the proper fix for this v5.15 BUG? Should f798a1d4f94d
be picked for v5.15 stable? Or should binder be fixed instead?

Thanks,
--
Carlos Llamas

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

[Suren Baghdasaryan]

On Thu, Sep 8, 2022 at 1:28 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> Hi,
>
> I've received several reports of a mmap_assert_locked() BUG triggering
> at binder_alloc_vma_close() in the v5.15 stable tree. This makes sense
> as the following two commits were recently picked for stable:
>
> a43cfc87caaf ("android: binder: stop saving a pointer to the VMA")
> b0cab80ecd54 ("android: binder: fix lockdep check on clearing vma")
>
> In such commits mmap_asserts are added to binder_alloc_set_vma() which
> is called back from vma->vm_ops->close() and file->f_op->mmap().
>
> However, mmap_locking was only added to the exit_mmap() path in commit
> f798a1d4f94d ("mm: fix use-after-free bug when mm->mmap is reused after
> being freed") and since this patch doesn't exist in v5.15 stable tree
> undoubtedly it leads to the following BUG:
>
>         kernel BUG at include/linux/mmap_lock.h:156!
>         Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>         [...]
>         Call trace:
>         binder_alloc_vma_close+0x14c/0x150
>         binder_vma_close+0xa4/0x184
>         remove_vma+0xa4/0x108
>         exit_mmap+0x320/0x424
>         __mmput+0xb4/0x2b4
>         mmput+0x8c/0xb0
>         exit_mm+0x51c/0x6c8
>         do_exit+0x488/0x1bf4
>         do_group_exit+0x118/0x270
>         [...]
>
> Note that I have removed such asserts from the binder driver here:
> https://lore.kernel.org/all/20220829201254.1814484-5-cmllamas@google.com/
> as a random driver didn't seem to me like the appropriate place to
> validate the mm stack locking. Since this patch is not going to the
> stable trees, the asserts still exist in v5.15.
>
> So, what is the proper fix for this v5.15 BUG? Should f798a1d4f94d
> be picked for v5.15 stable? Or should binder be fixed instead?

IIUC, the binder patches are backported to 5.15 kernel and they expect
mmap_lock to be held during remove_vma() operation in exit_mmap().
However in 5.15 kernel that assumption is incorrect. In that case IMHO
the backport needs to drop that invalid expectation.
Thanks,
Suren.

>
> Thanks,
> --
> Carlos Llamas

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

[Carlos Llamas]

On Thu, Sep 08, 2022 at 03:33:52PM -0700, Suren Baghdasaryan wrote:
> > However, mmap_locking was only added to the exit_mmap() path in commit
> > f798a1d4f94d ("mm: fix use-after-free bug when mm->mmap is reused after
> > being freed") and since this patch doesn't exist in v5.15 stable tree

Sorry, I meant 64591e8605d6 ("mm: protect free_pgtables with mmap_lock
write lock in exit_mmap") here, that's when protection was added.

> IIUC, the binder patches are backported to 5.15 kernel and they expect
> mmap_lock to be held during remove_vma() operation in exit_mmap().

Correct!

> However in 5.15 kernel that assumption is incorrect. In that case IMHO
> the backport needs to drop that invalid expectation.

Does this mean that users of async calls such as find_vma() can't rely
on mmap_lock to avoid racing with remove_vma()? I see the following
pattern is used quite often:

	mmap_read_lock(mm);
	vma = find_vma(mm, addr);
	[...]
	mmap_read_unlock(mm);

Is this not a real concern? I'd drop the asserts from binder and call it
a day. However, we would also need to fix our race with vm_ops->close().

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

[Suren Baghdasaryan]

On Fri, Sep 9, 2022 at 12:35 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Thu, Sep 08, 2022 at 03:33:52PM -0700, Suren Baghdasaryan wrote:
> > > However, mmap_locking was only added to the exit_mmap() path in commit
> > > f798a1d4f94d ("mm: fix use-after-free bug when mm->mmap is reused after
> > > being freed") and since this patch doesn't exist in v5.15 stable tree
>
> Sorry, I meant 64591e8605d6 ("mm: protect free_pgtables with mmap_lock
> write lock in exit_mmap") here, that's when protection was added.
>
> > IIUC, the binder patches are backported to 5.15 kernel and they expect
> > mmap_lock to be held during remove_vma() operation in exit_mmap().
>
> Correct!
>
> > However in 5.15 kernel that assumption is incorrect. In that case IMHO
> > the backport needs to drop that invalid expectation.
>
> Does this mean that users of async calls such as find_vma() can't rely
> on mmap_lock to avoid racing with remove_vma()? I see the following
> pattern is used quite often:
>
>         mmap_read_lock(mm);
>         vma = find_vma(mm, addr);
>         [...]
>         mmap_read_unlock(mm);
>
> Is this not a real concern? I'd drop the asserts from binder and call it
> a day. However, we would also need to fix our race with vm_ops->close().

I think by the time exit_mmap() calls remove_vma() there can be no
other user of that mm to race with, even oom-reaper would have
finished by then (see:
https://elixir.bootlin.com/linux/v5.15.67/source/mm/mmap.c#L3157).
So, generally remove_vma() would be done under mmap_lock write
protection but in case of exit_mmap() that's not necessary. Michal,
please correct me if I'm wrong.

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

[Carlos Llamas]

On Fri, Sep 09, 2022 at 01:03:08PM -0700, Suren Baghdasaryan wrote:
> On Fri, Sep 9, 2022 at 12:35 PM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > Does this mean that users of async calls such as find_vma() can't rely
> > on mmap_lock to avoid racing with remove_vma()? I see the following
> > pattern is used quite often:
> >
> >         mmap_read_lock(mm);
> >         vma = find_vma(mm, addr);
> >         [...]
> >         mmap_read_unlock(mm);
> >
> > Is this not a real concern? I'd drop the asserts from binder and call it
> > a day. However, we would also need to fix our race with vm_ops->close().
> 
> I think by the time exit_mmap() calls remove_vma() there can be no
> other user of that mm to race with, even oom-reaper would have
> finished by then (see:
> https://elixir.bootlin.com/linux/v5.15.67/source/mm/mmap.c#L3157).
> So, generally remove_vma() would be done under mmap_lock write
> protection but in case of exit_mmap() that's not necessary. Michal,
> please correct me if I'm wrong.

I see, that makes more sense.

Then it sounds to me like binder should be using mmget_not_zero() to
serialize against exit_mmap() during these async calls. I'll have a
closer look at this change.

Also, we should drop the mmap_lock asserts in binder from v5.15 as the
expectations there are incorrect. Again, this was done in [1], but for
different reasons. We could simply amend a small note to the commit log
with an accurate reason for the backport.

Liam, wdyt?

[1] https://lore.kernel.org/all/20220829201254.1814484-5-cmllamas@google.com/

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

[Liam Howlett]

* Carlos Llamas <cmllamas@google.com> [220912 15:11]:
> On Fri, Sep 09, 2022 at 01:03:08PM -0700, Suren Baghdasaryan wrote:
> > On Fri, Sep 9, 2022 at 12:35 PM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > Does this mean that users of async calls such as find_vma() can't rely
> > > on mmap_lock to avoid racing with remove_vma()? I see the following
> > > pattern is used quite often:
> > >
> > >         mmap_read_lock(mm);
> > >         vma = find_vma(mm, addr);
> > >         [...]
> > >         mmap_read_unlock(mm);
> > >
> > > Is this not a real concern? I'd drop the asserts from binder and call it
> > > a day. However, we would also need to fix our race with vm_ops->close().
> > 
> > I think by the time exit_mmap() calls remove_vma() there can be no
> > other user of that mm to race with, even oom-reaper would have
> > finished by then (see:
> > https://elixir.bootlin.com/linux/v5.15.67/source/mm/mmap.c#L3157).
> > So, generally remove_vma() would be done under mmap_lock write
> > protection but in case of exit_mmap() that's not necessary. Michal,
> > please correct me if I'm wrong.
> 
> I see, that makes more sense.
> 
> Then it sounds to me like binder should be using mmget_not_zero() to
> serialize against exit_mmap() during these async calls. I'll have a
> closer look at this change.
> 
> Also, we should drop the mmap_lock asserts in binder from v5.15 as the
> expectations there are incorrect. Again, this was done in [1], but for
> different reasons. We could simply amend a small note to the commit log
> with an accurate reason for the backport.
> 
> Liam, wdyt?

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?

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.


> 
> [1] https://lore.kernel.org/all/20220829201254.1814484-5-cmllamas@google.com/

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

[Carlos Llamas]

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()? 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 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

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

[Suren Baghdasaryan]

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

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

[Carlos Llamas]

On Wed, Sep 28, 2022 at 04:21:27PM -0700, Suren Baghdasaryan wrote:
> > 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.

I agree ->mmap() should undo its own changes if it fails. However, the
scenario I'm referring to is an error _after_ ->mmap() call succeeds.
In this case it can be the arch_validate_flags() check. On error, the
vma is torn down and the drivers are never informed about 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()?

My understanding is ->mmap() is called on the very first mapping and
vm_ops->close() is subsequently called whenever a mapping is removed.
So IIUC a vm_ops->open() is not required in this exchange.

There are multiple places where I can see rely on this behavior. Just to
provide an example, look at coda_file_mmap() which allocates cvm_ops and
increments references on ->mmap(). It then expects coda_vm_close() to be
called when the vma is removed to decrement these references and lastly
free cvm_ops. However, if mmap_region() fails after ->mmap() call then
vm_ops->close() is never invoked and the reference count in coda is now
off making cvm_ops leaked memory.

Other type of issues happen depending on the ->mmap() implementation. In
our case it was a BUG_ON() in binder.

I don't know if adding vm_ops->close() to the exit error path is an
appropriate solution. Perhaps ->mmap() should be a point of no return
instead and vm_flags should be validated earlier? I would be concerned
about drivers modifying the vm_flags during ->mmap() though. I'll send
out a patch to get more feedback on this vm_ops->close().

--
Carlos Llamas