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
>
>
next prev parent 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).