LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Martijn Coenen <maco@android.com>
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
Subject: Re: [PATCH 1/3] binder: avoid potential data leakage when copying txn
Date: Wed, 24 Nov 2021 08:50:17 +0100	[thread overview]
Message-ID: <CAB0TPYFffM1R4n7SxjB2f8wM-hJc9JN+SC3xgd_wTROaERyVZg@mail.gmail.com> (raw)
In-Reply-To: <20211123191737.1296541-2-tkjos@google.com>

On Tue, Nov 23, 2021 at 8:17 PM 'Todd Kjos' via kernel-team
<kernel-team@android.com> wrote:
>
> Transactions are copied from the sender to the target
> first and objects like BINDER_TYPE_PTR and BINDER_TYPE_FDA
> are then fixed up. This means there is a short period where
> the sender's version of these objects are visible to the
> target prior to the fixups.
>
> Instead of copying all of the data first, copy data only
> after any needed fixups have been applied.
>
> Signed-off-by: Todd Kjos <tkjos@google.com>
Reviewed-by: Martijn Coenen <maco@android.com>

> ---
>  drivers/android/binder.c | 95 +++++++++++++++++++++++++++++-----------
>  1 file changed, 70 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 49fb74196d02..571d3c203557 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1608,15 +1608,21 @@ static void binder_cleanup_transaction(struct binder_transaction *t,
>  /**
>   * binder_get_object() - gets object and checks for valid metadata
>   * @proc:      binder_proc owning the buffer
> + * @u:         sender's user pointer to base of buffer
>   * @buffer:    binder_buffer that we're parsing.
>   * @offset:    offset in the @buffer at which to validate an object.
>   * @object:    struct binder_object to read into
>   *
> - * Return:     If there's a valid metadata object at @offset in @buffer, the
> + * Copy the binder object at the given offset into @object. If @u is
> + * provided then the copy is from the sender's buffer. If not, then
> + * it is copied from the target's @buffer.
> + *
> + * Return:     If there's a valid metadata object at @offset, the
>   *             size of that object. Otherwise, it returns zero. The object
>   *             is read into the struct binder_object pointed to by @object.
>   */
>  static size_t binder_get_object(struct binder_proc *proc,
> +                               const void __user *u,
>                                 struct binder_buffer *buffer,
>                                 unsigned long offset,
>                                 struct binder_object *object)
> @@ -1626,10 +1632,16 @@ static size_t binder_get_object(struct binder_proc *proc,
>         size_t object_size = 0;
>
>         read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset);
> -       if (offset > buffer->data_size || read_size < sizeof(*hdr) ||
> -           binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
> -                                         offset, read_size))
> +       if (offset > buffer->data_size || read_size < sizeof(*hdr))
>                 return 0;
> +       if (u) {
> +               if (copy_from_user(object, u + offset, read_size))
> +                       return 0;
> +       } else {
> +               if (binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
> +                                                 offset, read_size))
> +                       return 0;
> +       }
>
>         /* Ok, now see if we read a complete object. */
>         hdr = &object->hdr;
> @@ -1702,7 +1714,7 @@ static struct binder_buffer_object *binder_validate_ptr(
>                                           b, buffer_offset,
>                                           sizeof(object_offset)))
>                 return NULL;
> -       object_size = binder_get_object(proc, b, object_offset, object);
> +       object_size = binder_get_object(proc, NULL, b, object_offset, object);
>         if (!object_size || object->hdr.type != BINDER_TYPE_PTR)
>                 return NULL;
>         if (object_offsetp)
> @@ -1767,7 +1779,8 @@ static bool binder_validate_fixup(struct binder_proc *proc,
>                 unsigned long buffer_offset;
>                 struct binder_object last_object;
>                 struct binder_buffer_object *last_bbo;
> -               size_t object_size = binder_get_object(proc, b, last_obj_offset,
> +               size_t object_size = binder_get_object(proc, NULL, b,
> +                                                      last_obj_offset,
>                                                        &last_object);
>                 if (object_size != sizeof(*last_bbo))
>                         return false;
> @@ -1882,7 +1895,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
>                 if (!binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
>                                                    buffer, buffer_offset,
>                                                    sizeof(object_offset)))
> -                       object_size = binder_get_object(proc, buffer,
> +                       object_size = binder_get_object(proc, NULL, buffer,
>                                                         object_offset, &object);
>                 if (object_size == 0) {
>                         pr_err("transaction release %d bad object at offset %lld, size %zd\n",
> @@ -2455,6 +2468,7 @@ static void binder_transaction(struct binder_proc *proc,
>         binder_size_t off_start_offset, off_end_offset;
>         binder_size_t off_min;
>         binder_size_t sg_buf_offset, sg_buf_end_offset;
> +       binder_size_t user_offset = 0;
>         struct binder_proc *target_proc = NULL;
>         struct binder_thread *target_thread = NULL;
>         struct binder_node *target_node = NULL;
> @@ -2469,6 +2483,8 @@ static void binder_transaction(struct binder_proc *proc,
>         int t_debug_id = atomic_inc_return(&binder_last_id);
>         char *secctx = NULL;
>         u32 secctx_sz = 0;
> +       const void __user *user_buffer = (const void __user *)
> +                               (uintptr_t)tr->data.ptr.buffer;
>
>         e = binder_transaction_log_add(&binder_transaction_log);
>         e->debug_id = t_debug_id;
> @@ -2692,7 +2708,7 @@ static void binder_transaction(struct binder_proc *proc,
>                              "%d:%d BC_REPLY %d -> %d:%d, data %016llx-%016llx size %lld-%lld-%lld\n",
>                              proc->pid, thread->pid, t->debug_id,
>                              target_proc->pid, target_thread->pid,
> -                            (u64)tr->data.ptr.buffer,
> +                            (u64)user_buffer,
>                              (u64)tr->data.ptr.offsets,
>                              (u64)tr->data_size, (u64)tr->offsets_size,
>                              (u64)extra_buffers_size);
> @@ -2701,7 +2717,7 @@ static void binder_transaction(struct binder_proc *proc,
>                              "%d:%d BC_TRANSACTION %d -> %d - node %d, data %016llx-%016llx size %lld-%lld-%lld\n",
>                              proc->pid, thread->pid, t->debug_id,
>                              target_proc->pid, target_node->debug_id,
> -                            (u64)tr->data.ptr.buffer,
> +                            (u64)user_buffer,
>                              (u64)tr->data.ptr.offsets,
>                              (u64)tr->data_size, (u64)tr->offsets_size,
>                              (u64)extra_buffers_size);
> @@ -2780,19 +2796,6 @@ static void binder_transaction(struct binder_proc *proc,
>         t->buffer->clear_on_free = !!(t->flags & TF_CLEAR_BUF);
>         trace_binder_transaction_alloc_buf(t->buffer);
>
> -       if (binder_alloc_copy_user_to_buffer(
> -                               &target_proc->alloc,
> -                               t->buffer, 0,
> -                               (const void __user *)
> -                                       (uintptr_t)tr->data.ptr.buffer,
> -                               tr->data_size)) {
> -               binder_user_error("%d:%d got transaction with invalid data ptr\n",
> -                               proc->pid, thread->pid);
> -               return_error = BR_FAILED_REPLY;
> -               return_error_param = -EFAULT;
> -               return_error_line = __LINE__;
> -               goto err_copy_data_failed;
> -       }
>         if (binder_alloc_copy_user_to_buffer(
>                                 &target_proc->alloc,
>                                 t->buffer,
> @@ -2837,6 +2840,7 @@ static void binder_transaction(struct binder_proc *proc,
>                 size_t object_size;
>                 struct binder_object object;
>                 binder_size_t object_offset;
> +               binder_size_t copy_size;
>
>                 if (binder_alloc_copy_from_buffer(&target_proc->alloc,
>                                                   &object_offset,
> @@ -2848,8 +2852,27 @@ static void binder_transaction(struct binder_proc *proc,
>                         return_error_line = __LINE__;
>                         goto err_bad_offset;
>                 }
> -               object_size = binder_get_object(target_proc, t->buffer,
> -                                               object_offset, &object);
> +
> +               /*
> +                * Copy the source user buffer up to the next object
> +                * that will be processed.
> +                */
> +               copy_size = object_offset - user_offset;
> +               if (copy_size && (user_offset > object_offset ||
> +                               binder_alloc_copy_user_to_buffer(
> +                                       &target_proc->alloc,
> +                                       t->buffer, user_offset,
> +                                       user_buffer + user_offset,
> +                                       copy_size))) {
> +                       binder_user_error("%d:%d got transaction with invalid data ptr\n",
> +                                       proc->pid, thread->pid);
> +                       return_error = BR_FAILED_REPLY;
> +                       return_error_param = -EFAULT;
> +                       return_error_line = __LINE__;
> +                       goto err_copy_data_failed;
> +               }
> +               object_size = binder_get_object(target_proc, user_buffer,
> +                               t->buffer, object_offset, &object);
>                 if (object_size == 0 || object_offset < off_min) {
>                         binder_user_error("%d:%d got transaction with invalid offset (%lld, min %lld max %lld) or object.\n",
>                                           proc->pid, thread->pid,
> @@ -2861,6 +2884,11 @@ static void binder_transaction(struct binder_proc *proc,
>                         return_error_line = __LINE__;
>                         goto err_bad_offset;
>                 }
> +               /*
> +                * Set offset to the next buffer fragment to be
> +                * copied
> +                */
> +               user_offset = object_offset + object_size;
>
>                 hdr = &object.hdr;
>                 off_min = object_offset + object_size;
> @@ -2956,7 +2984,11 @@ static void binder_transaction(struct binder_proc *proc,
>                         }
>                         ret = binder_translate_fd_array(fda, parent, t, thread,
>                                                         in_reply_to);
> -                       if (ret < 0) {
> +                       if (ret < 0 ||
> +                           binder_alloc_copy_to_buffer(&target_proc->alloc,
> +                                                       t->buffer,
> +                                                       object_offset,
> +                                                       fda, sizeof(*fda))) {
>                                 return_error = BR_FAILED_REPLY;
>                                 return_error_param = ret;
>                                 return_error_line = __LINE__;
> @@ -3028,6 +3060,19 @@ static void binder_transaction(struct binder_proc *proc,
>                         goto err_bad_object_type;
>                 }
>         }
> +       /* Done processing objects, copy the rest of the buffer */
> +       if (binder_alloc_copy_user_to_buffer(
> +                               &target_proc->alloc,
> +                               t->buffer, user_offset,
> +                               user_buffer + user_offset,
> +                               tr->data_size - user_offset)) {
> +               binder_user_error("%d:%d got transaction with invalid data ptr\n",
> +                               proc->pid, thread->pid);
> +               return_error = BR_FAILED_REPLY;
> +               return_error_param = -EFAULT;
> +               return_error_line = __LINE__;
> +               goto err_copy_data_failed;
> +       }
>         if (t->buffer->oneway_spam_suspect)
>                 tcomplete->type = BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT;
>         else
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>
>

  reply	other threads:[~2021-11-24  7:50 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-23 19:17 [PATCH 0/3] binder: Prevent untranslated sender data from being copied to target Todd Kjos
2021-11-23 19:17 ` [PATCH 1/3] binder: avoid potential data leakage when copying txn Todd Kjos
2021-11-24  7:50   ` Martijn Coenen [this message]
2021-11-24 13:01   ` Dan Carpenter
2021-11-24 20:11     ` Todd Kjos
2021-11-25  2:05   ` kernel test robot
2021-11-25 12:18   ` kernel test robot
2021-11-23 19:17 ` [PATCH 2/3] binder: read pre-translated fds from sender buffer Todd Kjos
2021-11-24  7:50   ` Martijn Coenen
2021-11-24 12:37   ` Dan Carpenter
2021-11-24 20:33     ` Todd Kjos
2021-11-25  6:37       ` Dan Carpenter
2021-11-23 19:17 ` [PATCH 3/3] binder: defer copies of pre-patched txn data Todd Kjos
2021-11-24  7:50   ` Martijn Coenen
2021-11-24 11:09   ` Dan Carpenter
2021-11-24 20:39     ` Todd Kjos
2021-11-24 12:43   ` Dan Carpenter
2021-11-24 20:37     ` Todd Kjos
2021-11-24  8:08 ` [PATCH 0/3] binder: Prevent untranslated sender data from being copied to target Greg KH
2021-11-24 15:54   ` Todd Kjos

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAB0TPYFffM1R4n7SxjB2f8wM-hJc9JN+SC3xgd_wTROaERyVZg@mail.gmail.com \
    --to=maco@android.com \
    --cc=arve@android.com \
    --cc=christian@brauner.io \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joel@joelfernandes.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maco@google.com \
    --cc=tkjos@google.com \
    --subject='Re: [PATCH 1/3] binder: avoid potential data leakage when copying txn' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).