Re: [PATCH 01/10] ANDROID: binder: Add strong ref checks

[Martijn Coenen]

On Mon, Oct 24, 2016 at 3:20 PM, Martijn Coenen <maco@xxxxxxxxxxx> wrote:
> From: Arve HjÃnnevÃg <arve@xxxxxxxxxxx>
>
> Prevent using a binder_ref with only weak references where a strong
> reference is required.
>
> Signed-off-by: Arve HjÃnnevÃg <arve@xxxxxxxxxxx>

Signed-off-by: Martijn Coenen <maco@xxxxxxxxxxx>

> ---
>  drivers/android/binder.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 562af94..3681759 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1002,7 +1002,7 @@ static int binder_dec_node(struct binder_node *node, int strong, int internal)
>
>
>  static struct binder_ref *binder_get_ref(struct binder_proc *proc,
> -                                        uint32_t desc)
> +                                        u32 desc, bool need_strong_ref)
>  {
>         struct rb_node *n = proc->refs_by_desc.rb_node;
>         struct binder_ref *ref;
> @@ -1010,12 +1010,16 @@ static struct binder_ref *binder_get_ref(struct binder_proc *proc,
>         while (n) {
>                 ref = rb_entry(n, struct binder_ref, rb_node_desc);
>
> -               if (desc < ref->desc)
> +               if (desc < ref->desc) {
>                         n = n->rb_left;
> -               else if (desc > ref->desc)
> +               } else if (desc > ref->desc) {
>                         n = n->rb_right;
> -               else
> +               } else if (need_strong_ref && !ref->strong) {
> +                       binder_user_error("tried to use weak ref as strong ref\n");
> +                       return NULL;
> +               } else {
>                         return ref;
> +               }
>         }
>         return NULL;
>  }
> @@ -1285,7 +1289,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
>                 } break;
>                 case BINDER_TYPE_HANDLE:
>                 case BINDER_TYPE_WEAK_HANDLE: {
> -                       struct binder_ref *ref = binder_get_ref(proc, fp->handle);
> +                       struct binder_ref *ref;
> +
> +                       ref = binder_get_ref(proc, fp->handle,
> +                                            fp->type == BINDER_TYPE_HANDLE);
>
>                         if (ref == NULL) {
>                                 pr_err("transaction release %d bad handle %d\n",
> @@ -1380,7 +1387,7 @@ static void binder_transaction(struct binder_proc *proc,
>                 if (tr->target.handle) {
>                         struct binder_ref *ref;
>
> -                       ref = binder_get_ref(proc, tr->target.handle);
> +                       ref = binder_get_ref(proc, tr->target.handle, true);
>                         if (ref == NULL) {
>                                 binder_user_error("%d:%d got transaction to invalid handle\n",
>                                         proc->pid, thread->pid);
> @@ -1589,7 +1596,10 @@ static void binder_transaction(struct binder_proc *proc,
>                 } break;
>                 case BINDER_TYPE_HANDLE:
>                 case BINDER_TYPE_WEAK_HANDLE: {
> -                       struct binder_ref *ref = binder_get_ref(proc, fp->handle);
> +                       struct binder_ref *ref;
> +
> +                       ref = binder_get_ref(proc, fp->handle,
> +                                            fp->type == BINDER_TYPE_HANDLE);
>
>                         if (ref == NULL) {
>                                 binder_user_error("%d:%d got transaction with invalid handle, %d\n",
> @@ -1800,7 +1810,9 @@ static int binder_thread_write(struct binder_proc *proc,
>                                                 ref->desc);
>                                 }
>                         } else
> -                               ref = binder_get_ref(proc, target);
> +                               ref = binder_get_ref(proc, target,
> +                                                    cmd == BC_ACQUIRE ||
> +                                                    cmd == BC_RELEASE);
>                         if (ref == NULL) {
>                                 binder_user_error("%d:%d refcount change on invalid ref %d\n",
>                                         proc->pid, thread->pid, target);
> @@ -1996,7 +2008,7 @@ static int binder_thread_write(struct binder_proc *proc,
>                         if (get_user(cookie, (binder_uintptr_t __user *)ptr))
>                                 return -EFAULT;
>                         ptr += sizeof(binder_uintptr_t);
> -                       ref = binder_get_ref(proc, target);
> +                       ref = binder_get_ref(proc, target, false);
>                         if (ref == NULL) {
>                                 binder_user_error("%d:%d %s invalid ref %d\n",
>                                         proc->pid, thread->pid,
> --
> 2.8.0.rc3.226.g39d4020
>