MIME-Version: 1.0
References: <20211123191737.1296541-1-tkjos@google.com> <20211123191737.1296541-3-tkjos@google.com>
In-Reply-To: <20211123191737.1296541-3-tkjos@google.com>
From:   Martijn Coenen <maco@android.com>
Date:   Wed, 24 Nov 2021 08:50:37 +0100
Message-ID: <CAB0TPYGHb4CiRZ3VphzWAB2wdU5gL_8WR7t=1ZLQawWivCMKHg@mail.gmail.com>
Subject: Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
To:     Todd Kjos <tkjos@google.com>
Cc:     gregkh@linuxfoundation.org, christian@brauner.io, arve@android.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        maco@google.com, joel@joelfernandes.org, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 23, 2021 at 8:17 PM 'Todd Kjos' via kernel-team
<kernel-team@android.com> wrote:
>
> Since we are no longer going to copy the pre-fixup
> data from the target buffer, we need to read
> pre-translated FD array information from the source
> buffer.
>
> Signed-off-by: Todd Kjos <tkjos@google.com>
Reviewed-by: Martijn Coenen <maco@android.com>

> ---
>  drivers/android/binder.c | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 571d3c203557..2300fa8e09d5 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2234,15 +2234,17 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
>  }
>
>  static int binder_translate_fd_array(struct binder_fd_array_object *fda,
> +                                    const void __user *u,
>                                      struct binder_buffer_object *parent,
> +                                    struct binder_buffer_object *uparent,
>                                      struct binder_transaction *t,
>                                      struct binder_thread *thread,
>                                      struct binder_transaction *in_reply_to)
>  {
>         binder_size_t fdi, fd_buf_size;
>         binder_size_t fda_offset;
> +       const void __user *ufda_base;
>         struct binder_proc *proc = thread->proc;
> -       struct binder_proc *target_proc = t->to_proc;
>
>         fd_buf_size = sizeof(u32) * fda->num_fds;
>         if (fda->num_fds >= SIZE_MAX / sizeof(u32)) {
> @@ -2266,7 +2268,10 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
>          */
>         fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) +
>                 fda->parent_offset;
> -       if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32))) {
> +       ufda_base = (void __user *)uparent->buffer + fda->parent_offset;
> +
> +       if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32)) ||
> +           !IS_ALIGNED((unsigned long)ufda_base, sizeof(u32))) {
>                 binder_user_error("%d:%d parent offset not aligned correctly.\n",
>                                   proc->pid, thread->pid);
>                 return -EINVAL;
> @@ -2275,10 +2280,9 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
>                 u32 fd;
>                 int ret;
>                 binder_size_t offset = fda_offset + fdi * sizeof(fd);
> +               binder_size_t uoffset = fdi * sizeof(fd);
>
> -               ret = binder_alloc_copy_from_buffer(&target_proc->alloc,
> -                                                   &fd, t->buffer,
> -                                                   offset, sizeof(fd));
> +               ret = copy_from_user(&fd, ufda_base + uoffset, sizeof(fd));
>                 if (!ret)
>                         ret = binder_translate_fd(fd, offset, t, thread,
>                                                   in_reply_to);
> @@ -2951,6 +2955,8 @@ static void binder_transaction(struct binder_proc *proc,
>                 case BINDER_TYPE_FDA: {
>                         struct binder_object ptr_object;
>                         binder_size_t parent_offset;
> +                       struct binder_object user_object;
> +                       size_t user_parent_size;
>                         struct binder_fd_array_object *fda =
>                                 to_binder_fd_array_object(hdr);
>                         size_t num_valid = (buffer_offset - off_start_offset) /
> @@ -2982,8 +2988,28 @@ static void binder_transaction(struct binder_proc *proc,
>                                 return_error_line = __LINE__;
>                                 goto err_bad_parent;
>                         }
> -                       ret = binder_translate_fd_array(fda, parent, t, thread,
> -                                                       in_reply_to);
> +
> +                       /*
> +                        * We need to read the user version of the parent
> +                        * object to get the original user offset
> +                        */
> +                       user_parent_size =
> +                               binder_get_object(proc, user_buffer, t->buffer,
> +                                                 parent_offset, &user_object);
> +                       if (user_parent_size != sizeof(user_object.bbo)) {
> +                               binder_user_error("%d:%d invalid ptr object size: %lld vs %lld\n",
> +                                                 proc->pid, thread->pid,
> +                                                 user_parent_size,
> +                                                 sizeof(user_object.bbo));
> +                               return_error = BR_FAILED_REPLY;
> +                               return_error_param = -EINVAL;
> +                               return_error_line = __LINE__;
> +                               goto err_bad_parent;
> +                       }
> +                       ret = binder_translate_fd_array(fda, user_buffer,
> +                                                       parent,
> +                                                       &user_object.bbo, t,
> +                                                       thread, in_reply_to);
>                         if (ret < 0 ||
>                             binder_alloc_copy_to_buffer(&target_proc->alloc,
>                                                         t->buffer,
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>
>