Skip to content

Commit

Permalink
binder: return errors from buffer copy functions
Browse files Browse the repository at this point in the history 
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@android.com>
Reported-by: syzbot+3ae18325f96190606754@syzkaller.appspotmail.com
Fixes: bde4a19 ("binder: use userspace pointer as base of buffer space")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
@gregkh
Todd Kjos authored and gregkh committed Jul 1, 2019
1 parent 25c7eab commit bb4a2e4
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 93 deletions.
153 changes: 92 additions & 61 deletions drivers/android/binder.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand All @@ -3262,31 +3281,34 @@ 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: {
struct flat_binder_object *fp;

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: {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
44 changes: 23 additions & 21 deletions drivers/android/binder_alloc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

22 changes: 11 additions & 11 deletions drivers/android/binder_alloc.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

0 comments on commit bb4a2e4

Please sign in to comment.