Commit bb4a2e48 authored by Todd Kjos's avatar Todd Kjos Committed by Greg Kroah-Hartman

binder: return errors from buffer copy functions

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: default avatarMartijn Coenen <maco@android.com>
Reported-by: syzbot+3ae18325f96190606754@syzkaller.appspotmail.com
Fixes: bde4a19f ("binder: use userspace pointer as base of buffer space")
Suggested-by: default avatarDan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: default avatarTodd Kjos <tkjos@google.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 25c7eabe
...@@ -2059,10 +2059,9 @@ static size_t binder_get_object(struct binder_proc *proc, ...@@ -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); read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset);
if (offset > buffer->data_size || read_size < sizeof(*hdr) || if (offset > buffer->data_size || read_size < sizeof(*hdr) ||
!IS_ALIGNED(offset, sizeof(u32)))
return 0;
binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
offset, read_size); offset, read_size))
return 0;
/* Ok, now see if we read a complete object. */ /* Ok, now see if we read a complete object. */
hdr = &object->hdr; hdr = &object->hdr;
...@@ -2131,8 +2130,10 @@ static struct binder_buffer_object *binder_validate_ptr( ...@@ -2131,8 +2130,10 @@ static struct binder_buffer_object *binder_validate_ptr(
return NULL; return NULL;
buffer_offset = start_offset + sizeof(binder_size_t) * index; buffer_offset = start_offset + sizeof(binder_size_t) * index;
binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, if (binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
b, buffer_offset, sizeof(object_offset)); b, buffer_offset,
sizeof(object_offset)))
return NULL;
object_size = binder_get_object(proc, b, object_offset, object); object_size = binder_get_object(proc, b, object_offset, object);
if (!object_size || object->hdr.type != BINDER_TYPE_PTR) if (!object_size || object->hdr.type != BINDER_TYPE_PTR)
return NULL; return NULL;
...@@ -2212,10 +2213,12 @@ static bool binder_validate_fixup(struct binder_proc *proc, ...@@ -2212,10 +2213,12 @@ static bool binder_validate_fixup(struct binder_proc *proc,
return false; return false;
last_min_offset = last_bbo->parent_offset + sizeof(uintptr_t); last_min_offset = last_bbo->parent_offset + sizeof(uintptr_t);
buffer_offset = objects_start_offset + buffer_offset = objects_start_offset +
sizeof(binder_size_t) * last_bbo->parent, sizeof(binder_size_t) * last_bbo->parent;
binder_alloc_copy_from_buffer(&proc->alloc, &last_obj_offset, if (binder_alloc_copy_from_buffer(&proc->alloc,
&last_obj_offset,
b, buffer_offset, b, buffer_offset,
sizeof(last_obj_offset)); sizeof(last_obj_offset)))
return false;
} }
return (fixup_offset >= last_min_offset); return (fixup_offset >= last_min_offset);
} }
...@@ -2301,13 +2304,13 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, ...@@ -2301,13 +2304,13 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
for (buffer_offset = off_start_offset; buffer_offset < off_end_offset; for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
buffer_offset += sizeof(binder_size_t)) { buffer_offset += sizeof(binder_size_t)) {
struct binder_object_header *hdr; struct binder_object_header *hdr;
size_t object_size; size_t object_size = 0;
struct binder_object object; struct binder_object object;
binder_size_t object_offset; binder_size_t object_offset;
binder_alloc_copy_from_buffer(&proc->alloc, &object_offset, if (!binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
buffer, buffer_offset, buffer, buffer_offset,
sizeof(object_offset)); sizeof(object_offset)))
object_size = binder_get_object(proc, buffer, object_size = binder_get_object(proc, buffer,
object_offset, &object); object_offset, &object);
if (object_size == 0) { if (object_size == 0) {
...@@ -2432,14 +2435,15 @@ static void binder_transaction_buffer_release(struct binder_proc *proc, ...@@ -2432,14 +2435,15 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
for (fd_index = 0; fd_index < fda->num_fds; for (fd_index = 0; fd_index < fda->num_fds;
fd_index++) { fd_index++) {
u32 fd; u32 fd;
int err;
binder_size_t offset = fda_offset + binder_size_t offset = fda_offset +
fd_index * sizeof(fd); fd_index * sizeof(fd);
binder_alloc_copy_from_buffer(&proc->alloc, err = binder_alloc_copy_from_buffer(
&fd, &proc->alloc, &fd, buffer,
buffer, offset, sizeof(fd));
offset, WARN_ON(err);
sizeof(fd)); if (!err)
binder_deferred_fd_close(fd); binder_deferred_fd_close(fd);
} }
} break; } break;
...@@ -2683,9 +2687,10 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda, ...@@ -2683,9 +2687,10 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
int ret; int ret;
binder_size_t offset = fda_offset + fdi * sizeof(fd); binder_size_t offset = fda_offset + fdi * sizeof(fd);
binder_alloc_copy_from_buffer(&target_proc->alloc, ret = binder_alloc_copy_from_buffer(&target_proc->alloc,
&fd, t->buffer, &fd, t->buffer,
offset, sizeof(fd)); offset, sizeof(fd));
if (!ret)
ret = binder_translate_fd(fd, offset, t, thread, ret = binder_translate_fd(fd, offset, t, thread,
in_reply_to); in_reply_to);
if (ret < 0) if (ret < 0)
...@@ -2740,8 +2745,12 @@ static int binder_fixup_parent(struct binder_transaction *t, ...@@ -2740,8 +2745,12 @@ static int binder_fixup_parent(struct binder_transaction *t,
} }
buffer_offset = bp->parent_offset + buffer_offset = bp->parent_offset +
(uintptr_t)parent->buffer - (uintptr_t)b->user_data; (uintptr_t)parent->buffer - (uintptr_t)b->user_data;
binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset, if (binder_alloc_copy_to_buffer(&target_proc->alloc, b, buffer_offset,
&bp->buffer, sizeof(bp->buffer)); &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; return 0;
} }
...@@ -3160,15 +3169,20 @@ static void binder_transaction(struct binder_proc *proc, ...@@ -3160,15 +3169,20 @@ static void binder_transaction(struct binder_proc *proc,
goto err_binder_alloc_buf_failed; goto err_binder_alloc_buf_failed;
} }
if (secctx) { if (secctx) {
int err;
size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) + size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
ALIGN(tr->offsets_size, sizeof(void *)) + ALIGN(tr->offsets_size, sizeof(void *)) +
ALIGN(extra_buffers_size, sizeof(void *)) - ALIGN(extra_buffers_size, sizeof(void *)) -
ALIGN(secctx_sz, sizeof(u64)); ALIGN(secctx_sz, sizeof(u64));
t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset; t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
binder_alloc_copy_to_buffer(&target_proc->alloc, err = binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, buf_offset, t->buffer, buf_offset,
secctx, secctx_sz); secctx, secctx_sz);
if (err) {
t->security_ctx = 0;
WARN_ON(1);
}
security_release_secctx(secctx, secctx_sz); security_release_secctx(secctx, secctx_sz);
secctx = NULL; secctx = NULL;
} }
...@@ -3234,11 +3248,16 @@ static void binder_transaction(struct binder_proc *proc, ...@@ -3234,11 +3248,16 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_object object; struct binder_object object;
binder_size_t object_offset; binder_size_t object_offset;
binder_alloc_copy_from_buffer(&target_proc->alloc, if (binder_alloc_copy_from_buffer(&target_proc->alloc,
&object_offset, &object_offset,
t->buffer, t->buffer,
buffer_offset, buffer_offset,
sizeof(object_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_size = binder_get_object(target_proc, t->buffer,
object_offset, &object); object_offset, &object);
if (object_size == 0 || object_offset < off_min) { if (object_size == 0 || object_offset < off_min) {
...@@ -3262,15 +3281,17 @@ static void binder_transaction(struct binder_proc *proc, ...@@ -3262,15 +3281,17 @@ static void binder_transaction(struct binder_proc *proc,
fp = to_flat_binder_object(hdr); fp = to_flat_binder_object(hdr);
ret = binder_translate_binder(fp, t, thread); 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 = BR_FAILED_REPLY;
return_error_param = ret; return_error_param = ret;
return_error_line = __LINE__; return_error_line = __LINE__;
goto err_translate_failed; goto err_translate_failed;
} }
binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, object_offset,
fp, sizeof(*fp));
} break; } break;
case BINDER_TYPE_HANDLE: case BINDER_TYPE_HANDLE:
case BINDER_TYPE_WEAK_HANDLE: { case BINDER_TYPE_WEAK_HANDLE: {
...@@ -3278,15 +3299,16 @@ static void binder_transaction(struct binder_proc *proc, ...@@ -3278,15 +3299,16 @@ static void binder_transaction(struct binder_proc *proc,
fp = to_flat_binder_object(hdr); fp = to_flat_binder_object(hdr);
ret = binder_translate_handle(fp, t, thread); 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 = BR_FAILED_REPLY;
return_error_param = ret; return_error_param = ret;
return_error_line = __LINE__; return_error_line = __LINE__;
goto err_translate_failed; goto err_translate_failed;
} }
binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, object_offset,
fp, sizeof(*fp));
} break; } break;
case BINDER_TYPE_FD: { case BINDER_TYPE_FD: {
...@@ -3296,16 +3318,17 @@ static void binder_transaction(struct binder_proc *proc, ...@@ -3296,16 +3318,17 @@ static void binder_transaction(struct binder_proc *proc,
int ret = binder_translate_fd(fp->fd, fd_offset, t, int ret = binder_translate_fd(fp->fd, fd_offset, t,
thread, in_reply_to); 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 = BR_FAILED_REPLY;
return_error_param = ret; return_error_param = ret;
return_error_line = __LINE__; return_error_line = __LINE__;
goto err_translate_failed; goto err_translate_failed;
} }
fp->pad_binder = 0;
binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, object_offset,
fp, sizeof(*fp));
} break; } break;
case BINDER_TYPE_FDA: { case BINDER_TYPE_FDA: {
struct binder_object ptr_object; struct binder_object ptr_object;
...@@ -3393,15 +3416,16 @@ static void binder_transaction(struct binder_proc *proc, ...@@ -3393,15 +3416,16 @@ static void binder_transaction(struct binder_proc *proc,
num_valid, num_valid,
last_fixup_obj_off, last_fixup_obj_off,
last_fixup_min_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 = BR_FAILED_REPLY;
return_error_param = ret; return_error_param = ret;
return_error_line = __LINE__; return_error_line = __LINE__;
goto err_translate_failed; 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_obj_off = object_offset;
last_fixup_min_off = 0; last_fixup_min_off = 0;
} break; } break;
...@@ -4140,19 +4164,26 @@ static int binder_apply_fd_fixups(struct binder_proc *proc, ...@@ -4140,19 +4164,26 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
trace_binder_transaction_fd_recv(t, fd, fixup->offset); trace_binder_transaction_fd_recv(t, fd, fixup->offset);
fd_install(fd, fixup->file); fd_install(fd, fixup->file);
fixup->file = NULL; fixup->file = NULL;
binder_alloc_copy_to_buffer(&proc->alloc, t->buffer, if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
fixup->offset, &fd, fixup->offset, &fd,
sizeof(u32)); sizeof(u32))) {
ret = -EINVAL;
break;
}
} }
list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) { list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
if (fixup->file) { if (fixup->file) {
fput(fixup->file); fput(fixup->file);
} else if (ret) { } else if (ret) {
u32 fd; u32 fd;
int err;
binder_alloc_copy_from_buffer(&proc->alloc, &fd, err = binder_alloc_copy_from_buffer(&proc->alloc, &fd,
t->buffer, fixup->offset, t->buffer,
fixup->offset,
sizeof(fd)); sizeof(fd));
WARN_ON(err);
if (!err)
binder_deferred_fd_close(fd); binder_deferred_fd_close(fd);
} }
list_del(&fixup->fixup_entry); list_del(&fixup->fixup_entry);
......
...@@ -1119,7 +1119,7 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, ...@@ -1119,7 +1119,7 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
return 0; return 0;
} }
static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc, static int binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
bool to_buffer, bool to_buffer,
struct binder_buffer *buffer, struct binder_buffer *buffer,
binder_size_t buffer_offset, binder_size_t buffer_offset,
...@@ -1127,7 +1127,8 @@ static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc, ...@@ -1127,7 +1127,8 @@ static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
size_t bytes) size_t bytes)
{ {
/* All copies must be 32-bit aligned and 32-bit size */ /* 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) { while (bytes) {
unsigned long size; unsigned long size;
...@@ -1155,25 +1156,26 @@ static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc, ...@@ -1155,25 +1156,26 @@ static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
ptr = ptr + size; ptr = ptr + size;
buffer_offset += size; buffer_offset += size;
} }
return 0;
} }
void binder_alloc_copy_to_buffer(struct binder_alloc *alloc, int binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
struct binder_buffer *buffer, struct binder_buffer *buffer,
binder_size_t buffer_offset, binder_size_t buffer_offset,
void *src, void *src,
size_t bytes) size_t bytes)
{ {
binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset, return binder_alloc_do_buffer_copy(alloc, true, buffer, buffer_offset,
src, bytes); src, bytes);
} }
void binder_alloc_copy_from_buffer(struct binder_alloc *alloc, int binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
void *dest, void *dest,
struct binder_buffer *buffer, struct binder_buffer *buffer,
binder_size_t buffer_offset, binder_size_t buffer_offset,
size_t bytes) size_t bytes)
{ {
binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset, return binder_alloc_do_buffer_copy(alloc, false, buffer, buffer_offset,
dest, bytes); dest, bytes);
} }
...@@ -159,13 +159,13 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc, ...@@ -159,13 +159,13 @@ binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
const void __user *from, const void __user *from,
size_t bytes); size_t bytes);
void binder_alloc_copy_to_buffer(struct binder_alloc *alloc, int binder_alloc_copy_to_buffer(struct binder_alloc *alloc,
struct binder_buffer *buffer, struct binder_buffer *buffer,
binder_size_t buffer_offset, binder_size_t buffer_offset,
void *src, void *src,
size_t bytes); size_t bytes);
void binder_alloc_copy_from_buffer(struct binder_alloc *alloc, int binder_alloc_copy_from_buffer(struct binder_alloc *alloc,
void *dest, void *dest,
struct binder_buffer *buffer, struct binder_buffer *buffer,
binder_size_t buffer_offset, binder_size_t buffer_offset,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment