Re: KASAN: null-ptr-deref Write in binder_update_page_range

[Martijn Coenen <maco@android.com>]

Thanks Minchan!

On Thu, Aug 23, 2018 at 7:29 AM, Minchan Kim <minchan@kernel.org> wrote:
> Signed-off-by: Todd Kjos <tkjos@google.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Reviewed-by: Martijn Coenen <maco@android.com>

> ---
>  drivers/android/binder_alloc.c | 43 +++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 3f3b7b253445..64fd96eada31 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -332,6 +332,35 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
>         return vma ? -ENOMEM : -ESRCH;
>  }
>
> +
> +static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
> +               struct vm_area_struct *vma)
> +{
> +       if (vma)
> +               alloc->vma_vm_mm = vma->vm_mm;
> +       /*
> +        * If we see alloc->vma is not NULL, buffer data structures set up
> +        * completely. Look at smp_rmb side binder_alloc_get_vma.
> +        * We also want to guarantee new alloc->vma_vm_mm is always visible
> +        * if alloc->vma is set.
> +        */
> +       smp_wmb();
> +       alloc->vma = vma;
> +}
> +
> +static inline struct vm_area_struct *binder_alloc_get_vma(
> +               struct binder_alloc *alloc)
> +{
> +       struct vm_area_struct *vma = NULL;
> +
> +       if (alloc->vma) {
> +               /* Look at description in binder_alloc_set_vma */
> +               smp_rmb();
> +               vma = alloc->vma;
> +       }
> +       return vma;
> +}
> +
>  static struct binder_buffer *binder_alloc_new_buf_locked(
>                                 struct binder_alloc *alloc,
>                                 size_t data_size,
> @@ -348,7 +377,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
>         size_t size, data_offsets_size;
>         int ret;
>
> -       if (alloc->vma == NULL) {
> +       if (!binder_alloc_get_vma(alloc)) {
>                 binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
>                                    "%d: binder_alloc_buf, no vma\n",
>                                    alloc->pid);
> @@ -723,9 +752,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
>         buffer->free = 1;
>         binder_insert_free_buffer(alloc, buffer);
>         alloc->free_async_space = alloc->buffer_size / 2;
> -       barrier();
> -       alloc->vma = vma;
> -       alloc->vma_vm_mm = vma->vm_mm;
> +       binder_alloc_set_vma(alloc, vma);
>         mmgrab(alloc->vma_vm_mm);
>
>         return 0;
> @@ -754,10 +781,10 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
>         int buffers, page_count;
>         struct binder_buffer *buffer;
>
> -       BUG_ON(alloc->vma);
> -
>         buffers = 0;
>         mutex_lock(&alloc->mutex);
> +       BUG_ON(alloc->vma);
> +
>         while ((n = rb_first(&alloc->allocated_buffers))) {
>                 buffer = rb_entry(n, struct binder_buffer, rb_node);
>
> @@ -900,7 +927,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc)
>   */
>  void binder_alloc_vma_close(struct binder_alloc *alloc)
>  {
> -       WRITE_ONCE(alloc->vma, NULL);
> +       binder_alloc_set_vma(alloc, NULL);
>  }
>
>  /**
> @@ -935,7 +962,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
>
>         index = page - alloc->pages;
>         page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE;
> -       vma = alloc->vma;
> +       vma = binder_alloc_get_vma(alloc);
>         if (vma) {
>                 if (!mmget_not_zero(alloc->vma_vm_mm))
>                         goto err_mmget;
> --
> 2.18.0.1017.ga543ac7ca45-goog
>
>
>
>