[PATCH] binder: return errors from buffer copy functions

From: Todd Kjos
Date: Fri Jun 28 2019 - 12:50:31 EST

Next message: Mukesh Ojha: "Perf framework : Cluster based counter support"
Previous message: Sylwester Nawrocki: "Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

The buffer copy functions assumed the caller would ensure
correct alignment and that the memory to be copied was
completely within the binder buffer. There have been
a few cases discovered by syzkallar where a malformed
transaction created by a user could violated the
assumptions and resulted in a BUG_ON.

The fix is to remove the BUG_ON and always return the
error to be handled appropriately by the caller.

Acked-by: Martijn Coenen <maco@xxxxxxxxxxx>
Reported-by: syzbot+3ae18325f96190606754@xxxxxxxxxxxxxxxxxxxxxxxxx
Fixes: bde4a19fc04f ("binder: use userspace pointer as base of buffer space")
Suggested-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Signed-off-by: Todd Kjos <tkjos@xxxxxxxxxx>
---
drivers/android/binder.c | 153 ++++++++++++++++++++-------------
drivers/android/binder_alloc.c | 44 +++++-----
drivers/android/binder_alloc.h | 22 ++---
3 files changed, 126 insertions(+), 93 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index bc26b5511f0a9..414492c136ddf 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2059,10 +2059,9 @@ static size_t binder_get_object(struct binder_proc *proc,

read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset);
if (offset > buffer->data_size || read_size < sizeof(*hdr) ||
- !IS_ALIGNED(offset, sizeof(u32)))
+ binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
+ offset, read_size))
return 0;
- binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
- offset, read_size);

/* Ok, now see if we read a complete object. */
hdr = &object->hdr;
@@ -2131,8 +2130,10 @@ static struct binder_buffer_object *binder_validate_ptr(
return NULL;

buffer_offset = start_offset + sizeof(binder_size_t) * index;
- binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
- b, buffer_offset, sizeof(object_offset));
+ if (binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
+ b, buffer_offset,
+ sizeof(object_offset)))
+ return NULL;
object_size = binder_get_object(proc, b, object_offset, object);
if (!object_size || object->hdr.type != BINDER_TYPE_PTR)
return NULL;
@@ -2212,10 +2213,12 @@ static bool binder_validate_fixup(struct binder_proc *proc,
return false;
last_min_offset = last_bbo->parent_offset + sizeof(uintptr_t);
buffer_offset = objects_start_offset +
- sizeof(binder_size_t) * last_bbo->parent,
- binder_alloc_copy_from_buffer(&proc->alloc, &last_obj_offset,
- b, buffer_offset,
- sizeof(last_obj_offset));
+ sizeof(binder_size_t) * last_bbo->parent;
+ if (binder_alloc_copy_from_buffer(&proc->alloc,
+ &last_obj_offset,
+ b, buffer_offset,
+ sizeof(last_obj_offset)))
+ return false;
}
return (fixup_offset >= last_min_offset);
}
@@ -2301,15 +2304,15 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
buffer_offset += sizeof(binder_size_t)) {
struct binder_object_header *hdr;
- size_t object_size;
+ size_t object_size = 0;
struct binder_object object;
binder_size_t object_offset;

- binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
- buffer, buffer_offset,
- sizeof(object_offset));
- object_size = binder_get_object(proc, buffer,
- object_offset, &object);
+ if (!binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
+ buffer, buffer_offset,
+ sizeof(object_offset)))
+ object_size = binder_get_object(proc, buffer,
+ object_offset, &object);
if (object_size == 0) {
pr_err("transaction release %d bad object at offset %lld, size %zd\n",
debug_id, (u64)object_offset, buffer->data_size);
@@ -2432,15 +2435,16 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
for (fd_index = 0; fd_index < fda->num_fds;
fd_index++) {
u32 fd;
+ int err;
binder_size_t offset = fda_offset +
fd_index * sizeof(fd);

- binder_alloc_copy_from_buffer(&proc->alloc,
- &fd,
- buffer,
- offset,
- sizeof(fd));
- binder_deferred_fd_close(fd);
+ err = binder_alloc_copy_from_buffer(
+ &proc->alloc, &fd, buffer,
+ offset, sizeof(fd));
+ WARN_ON(err);
+ if (!err)
+ binder_deferred_fd_close(fd);
}
} break;
default:
@@ -2683,11 +2687,12 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
int ret;
binder_size_t offset = fda_offset + fdi * sizeof(fd);

- binder_alloc_copy_from_buffer(&target_proc->alloc,
- &fd, t->buffer,
- offset, sizeof(fd));
- ret = binder_translate_fd(fd, offset, t, thread,
- in_reply_to);
+ ret = binder_alloc_copy_from_buffer(&target_proc->alloc,
+ &fd, t->buffer,
+ offset, sizeof(fd));
+ if (!ret)
+ ret = binder_translate_fd(fd, offset, t, thread,
+ in_reply_to);
if (ret < 0)
return ret;
}
@@ -2740,8 +2745,12 @@ static int binder_fixup_parent(struct binder_transaction *t,
}
buffer_offset = bp->parent_offset +
(uintptr_t)parent->buffer - (uintptr_t)b->user_data;
- binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset,
- &bp->buffer, sizeof(bp->buffer));
+ if (binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset,
+ &bp->buffer, sizeof(bp->buffer))) {
+ binder_user_error("%d:%d got transaction with invalid parent offset\n",
+ proc->pid, thread->pid);
+ return -EINVAL;
+ }

return 0;
}
@@ -3160,15 +3169,20 @@ static void binder_transaction(struct binder_proc *proc,
goto err_binder_alloc_buf_failed;
}
if (secctx) {
+ int err;
size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
ALIGN(tr->offsets_size, sizeof(void *)) +
ALIGN(extra_buffers_size, sizeof(void *)) -
ALIGN(secctx_sz, sizeof(u64));

t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
- binder_alloc_copy_to_buffer(&target_proc->alloc,
- t->buffer, buf_offset,
- secctx, secctx_sz);
+ err = binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer, buf_offset,
+ secctx, secctx_sz);
+ if (err) {
+ t->security_ctx = 0;
+ WARN_ON(1);
+ }
security_release_secctx(secctx, secctx_sz);
secctx = NULL;
}
@@ -3234,11 +3248,16 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_object object;
binder_size_t object_offset;

- binder_alloc_copy_from_buffer(&target_proc->alloc,
- &object_offset,
- t->buffer,
- buffer_offset,
- sizeof(object_offset));
+ if (binder_alloc_copy_from_buffer(&target_proc->alloc,
+ &object_offset,
+ t->buffer,
+ buffer_offset,
+ sizeof(object_offset))) {
+ return_error = BR_FAILED_REPLY;
+ return_error_param = -EINVAL;
+ return_error_line = __LINE__;
+ goto err_bad_offset;
+ }
object_size = binder_get_object(target_proc, t->buffer,
object_offset, &object);
if (object_size == 0 || object_offset < off_min) {
@@ -3262,15 +3281,17 @@ static void binder_transaction(struct binder_proc *proc,

fp = to_flat_binder_object(hdr);
ret = binder_translate_binder(fp, t, thread);
- if (ret < 0) {
+
+ if (ret < 0 ||
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer,
+ object_offset,
+ fp, sizeof(*fp))) {
return_error = BR_FAILED_REPLY;
return_error_param = ret;
return_error_line = __LINE__;
goto err_translate_failed;
}
- binder_alloc_copy_to_buffer(&target_proc->alloc,
- t->buffer, object_offset,
- fp, sizeof(*fp));
} break;
case BINDER_TYPE_HANDLE:
case BINDER_TYPE_WEAK_HANDLE: {
@@ -3278,15 +3299,16 @@ static void binder_transaction(struct binder_proc *proc,

fp = to_flat_binder_object(hdr);
ret = binder_translate_handle(fp, t, thread);
- if (ret < 0) {
+ if (ret < 0 ||
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer,
+ object_offset,
+ fp, sizeof(*fp))) {
return_error = BR_FAILED_REPLY;
return_error_param = ret;
return_error_line = __LINE__;
goto err_translate_failed;
}
- binder_alloc_copy_to_buffer(&target_proc->alloc,
- t->buffer, object_offset,
- fp, sizeof(*fp));
} break;

case BINDER_TYPE_FD: {
@@ -3296,16 +3318,17 @@ static void binder_transaction(struct binder_proc *proc,
int ret = binder_translate_fd(fp->fd, fd_offset, t,
thread, in_reply_to);

- if (ret < 0) {
+ fp->pad_binder = 0;
+ if (ret < 0 ||
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer,
+ object_offset,
+ fp, sizeof(*fp))) {
return_error = BR_FAILED_REPLY;
return_error_param = ret;
return_error_line = __LINE__;
goto err_translate_failed;
}
- fp->pad_binder = 0;
- binder_alloc_copy_to_buffer(&target_proc->alloc,
- t->buffer, object_offset,
- fp, sizeof(*fp));
} break;
case BINDER_TYPE_FDA: {
struct binder_object ptr_object;
@@ -3393,15 +3416,16 @@ static void binder_transaction(struct binder_proc *proc,
num_valid,
last_fixup_obj_off,
last_fixup_min_off);
- if (ret < 0) {
+ if (ret < 0 ||
+ binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer,
+ object_offset,
+ bp, sizeof(*bp))) {
return_error = BR_FAILED_REPLY;
return_error_param = ret;
return_error_line = __LINE__;
goto err_translate_failed;
}
- binder_alloc_copy_to_buffer(&target_proc->alloc,
- t->buffer, object_offset,
- bp, sizeof(*bp));
last_fixup_obj_off = object_offset;
last_fixup_min_off = 0;
} break;
@@ -4140,20 +4164,27 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
trace_binder_transaction_fd_recv(t, fd, fixup->offset);
fd_install(fd, fixup->file);
fixup->file = NULL;
- binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
- fixup->offset, &fd,
- sizeof(u32));
+ if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
+ fixup->offset, &fd,
+ sizeof(u32))) {
+ ret = -EINVAL;
+ break;
+ }
}
list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
if (fixup->file) {
fput(fixup->file);
} else if (ret) {
u32 fd;
-
- binder_alloc_copy_from_buffer(&proc->alloc, &fd,
- t->buffer, fixup->offset,
- sizeof(fd));
- binder_deferred_fd_close(fd);
+ int err;
+
+ err = binder_alloc_copy_from_buffer(&proc->alloc, &fd,
+ t->buffer,
+ fixup->offset,
+ sizeof(fd));
+ WARN_ON(err);
+ if (!err)
+ binder_deferred_fd_close(fd);
}
list_del(&fixup->fixup_entry);
kfree(fixup);
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index ce5603c2291c6..6d79a1b0d4463 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1119,15 +1119,16 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
return 0;
}

-static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
- bool to_buffer,
- struct binder_buffer *buffer,
- binder_size_t buffer_offset,
- void *ptr,
- size_t bytes)
+static int binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
+ bool to_buffer,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ void *ptr,
+ size_t bytes)
{
/* All copies must be 32-bit aligned and 32-bit size */
- BUG_ON(!check_buffer(alloc, buffer, buffer_offset, bytes));
+ if (!check_buffer(alloc, buffer, buffer_offset, bytes))
+ return -EINVAL;

while (bytes) {
unsigned long size;
@@ -1155,25 +1156,26 @@ static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
ptr = ptr + size;
buffer_offset += size;
}
+ return 0;
}

-void binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
- struct binder_buffer *buffer,
- binder_size_t buffer_offset,
- void *src,
- size_t bytes)
+int binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ void *src,
+ size_t bytes)
{
- binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset,
- src, bytes);
+ return binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset,
+ src, bytes);
}

-void binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
- void *dest,
- struct binder_buffer *buffer,
- binder_size_t buffer_offset,
- size_t bytes)
+int binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
+ void *dest,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ size_t bytes)
{
- binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset,
- dest, bytes);
+ return binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset,
+ dest, bytes);
}

diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 71bfa95f8e09b..db9c1b984695d 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -159,17 +159,17 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
const void __user *from,
size_t bytes);

-void binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
- struct binder_buffer *buffer,
- binder_size_t buffer_offset,
- void *src,
- size_t bytes);
-
-void binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
- void *dest,
- struct binder_buffer *buffer,
- binder_size_t buffer_offset,
- size_t bytes);
+int binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ void *src,
+ size_t bytes);
+
+int binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
+ void *dest,
+ struct binder_buffer *buffer,
+ binder_size_t buffer_offset,
+ size_t bytes);

#endif /* _LINUX_BINDER_ALLOC_H */

--
2.22.0.410.gd8fdbe21b5-goog

Next message: Mukesh Ojha: "Perf framework : Cluster based counter support"
Previous message: Sylwester Nawrocki: "Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]