Home Home > GIT Browse
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJiri Slaby <jslaby@suse.cz>2019-07-14 11:33:41 +0200
committerJiri Slaby <jslaby@suse.cz>2019-07-14 11:33:52 +0200
commitf21448d2c990a3a55f91412dc64cb049612a99b8 (patch)
tree97150573102a92349ee2eb59a7fc24bbeb736935
parente6e4ac402cd7cb377779b82e34946c4dfeb32d4c (diff)
binder: return errors from buffer copy functions (bnc#1012628).
-rw-r--r--patches.kernel.org/5.2.1-042-binder-return-errors-from-buffer-copy-functions.patch432
-rw-r--r--series.conf1
2 files changed, 433 insertions, 0 deletions
diff --git a/patches.kernel.org/5.2.1-042-binder-return-errors-from-buffer-copy-functions.patch b/patches.kernel.org/5.2.1-042-binder-return-errors-from-buffer-copy-functions.patch
new file mode 100644
index 0000000000..a7b6db7591
--- /dev/null
+++ b/patches.kernel.org/5.2.1-042-binder-return-errors-from-buffer-copy-functions.patch
@@ -0,0 +1,432 @@
+From: Todd Kjos <tkjos@android.com>
+Date: Fri, 28 Jun 2019 09:50:12 -0700
+Subject: [PATCH] binder: return errors from buffer copy functions
+References: bnc#1012628
+Patch-mainline: 5.2.1
+Git-commit: bb4a2e48d5100ed3ff614df158a636bca3c6bf9f
+
+commit bb4a2e48d5100ed3ff614df158a636bca3c6bf9f upstream.
+
+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: bde4a19fc04f ("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>
+Signed-off-by: Jiri Slaby <jslaby@suse.cz>
+---
+ 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 8bf039fdeb91..38a59a630cd4 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 ce5603c2291c..6d79a1b0d446 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 71bfa95f8e09..db9c1b984695 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
+
diff --git a/series.conf b/series.conf
index 2eb2720a44..cf328167ae 100644
--- a/series.conf
+++ b/series.conf
@@ -68,6 +68,7 @@
patches.kernel.org/5.2.1-039-HID-Add-another-Primax-PIXART-OEM-mouse-quirk.patch
patches.kernel.org/5.2.1-040-lkdtm-support-llvm-objcopy.patch
patches.kernel.org/5.2.1-041-binder-fix-memory-leak-in-error-path.patch
+ patches.kernel.org/5.2.1-042-binder-return-errors-from-buffer-copy-functions.patch
########################################################
# Build fixes that apply to the vanilla kernel too.