Skip to content

Commit

Permalink
binder: avoid potential data leakage when copying txn
Browse files Browse the repository at this point in the history 
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.

Fixes: 457b9a6 ("Staging: android: add binder driver")
Reviewed-by: Martijn Coenen <maco@android.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Link: https://lore.kernel.org/r/20211130185152.437403-3-tkjos@google.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
@toddkjos @gregkh
toddkjos authored and gregkh committed Dec 3, 2021
1 parent fe6b186 commit 6d98eb9
Showing 1 changed file with 70 additions and 24 deletions.
94 changes: 70 additions & 24 deletions drivers/android/binder.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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;
Expand Down Expand Up @@ -2956,9 +2984,14 @@ 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)
ret = binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer,
object_offset,
fda, sizeof(*fda));
if (ret) {
return_error = BR_FAILED_REPLY;
return_error_param = ret;
return_error_param = ret > 0 ? -EINVAL : ret;
return_error_line = __LINE__;
goto err_translate_failed;
}
Expand Down Expand Up @@ -3028,6 +3061,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
Expand Down

0 comments on commit 6d98eb9

Please sign in to comment.