Re: [PATCH 02/21] binder: fix use-after-free in shinker's callback

[Alice Ryhl]

Carlos Llamas <cmllamas@xxxxxxxxxx> writes:
> The mmap read lock is used during the shrinker's callback, which means
> that using alloc->vma pointer isn't safe as it can race with munmap().
> As of commit dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in
> munmap") the mmap lock is downgraded after the vma has been isolated.
> 
> I was able to reproduce this issue by manually adding some delays and
> triggering page reclaiming through the shrinker's debug sysfs. The
> following KASAN report confirms the UAF:
> 
>   [...snip...]
> 
> Fix this issue by performing instead a vma_lookup() which will fail to
> find the vma that was isolated before the mmap lock downgrade. Note that
> this option has better performance than upgrading to a mmap write lock
> which would increase contention. Plus, mmap_write_trylock() has been
> recently removed anyway.
> 
> Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Liam Howlett <liam.howlett@xxxxxxxxxx>
> Cc: Minchan Kim <minchan@xxxxxxxxxx>
> Signed-off-by: Carlos Llamas <cmllamas@xxxxxxxxxx>

This change makes sense to me, and I agree that the code still needs to
run when the vma is null.

Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>