From: Todd Kjos tkjos@android.com
mainline inclusion from mainline-v5.3-rc1 commit bb4a2e48d5100ed3ff614df158a636bca3c6bf9f category: bugfix bugzilla: 188431, https://gitee.com/src-openeuler/kernel/issues/I6DKVG CVE: CVE-2023-20938
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
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: Li Huafei lihuafei1@huawei.com Reviewed-by: Zheng Yejian zhengyejian1@huawei.com Reviewed-by: Wang Weiyang wangweiyang2@huawei.com Signed-off-by: Yongqiang Liu liuyongqiang13@huawei.com --- 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 7f37f493715d..3dcf28177ce6 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2066,10 +2066,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; @@ -2138,8 +2137,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; @@ -2219,10 +2220,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); } @@ -2310,15 +2313,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); @@ -2441,15 +2444,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: @@ -2688,11 +2692,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; } @@ -2745,8 +2750,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; } @@ -3163,15 +3172,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; } @@ -3237,11 +3251,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) { @@ -3265,15 +3284,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: { @@ -3281,15 +3302,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: { @@ -3299,16 +3321,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; @@ -3396,15 +3419,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; @@ -4143,20 +4167,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 f04dc5976751..ebdf714d3f2d 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -1135,15 +1135,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; @@ -1171,24 +1172,25 @@ 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 b60d161b7a7a..9a51b72624ae 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -168,17 +168,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 */