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

[Liam R. Howlett]

* Carlos Llamas <cmllamas@xxxxxxxxxx> [231102 16:09]:

...
> 
> > > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> > > index e3db8297095a..c4d60d81221b 100644
> > > --- a/drivers/android/binder_alloc.c
> > > +++ b/drivers/android/binder_alloc.c
> > > @@ -1005,7 +1005,9 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
> > >  		goto err_mmget;
> > >  	if (!mmap_read_trylock(mm))
> > >  		goto err_mmap_read_lock_failed;
> > > -	vma = binder_alloc_get_vma(alloc);
> > > +	vma = vma_lookup(mm, page_addr);
> > > +	if (vma && vma != binder_alloc_get_vma(alloc))
> > > +		goto err_invalid_vma;
> > 
> > Doesn't this need to be:
> > if (!vma || vma != binder_alloc_get_vma(alloc))
> > 
> > This way, we catch a different vma and a NULL vma.
> > 
> > Or even, just:
> > if (vma != binder_alloc_get_vma(alloc))
> > 
> > if the alloc vma cannot be NULL?
> > 
> 
> If the vma_lookup() is NULL then we still need to isolate and free the
> given binder page and we obviously skip the zap() in this case.

I would have thought if there was no VMA, then the entire process could
be avoided.  Thanks for clarifying.

> 
> However, if we receive a random unexpected vma because of a corrupted
> address or similar, then the whole process is skipped.
> 
> Thus, why we use the check above.
> 
> --
> Carlos Llamas