* [PATCH 5.10 1/6] binder: avoid potential data leakage when copying txn
2022-11-30 3:57 [PATCH 5.10 0/6] binder: backports for data leak and UAF Carlos Llamas
@ 2022-11-30 3:58 ` Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 2/6] binder: read pre-translated fds from sender buffer Carlos Llamas
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Carlos Llamas @ 2022-11-30 3:58 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Joel Fernandes, Christian Brauner,
Hridya Valsaraju, Suren Baghdasaryan, Brian Swetland
Cc: linux-kernel, kernel-team, Todd Kjos, Christian Brauner,
Carlos Llamas, Greg Kroah-Hartman
From: Todd Kjos <tkjos@google.com>
commit 6d98eb95b450a75adb4516a1d33652dc78d2b20c upstream.
Transactions are copied from the sender to the target
first and objects like BINDER_TYPE_PTR and BINDER_TYPE_FDA
are then fixed up. This means there is a short period where
the sender's version of these objects are visible to the
target prior to the fixups.
Instead of copying all of the data first, copy data only
after any needed fixups have been applied.
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Reviewed-by: Martijn Coenen <maco@android.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Link: https://lore.kernel.org/r/20211130185152.437403-3-tkjos@google.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[cmllamas: fix trivial merge conflict]
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder.c | 94 ++++++++++++++++++++++++++++++----------
1 file changed, 70 insertions(+), 24 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cfb1393a0891..58ab76b7a787 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2008,15 +2008,21 @@ static void binder_cleanup_transaction(struct binder_transaction *t,
/**
* binder_get_object() - gets object and checks for valid metadata
* @proc: binder_proc owning the buffer
+ * @u: sender's user pointer to base of buffer
* @buffer: binder_buffer that we're parsing.
* @offset: offset in the @buffer at which to validate an object.
* @object: struct binder_object to read into
*
- * Return: If there's a valid metadata object at @offset in @buffer, the
+ * Copy the binder object at the given offset into @object. If @u is
+ * provided then the copy is from the sender's buffer. If not, then
+ * it is copied from the target's @buffer.
+ *
+ * Return: If there's a valid metadata object at @offset, the
* size of that object. Otherwise, it returns zero. The object
* is read into the struct binder_object pointed to by @object.
*/
static size_t binder_get_object(struct binder_proc *proc,
+ const void __user *u,
struct binder_buffer *buffer,
unsigned long offset,
struct binder_object *object)
@@ -2026,10 +2032,16 @@ static size_t binder_get_object(struct binder_proc *proc,
size_t object_size = 0;
read_size = min_t(size_t, sizeof(*object), buffer->data_size - offset);
- if (offset > buffer->data_size || read_size < sizeof(*hdr) ||
- binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
- offset, read_size))
+ if (offset > buffer->data_size || read_size < sizeof(*hdr))
return 0;
+ if (u) {
+ if (copy_from_user(object, u + offset, read_size))
+ return 0;
+ } else {
+ if (binder_alloc_copy_from_buffer(&proc->alloc, object, buffer,
+ offset, read_size))
+ return 0;
+ }
/* Ok, now see if we read a complete object. */
hdr = &object->hdr;
@@ -2102,7 +2114,7 @@ static struct binder_buffer_object *binder_validate_ptr(
b, buffer_offset,
sizeof(object_offset)))
return NULL;
- object_size = binder_get_object(proc, b, object_offset, object);
+ object_size = binder_get_object(proc, NULL, b, object_offset, object);
if (!object_size || object->hdr.type != BINDER_TYPE_PTR)
return NULL;
if (object_offsetp)
@@ -2167,7 +2179,8 @@ static bool binder_validate_fixup(struct binder_proc *proc,
unsigned long buffer_offset;
struct binder_object last_object;
struct binder_buffer_object *last_bbo;
- size_t object_size = binder_get_object(proc, b, last_obj_offset,
+ size_t object_size = binder_get_object(proc, NULL, b,
+ last_obj_offset,
&last_object);
if (object_size != sizeof(*last_bbo))
return false;
@@ -2282,7 +2295,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
if (!binder_alloc_copy_from_buffer(&proc->alloc, &object_offset,
buffer, buffer_offset,
sizeof(object_offset)))
- object_size = binder_get_object(proc, buffer,
+ object_size = binder_get_object(proc, NULL, buffer,
object_offset, &object);
if (object_size == 0) {
pr_err("transaction release %d bad object at offset %lld, size %zd\n",
@@ -2848,6 +2861,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_size_t off_start_offset, off_end_offset;
binder_size_t off_min;
binder_size_t sg_buf_offset, sg_buf_end_offset;
+ binder_size_t user_offset = 0;
struct binder_proc *target_proc = NULL;
struct binder_thread *target_thread = NULL;
struct binder_node *target_node = NULL;
@@ -2862,6 +2876,8 @@ static void binder_transaction(struct binder_proc *proc,
int t_debug_id = atomic_inc_return(&binder_last_id);
char *secctx = NULL;
u32 secctx_sz = 0;
+ const void __user *user_buffer = (const void __user *)
+ (uintptr_t)tr->data.ptr.buffer;
e = binder_transaction_log_add(&binder_transaction_log);
e->debug_id = t_debug_id;
@@ -3173,19 +3189,6 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer->clear_on_free = !!(t->flags & TF_CLEAR_BUF);
trace_binder_transaction_alloc_buf(t->buffer);
- if (binder_alloc_copy_user_to_buffer(
- &target_proc->alloc,
- t->buffer, 0,
- (const void __user *)
- (uintptr_t)tr->data.ptr.buffer,
- tr->data_size)) {
- binder_user_error("%d:%d got transaction with invalid data ptr\n",
- proc->pid, thread->pid);
- return_error = BR_FAILED_REPLY;
- return_error_param = -EFAULT;
- return_error_line = __LINE__;
- goto err_copy_data_failed;
- }
if (binder_alloc_copy_user_to_buffer(
&target_proc->alloc,
t->buffer,
@@ -3230,6 +3233,7 @@ static void binder_transaction(struct binder_proc *proc,
size_t object_size;
struct binder_object object;
binder_size_t object_offset;
+ binder_size_t copy_size;
if (binder_alloc_copy_from_buffer(&target_proc->alloc,
&object_offset,
@@ -3241,8 +3245,27 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_offset;
}
- object_size = binder_get_object(target_proc, t->buffer,
- object_offset, &object);
+
+ /*
+ * Copy the source user buffer up to the next object
+ * that will be processed.
+ */
+ copy_size = object_offset - user_offset;
+ if (copy_size && (user_offset > object_offset ||
+ binder_alloc_copy_user_to_buffer(
+ &target_proc->alloc,
+ t->buffer, user_offset,
+ user_buffer + user_offset,
+ copy_size))) {
+ binder_user_error("%d:%d got transaction with invalid data ptr\n",
+ proc->pid, thread->pid);
+ return_error = BR_FAILED_REPLY;
+ return_error_param = -EFAULT;
+ return_error_line = __LINE__;
+ goto err_copy_data_failed;
+ }
+ object_size = binder_get_object(target_proc, user_buffer,
+ t->buffer, object_offset, &object);
if (object_size == 0 || object_offset < off_min) {
binder_user_error("%d:%d got transaction with invalid offset (%lld, min %lld max %lld) or object.\n",
proc->pid, thread->pid,
@@ -3254,6 +3277,11 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_offset;
}
+ /*
+ * Set offset to the next buffer fragment to be
+ * copied
+ */
+ user_offset = object_offset + object_size;
hdr = &object.hdr;
off_min = object_offset + object_size;
@@ -3349,9 +3377,14 @@ static void binder_transaction(struct binder_proc *proc,
}
ret = binder_translate_fd_array(fda, parent, t, thread,
in_reply_to);
- if (ret < 0) {
+ if (!ret)
+ ret = binder_alloc_copy_to_buffer(&target_proc->alloc,
+ t->buffer,
+ object_offset,
+ fda, sizeof(*fda));
+ if (ret) {
return_error = BR_FAILED_REPLY;
- return_error_param = ret;
+ return_error_param = ret > 0 ? -EINVAL : ret;
return_error_line = __LINE__;
goto err_translate_failed;
}
@@ -3421,6 +3454,19 @@ static void binder_transaction(struct binder_proc *proc,
goto err_bad_object_type;
}
}
+ /* Done processing objects, copy the rest of the buffer */
+ if (binder_alloc_copy_user_to_buffer(
+ &target_proc->alloc,
+ t->buffer, user_offset,
+ user_buffer + user_offset,
+ tr->data_size - user_offset)) {
+ binder_user_error("%d:%d got transaction with invalid data ptr\n",
+ proc->pid, thread->pid);
+ return_error = BR_FAILED_REPLY;
+ return_error_param = -EFAULT;
+ return_error_line = __LINE__;
+ goto err_copy_data_failed;
+ }
tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE;
t->work.type = BINDER_WORK_TRANSACTION;
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5.10 2/6] binder: read pre-translated fds from sender buffer
2022-11-30 3:57 [PATCH 5.10 0/6] binder: backports for data leak and UAF Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 1/6] binder: avoid potential data leakage when copying txn Carlos Llamas
@ 2022-11-30 3:58 ` Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 3/6] binder: defer copies of pre-patched txn data Carlos Llamas
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Carlos Llamas @ 2022-11-30 3:58 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Joel Fernandes, Christian Brauner,
Hridya Valsaraju, Suren Baghdasaryan
Cc: linux-kernel, kernel-team, Todd Kjos, Christian Brauner,
Carlos Llamas
From: Todd Kjos <tkjos@google.com>
commit 656e01f3ab54afe71bed066996fc2640881e1220 upstream.
This patch is to prepare for an up coming patch where we read
pre-translated fds from the sender buffer and translate them before
copying them to the target. It does not change run time.
The patch adds two new parameters to binder_translate_fd_array() to
hold the sender buffer and sender buffer parent. These parameters let
us call copy_from_user() directly from the sender instead of using
binder_alloc_copy_from_buffer() to copy from the target. Also the patch
adds some new alignment checks. Previously the alignment checks would
have been done in a different place, but this lets us print more
useful error messages.
Reviewed-by: Martijn Coenen <maco@android.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Link: https://lore.kernel.org/r/20211130185152.437403-4-tkjos@google.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder.c | 39 ++++++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 58ab76b7a787..4abb621300ec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2634,15 +2634,17 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
}
static int binder_translate_fd_array(struct binder_fd_array_object *fda,
+ const void __user *sender_ubuffer,
struct binder_buffer_object *parent,
+ struct binder_buffer_object *sender_uparent,
struct binder_transaction *t,
struct binder_thread *thread,
struct binder_transaction *in_reply_to)
{
binder_size_t fdi, fd_buf_size;
binder_size_t fda_offset;
+ const void __user *sender_ufda_base;
struct binder_proc *proc = thread->proc;
- struct binder_proc *target_proc = t->to_proc;
fd_buf_size = sizeof(u32) * fda->num_fds;
if (fda->num_fds >= SIZE_MAX / sizeof(u32)) {
@@ -2666,7 +2668,10 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
*/
fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) +
fda->parent_offset;
- if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32))) {
+ sender_ufda_base = (void __user *)sender_uparent->buffer + fda->parent_offset;
+
+ if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32)) ||
+ !IS_ALIGNED((unsigned long)sender_ufda_base, sizeof(u32))) {
binder_user_error("%d:%d parent offset not aligned correctly.\n",
proc->pid, thread->pid);
return -EINVAL;
@@ -2675,10 +2680,9 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
u32 fd;
int ret;
binder_size_t offset = fda_offset + fdi * sizeof(fd);
+ binder_size_t sender_uoffset = fdi * sizeof(fd);
- ret = binder_alloc_copy_from_buffer(&target_proc->alloc,
- &fd, t->buffer,
- offset, sizeof(fd));
+ ret = copy_from_user(&fd, sender_ufda_base + sender_uoffset, sizeof(fd));
if (!ret)
ret = binder_translate_fd(fd, offset, t, thread,
in_reply_to);
@@ -3344,6 +3348,8 @@ static void binder_transaction(struct binder_proc *proc,
case BINDER_TYPE_FDA: {
struct binder_object ptr_object;
binder_size_t parent_offset;
+ struct binder_object user_object;
+ size_t user_parent_size;
struct binder_fd_array_object *fda =
to_binder_fd_array_object(hdr);
size_t num_valid = (buffer_offset - off_start_offset) /
@@ -3375,8 +3381,27 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_parent;
}
- ret = binder_translate_fd_array(fda, parent, t, thread,
- in_reply_to);
+ /*
+ * We need to read the user version of the parent
+ * object to get the original user offset
+ */
+ user_parent_size =
+ binder_get_object(proc, user_buffer, t->buffer,
+ parent_offset, &user_object);
+ if (user_parent_size != sizeof(user_object.bbo)) {
+ binder_user_error("%d:%d invalid ptr object size: %zd vs %zd\n",
+ proc->pid, thread->pid,
+ user_parent_size,
+ sizeof(user_object.bbo));
+ return_error = BR_FAILED_REPLY;
+ return_error_param = -EINVAL;
+ return_error_line = __LINE__;
+ goto err_bad_parent;
+ }
+ ret = binder_translate_fd_array(fda, user_buffer,
+ parent,
+ &user_object.bbo, t,
+ thread, in_reply_to);
if (!ret)
ret = binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer,
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5.10 3/6] binder: defer copies of pre-patched txn data
2022-11-30 3:57 [PATCH 5.10 0/6] binder: backports for data leak and UAF Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 1/6] binder: avoid potential data leakage when copying txn Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 2/6] binder: read pre-translated fds from sender buffer Carlos Llamas
@ 2022-11-30 3:58 ` Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 4/6] binder: fix pointer cast warning Carlos Llamas
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Carlos Llamas @ 2022-11-30 3:58 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Joel Fernandes, Christian Brauner,
Hridya Valsaraju, Suren Baghdasaryan
Cc: linux-kernel, kernel-team, Todd Kjos, Carlos Llamas
From: Todd Kjos <tkjos@google.com>
commit 09184ae9b5756cc469db6fd1d1cfdcffbf627c2d upstream.
BINDER_TYPE_PTR objects point to memory areas in the
source process to be copied into the target buffer
as part of a transaction. This implements a scatter-
gather model where non-contiguous memory in a source
process is "gathered" into a contiguous region in
the target buffer.
The data can include pointers that must be fixed up
to correctly point to the copied data. To avoid making
source process pointers visible to the target process,
this patch defers the copy until the fixups are known
and then copies and fixeups are done together.
There is a special case of BINDER_TYPE_FDA which applies
the fixup later in the target process context. In this
case the user data is skipped (so no untranslated fds
become visible to the target).
Reviewed-by: Martijn Coenen <maco@android.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Link: https://lore.kernel.org/r/20211130185152.437403-5-tkjos@google.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[cmllamas: fix trivial merge conflict]
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder.c | 299 +++++++++++++++++++++++++++++++++++----
1 file changed, 274 insertions(+), 25 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4abb621300ec..83c4501153b4 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2633,7 +2633,246 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
return ret;
}
-static int binder_translate_fd_array(struct binder_fd_array_object *fda,
+/**
+ * struct binder_ptr_fixup - data to be fixed-up in target buffer
+ * @offset offset in target buffer to fixup
+ * @skip_size bytes to skip in copy (fixup will be written later)
+ * @fixup_data data to write at fixup offset
+ * @node list node
+ *
+ * This is used for the pointer fixup list (pf) which is created and consumed
+ * during binder_transaction() and is only accessed locally. No
+ * locking is necessary.
+ *
+ * The list is ordered by @offset.
+ */
+struct binder_ptr_fixup {
+ binder_size_t offset;
+ size_t skip_size;
+ binder_uintptr_t fixup_data;
+ struct list_head node;
+};
+
+/**
+ * struct binder_sg_copy - scatter-gather data to be copied
+ * @offset offset in target buffer
+ * @sender_uaddr user address in source buffer
+ * @length bytes to copy
+ * @node list node
+ *
+ * This is used for the sg copy list (sgc) which is created and consumed
+ * during binder_transaction() and is only accessed locally. No
+ * locking is necessary.
+ *
+ * The list is ordered by @offset.
+ */
+struct binder_sg_copy {
+ binder_size_t offset;
+ const void __user *sender_uaddr;
+ size_t length;
+ struct list_head node;
+};
+
+/**
+ * binder_do_deferred_txn_copies() - copy and fixup scatter-gather data
+ * @alloc: binder_alloc associated with @buffer
+ * @buffer: binder buffer in target process
+ * @sgc_head: list_head of scatter-gather copy list
+ * @pf_head: list_head of pointer fixup list
+ *
+ * Processes all elements of @sgc_head, applying fixups from @pf_head
+ * and copying the scatter-gather data from the source process' user
+ * buffer to the target's buffer. It is expected that the list creation
+ * and processing all occurs during binder_transaction() so these lists
+ * are only accessed in local context.
+ *
+ * Return: 0=success, else -errno
+ */
+static int binder_do_deferred_txn_copies(struct binder_alloc *alloc,
+ struct binder_buffer *buffer,
+ struct list_head *sgc_head,
+ struct list_head *pf_head)
+{
+ int ret = 0;
+ struct binder_sg_copy *sgc, *tmpsgc;
+ struct binder_ptr_fixup *pf =
+ list_first_entry_or_null(pf_head, struct binder_ptr_fixup,
+ node);
+
+ list_for_each_entry_safe(sgc, tmpsgc, sgc_head, node) {
+ size_t bytes_copied = 0;
+
+ while (bytes_copied < sgc->length) {
+ size_t copy_size;
+ size_t bytes_left = sgc->length - bytes_copied;
+ size_t offset = sgc->offset + bytes_copied;
+
+ /*
+ * We copy up to the fixup (pointed to by pf)
+ */
+ copy_size = pf ? min(bytes_left, (size_t)pf->offset - offset)
+ : bytes_left;
+ if (!ret && copy_size)
+ ret = binder_alloc_copy_user_to_buffer(
+ alloc, buffer,
+ offset,
+ sgc->sender_uaddr + bytes_copied,
+ copy_size);
+ bytes_copied += copy_size;
+ if (copy_size != bytes_left) {
+ BUG_ON(!pf);
+ /* we stopped at a fixup offset */
+ if (pf->skip_size) {
+ /*
+ * we are just skipping. This is for
+ * BINDER_TYPE_FDA where the translated
+ * fds will be fixed up when we get
+ * to target context.
+ */
+ bytes_copied += pf->skip_size;
+ } else {
+ /* apply the fixup indicated by pf */
+ if (!ret)
+ ret = binder_alloc_copy_to_buffer(
+ alloc, buffer,
+ pf->offset,
+ &pf->fixup_data,
+ sizeof(pf->fixup_data));
+ bytes_copied += sizeof(pf->fixup_data);
+ }
+ list_del(&pf->node);
+ kfree(pf);
+ pf = list_first_entry_or_null(pf_head,
+ struct binder_ptr_fixup, node);
+ }
+ }
+ list_del(&sgc->node);
+ kfree(sgc);
+ }
+ BUG_ON(!list_empty(pf_head));
+ BUG_ON(!list_empty(sgc_head));
+
+ return ret > 0 ? -EINVAL : ret;
+}
+
+/**
+ * binder_cleanup_deferred_txn_lists() - free specified lists
+ * @sgc_head: list_head of scatter-gather copy list
+ * @pf_head: list_head of pointer fixup list
+ *
+ * Called to clean up @sgc_head and @pf_head if there is an
+ * error.
+ */
+static void binder_cleanup_deferred_txn_lists(struct list_head *sgc_head,
+ struct list_head *pf_head)
+{
+ struct binder_sg_copy *sgc, *tmpsgc;
+ struct binder_ptr_fixup *pf, *tmppf;
+
+ list_for_each_entry_safe(sgc, tmpsgc, sgc_head, node) {
+ list_del(&sgc->node);
+ kfree(sgc);
+ }
+ list_for_each_entry_safe(pf, tmppf, pf_head, node) {
+ list_del(&pf->node);
+ kfree(pf);
+ }
+}
+
+/**
+ * binder_defer_copy() - queue a scatter-gather buffer for copy
+ * @sgc_head: list_head of scatter-gather copy list
+ * @offset: binder buffer offset in target process
+ * @sender_uaddr: user address in source process
+ * @length: bytes to copy
+ *
+ * Specify a scatter-gather block to be copied. The actual copy must
+ * be deferred until all the needed fixups are identified and queued.
+ * Then the copy and fixups are done together so un-translated values
+ * from the source are never visible in the target buffer.
+ *
+ * We are guaranteed that repeated calls to this function will have
+ * monotonically increasing @offset values so the list will naturally
+ * be ordered.
+ *
+ * Return: 0=success, else -errno
+ */
+static int binder_defer_copy(struct list_head *sgc_head, binder_size_t offset,
+ const void __user *sender_uaddr, size_t length)
+{
+ struct binder_sg_copy *bc = kzalloc(sizeof(*bc), GFP_KERNEL);
+
+ if (!bc)
+ return -ENOMEM;
+
+ bc->offset = offset;
+ bc->sender_uaddr = sender_uaddr;
+ bc->length = length;
+ INIT_LIST_HEAD(&bc->node);
+
+ /*
+ * We are guaranteed that the deferred copies are in-order
+ * so just add to the tail.
+ */
+ list_add_tail(&bc->node, sgc_head);
+
+ return 0;
+}
+
+/**
+ * binder_add_fixup() - queue a fixup to be applied to sg copy
+ * @pf_head: list_head of binder ptr fixup list
+ * @offset: binder buffer offset in target process
+ * @fixup: bytes to be copied for fixup
+ * @skip_size: bytes to skip when copying (fixup will be applied later)
+ *
+ * Add the specified fixup to a list ordered by @offset. When copying
+ * the scatter-gather buffers, the fixup will be copied instead of
+ * data from the source buffer. For BINDER_TYPE_FDA fixups, the fixup
+ * will be applied later (in target process context), so we just skip
+ * the bytes specified by @skip_size. If @skip_size is 0, we copy the
+ * value in @fixup.
+ *
+ * This function is called *mostly* in @offset order, but there are
+ * exceptions. Since out-of-order inserts are relatively uncommon,
+ * we insert the new element by searching backward from the tail of
+ * the list.
+ *
+ * Return: 0=success, else -errno
+ */
+static int binder_add_fixup(struct list_head *pf_head, binder_size_t offset,
+ binder_uintptr_t fixup, size_t skip_size)
+{
+ struct binder_ptr_fixup *pf = kzalloc(sizeof(*pf), GFP_KERNEL);
+ struct binder_ptr_fixup *tmppf;
+
+ if (!pf)
+ return -ENOMEM;
+
+ pf->offset = offset;
+ pf->fixup_data = fixup;
+ pf->skip_size = skip_size;
+ INIT_LIST_HEAD(&pf->node);
+
+ /* Fixups are *mostly* added in-order, but there are some
+ * exceptions. Look backwards through list for insertion point.
+ */
+ list_for_each_entry_reverse(tmppf, pf_head, node) {
+ if (tmppf->offset < pf->offset) {
+ list_add(&pf->node, &tmppf->node);
+ return 0;
+ }
+ }
+ /*
+ * if we get here, then the new offset is the lowest so
+ * insert at the head
+ */
+ list_add(&pf->node, pf_head);
+ return 0;
+}
+
+static int binder_translate_fd_array(struct list_head *pf_head,
+ struct binder_fd_array_object *fda,
const void __user *sender_ubuffer,
struct binder_buffer_object *parent,
struct binder_buffer_object *sender_uparent,
@@ -2645,6 +2884,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
binder_size_t fda_offset;
const void __user *sender_ufda_base;
struct binder_proc *proc = thread->proc;
+ int ret;
fd_buf_size = sizeof(u32) * fda->num_fds;
if (fda->num_fds >= SIZE_MAX / sizeof(u32)) {
@@ -2676,9 +2916,12 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
proc->pid, thread->pid);
return -EINVAL;
}
+ ret = binder_add_fixup(pf_head, fda_offset, 0, fda->num_fds * sizeof(u32));
+ if (ret)
+ return ret;
+
for (fdi = 0; fdi < fda->num_fds; fdi++) {
u32 fd;
- int ret;
binder_size_t offset = fda_offset + fdi * sizeof(fd);
binder_size_t sender_uoffset = fdi * sizeof(fd);
@@ -2692,7 +2935,8 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
return 0;
}
-static int binder_fixup_parent(struct binder_transaction *t,
+static int binder_fixup_parent(struct list_head *pf_head,
+ struct binder_transaction *t,
struct binder_thread *thread,
struct binder_buffer_object *bp,
binder_size_t off_start_offset,
@@ -2738,14 +2982,7 @@ static int binder_fixup_parent(struct binder_transaction *t,
}
buffer_offset = bp->parent_offset +
(uintptr_t)parent->buffer - (uintptr_t)b->user_data;
- 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;
+ return binder_add_fixup(pf_head, buffer_offset, bp->buffer, 0);
}
/**
@@ -2880,8 +3117,12 @@ static void binder_transaction(struct binder_proc *proc,
int t_debug_id = atomic_inc_return(&binder_last_id);
char *secctx = NULL;
u32 secctx_sz = 0;
+ struct list_head sgc_head;
+ struct list_head pf_head;
const void __user *user_buffer = (const void __user *)
(uintptr_t)tr->data.ptr.buffer;
+ INIT_LIST_HEAD(&sgc_head);
+ INIT_LIST_HEAD(&pf_head);
e = binder_transaction_log_add(&binder_transaction_log);
e->debug_id = t_debug_id;
@@ -3398,8 +3639,8 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_parent;
}
- ret = binder_translate_fd_array(fda, user_buffer,
- parent,
+ ret = binder_translate_fd_array(&pf_head, fda,
+ user_buffer, parent,
&user_object.bbo, t,
thread, in_reply_to);
if (!ret)
@@ -3431,19 +3672,14 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_bad_offset;
}
- if (binder_alloc_copy_user_to_buffer(
- &target_proc->alloc,
- t->buffer,
- sg_buf_offset,
- (const void __user *)
- (uintptr_t)bp->buffer,
- bp->length)) {
- binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
- proc->pid, thread->pid);
- return_error_param = -EFAULT;
+ ret = binder_defer_copy(&sgc_head, sg_buf_offset,
+ (const void __user *)(uintptr_t)bp->buffer,
+ bp->length);
+ if (ret) {
return_error = BR_FAILED_REPLY;
+ return_error_param = ret;
return_error_line = __LINE__;
- goto err_copy_data_failed;
+ goto err_translate_failed;
}
/* Fixup buffer pointer to target proc address space */
bp->buffer = (uintptr_t)
@@ -3452,7 +3688,8 @@ static void binder_transaction(struct binder_proc *proc,
num_valid = (buffer_offset - off_start_offset) /
sizeof(binder_size_t);
- ret = binder_fixup_parent(t, thread, bp,
+ ret = binder_fixup_parent(&pf_head, t,
+ thread, bp,
off_start_offset,
num_valid,
last_fixup_obj_off,
@@ -3492,6 +3729,17 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_copy_data_failed;
}
+
+ ret = binder_do_deferred_txn_copies(&target_proc->alloc, t->buffer,
+ &sgc_head, &pf_head);
+ if (ret) {
+ binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
+ proc->pid, thread->pid);
+ return_error = BR_FAILED_REPLY;
+ return_error_param = ret;
+ return_error_line = __LINE__;
+ goto err_copy_data_failed;
+ }
tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE;
t->work.type = BINDER_WORK_TRANSACTION;
@@ -3558,6 +3806,7 @@ static void binder_transaction(struct binder_proc *proc,
err_bad_offset:
err_bad_parent:
err_copy_data_failed:
+ binder_cleanup_deferred_txn_lists(&sgc_head, &pf_head);
binder_free_txn_fixups(t);
trace_binder_transaction_failed_buffer_release(t->buffer);
binder_transaction_buffer_release(target_proc, NULL, t->buffer,
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5.10 4/6] binder: fix pointer cast warning
2022-11-30 3:57 [PATCH 5.10 0/6] binder: backports for data leak and UAF Carlos Llamas
` (2 preceding siblings ...)
2022-11-30 3:58 ` [PATCH 5.10 3/6] binder: defer copies of pre-patched txn data Carlos Llamas
@ 2022-11-30 3:58 ` Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 5/6] binder: Address corner cases in deferred copy and fixup Carlos Llamas
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Carlos Llamas @ 2022-11-30 3:58 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Joel Fernandes, Christian Brauner,
Hridya Valsaraju, Suren Baghdasaryan
Cc: linux-kernel, kernel-team, Arnd Bergmann, Todd Kjos, Randy Dunlap,
Christian Brauner, Carlos Llamas
From: Arnd Bergmann <arnd@arndb.de>
commit 9a0a930fe2535a76ad70d3f43caeccf0d86a3009 upstream.
binder_uintptr_t is not the same as uintptr_t, so converting it into a
pointer requires a second cast:
drivers/android/binder.c: In function 'binder_translate_fd_array':
drivers/android/binder.c:2511:28: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
2511 | sender_ufda_base = (void __user *)sender_uparent->buffer + fda->parent_offset;
| ^
Fixes: 656e01f3ab54 ("binder: read pre-translated fds from sender buffer")
Acked-by: Todd Kjos <tkjos@google.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/r/20211207122448.1185769-1-arnd@kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 83c4501153b4..398ce65b578b 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2908,7 +2908,8 @@ static int binder_translate_fd_array(struct list_head *pf_head,
*/
fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) +
fda->parent_offset;
- sender_ufda_base = (void __user *)sender_uparent->buffer + fda->parent_offset;
+ sender_ufda_base = (void __user *)(uintptr_t)sender_uparent->buffer +
+ fda->parent_offset;
if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32)) ||
!IS_ALIGNED((unsigned long)sender_ufda_base, sizeof(u32))) {
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5.10 5/6] binder: Address corner cases in deferred copy and fixup
2022-11-30 3:57 [PATCH 5.10 0/6] binder: backports for data leak and UAF Carlos Llamas
` (3 preceding siblings ...)
2022-11-30 3:58 ` [PATCH 5.10 4/6] binder: fix pointer cast warning Carlos Llamas
@ 2022-11-30 3:58 ` Carlos Llamas
2022-11-30 3:58 ` [PATCH 5.10 6/6] binder: Gracefully handle BINDER_TYPE_FDA objects with num_fds=0 Carlos Llamas
2022-11-30 12:42 ` [PATCH 5.10 0/6] binder: backports for data leak and UAF Greg Kroah-Hartman
6 siblings, 0 replies; 10+ messages in thread
From: Carlos Llamas @ 2022-11-30 3:58 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Joel Fernandes, Christian Brauner,
Hridya Valsaraju, Suren Baghdasaryan
Cc: linux-kernel, kernel-team, Alessandro Astone, Todd Kjos,
Carlos Llamas
From: Alessandro Astone <ales.astone@gmail.com>
commit 2d1746e3fda0c3612143d7c06f8e1d1830c13e23 upstream.
When handling BINDER_TYPE_FDA object we are pushing a parent fixup
with a certain skip_size but no scatter-gather copy object, since
the copy is handled standalone.
If BINDER_TYPE_FDA is the last children the scatter-gather copy
loop will never stop to skip it, thus we are left with an item in
the parent fixup list. This will trigger the BUG_ON().
This is reproducible in android when playing a video.
We receive a transaction that looks like this:
obj[0] BINDER_TYPE_PTR, parent
obj[1] BINDER_TYPE_PTR, child
obj[2] BINDER_TYPE_PTR, child
obj[3] BINDER_TYPE_FDA, child
Fixes: 09184ae9b575 ("binder: defer copies of pre-patched txn data")
Acked-by: Todd Kjos <tkjos@google.com>
Cc: stable <stable@kernel.org>
Signed-off-by: Alessandro Astone <ales.astone@gmail.com>
Link: https://lore.kernel.org/r/20220415120015.52684-2-ales.astone@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 398ce65b578b..1ba8a98094b4 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2695,6 +2695,7 @@ static int binder_do_deferred_txn_copies(struct binder_alloc *alloc,
{
int ret = 0;
struct binder_sg_copy *sgc, *tmpsgc;
+ struct binder_ptr_fixup *tmppf;
struct binder_ptr_fixup *pf =
list_first_entry_or_null(pf_head, struct binder_ptr_fixup,
node);
@@ -2749,7 +2750,11 @@ static int binder_do_deferred_txn_copies(struct binder_alloc *alloc,
list_del(&sgc->node);
kfree(sgc);
}
- BUG_ON(!list_empty(pf_head));
+ list_for_each_entry_safe(pf, tmppf, pf_head, node) {
+ BUG_ON(pf->skip_size == 0);
+ list_del(&pf->node);
+ kfree(pf);
+ }
BUG_ON(!list_empty(sgc_head));
return ret > 0 ? -EINVAL : ret;
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5.10 6/6] binder: Gracefully handle BINDER_TYPE_FDA objects with num_fds=0
2022-11-30 3:57 [PATCH 5.10 0/6] binder: backports for data leak and UAF Carlos Llamas
` (4 preceding siblings ...)
2022-11-30 3:58 ` [PATCH 5.10 5/6] binder: Address corner cases in deferred copy and fixup Carlos Llamas
@ 2022-11-30 3:58 ` Carlos Llamas
2022-11-30 12:42 ` [PATCH 5.10 0/6] binder: backports for data leak and UAF Greg Kroah-Hartman
6 siblings, 0 replies; 10+ messages in thread
From: Carlos Llamas @ 2022-11-30 3:58 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Joel Fernandes, Christian Brauner,
Hridya Valsaraju, Suren Baghdasaryan
Cc: linux-kernel, kernel-team, Alessandro Astone, Todd Kjos,
Carlos Llamas
From: Alessandro Astone <ales.astone@gmail.com>
commit ef38de9217a04c9077629a24652689d8fdb4c6c6 upstream.
Some android userspace is sending BINDER_TYPE_FDA objects with
num_fds=0. Like the previous patch, this is reproducible when
playing a video.
Before commit 09184ae9b575 BINDER_TYPE_FDA objects with num_fds=0
were 'correctly handled', as in no fixup was performed.
After commit 09184ae9b575 we aggregate fixup and skip regions in
binder_ptr_fixup structs and distinguish between the two by using
the skip_size field: if it's 0, then it's a fixup, otherwise skip.
When processing BINDER_TYPE_FDA objects with num_fds=0 we add a
skip region of skip_size=0, and this causes issues because now
binder_do_deferred_txn_copies will think this was a fixup region.
To address that, return early from binder_translate_fd_array to
avoid adding an empty skip region.
Fixes: 09184ae9b575 ("binder: defer copies of pre-patched txn data")
Acked-by: Todd Kjos <tkjos@google.com>
Cc: stable <stable@kernel.org>
Signed-off-by: Alessandro Astone <ales.astone@gmail.com>
Link: https://lore.kernel.org/r/20220415120015.52684-1-ales.astone@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 1ba8a98094b4..4473adef2f5a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2891,6 +2891,9 @@ static int binder_translate_fd_array(struct list_head *pf_head,
struct binder_proc *proc = thread->proc;
int ret;
+ if (fda->num_fds == 0)
+ return 0;
+
fd_buf_size = sizeof(u32) * fda->num_fds;
if (fda->num_fds >= SIZE_MAX / sizeof(u32)) {
binder_user_error("%d:%d got transaction with invalid number of fds (%lld)\n",
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 5.10 0/6] binder: backports for data leak and UAF
2022-11-30 3:57 [PATCH 5.10 0/6] binder: backports for data leak and UAF Carlos Llamas
` (5 preceding siblings ...)
2022-11-30 3:58 ` [PATCH 5.10 6/6] binder: Gracefully handle BINDER_TYPE_FDA objects with num_fds=0 Carlos Llamas
@ 2022-11-30 12:42 ` Greg Kroah-Hartman
2022-11-30 16:12 ` Carlos Llamas
6 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2022-11-30 12:42 UTC (permalink / raw)
To: Carlos Llamas; +Cc: stable, linux-kernel, kernel-team, Zi Fan Tan, Todd Kjos
On Wed, Nov 30, 2022 at 03:57:59AM +0000, Carlos Llamas wrote:
> This series of backports consists of 3 main patches from Todd submitted
> upstream in [1]. The intention is to avoid untranslated data from the
> senders to be visible to the target processes. More details of this
> issue can be found in the same thread.
>
> Furthermore, Todd's patches also fix a use-after-free issue introduced
> by commit 32e9f56a96d8 ("binder: don't detect sender/target during
> buffer cleanup"). In which invalid userspace input causes unprocessed
> objects to be incorrectly released. Any subsequent references to these
> objects will trigger a UAF as noted by the following KASAN trace:
>
> [ 244.748468] ==================================================================
> [ 244.750486] BUG: KASAN: use-after-free in binder_ioctl+0xb88/0x32e0
> [ 244.751276] Read of size 8 at addr ffff67b1865bea58 by task poc/593
> [ 244.752074]
> [ 244.752725] CPU: 0 PID: 593 Comm: poc Not tainted 5.10.156 #1
> [ 244.753683] Hardware name: linux,dummy-virt (DT)
> [ 244.754717] Call trace:
> [ 244.755216] dump_backtrace+0x0/0x2a0
> [ 244.755836] show_stack+0x18/0x2c
> [ 244.756306] dump_stack+0xf8/0x164
> [ 244.756807] print_address_description.constprop.0+0x9c/0x538
> [ 244.757590] kasan_report+0x120/0x200
> [ 244.758236] __asan_load8+0xa0/0xc4
> [ 244.758756] binder_ioctl+0xb88/0x32e0
> [ 244.759283] __arm64_sys_ioctl+0xd4/0x120
> [ 244.759677] el0_svc_common.constprop.0+0xac/0x270
> [ 244.760184] do_el0_svc+0x38/0xa0
> [ 244.760540] el0_svc+0x1c/0x2c
> [ 244.760898] el0_sync_handler+0xe8/0x114
> [ 244.761419] el0_sync+0x180/0x1c0
>
> This second issue along with the reference to the commit fixing it was
> first reported by Zi Fan.
>
> The other 3 commits included in this series are simply upstream fixes
> for the main patches.
>
> I've tested this series applied to 5.10 and 5.4 which fixes the issues
> above as expected. So please pick these up for 5.10 and 5.4 stable.
>
> [1] https://lore.kernel.org/all/20211130185152.437403-1-tkjos@google.com/
>
> Thanks,
> Carlos Llamas
>
> Cc: Zi Fan Tan <zifantan@google.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
All now queued up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 5.10 0/6] binder: backports for data leak and UAF
2022-11-30 12:42 ` [PATCH 5.10 0/6] binder: backports for data leak and UAF Greg Kroah-Hartman
@ 2022-11-30 16:12 ` Carlos Llamas
2022-11-30 16:33 ` Greg Kroah-Hartman
0 siblings, 1 reply; 10+ messages in thread
From: Carlos Llamas @ 2022-11-30 16:12 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: stable, linux-kernel, kernel-team, Zi Fan Tan, Todd Kjos
On Wed, Nov 30, 2022 at 01:42:23PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 30, 2022 at 03:57:59AM +0000, Carlos Llamas wrote:
> > This series of backports consists of 3 main patches from Todd submitted
> > upstream in [1]. The intention is to avoid untranslated data from the
> > senders to be visible to the target processes. More details of this
> > issue can be found in the same thread.
> >
> > Furthermore, Todd's patches also fix a use-after-free issue introduced
> > by commit 32e9f56a96d8 ("binder: don't detect sender/target during
> > buffer cleanup"). In which invalid userspace input causes unprocessed
> > objects to be incorrectly released. Any subsequent references to these
> > objects will trigger a UAF as noted by the following KASAN trace:
> >
> > [ 244.748468] ==================================================================
> > [ 244.750486] BUG: KASAN: use-after-free in binder_ioctl+0xb88/0x32e0
> > [ 244.751276] Read of size 8 at addr ffff67b1865bea58 by task poc/593
> > [ 244.752074]
> > [ 244.752725] CPU: 0 PID: 593 Comm: poc Not tainted 5.10.156 #1
> > [ 244.753683] Hardware name: linux,dummy-virt (DT)
> > [ 244.754717] Call trace:
> > [ 244.755216] dump_backtrace+0x0/0x2a0
> > [ 244.755836] show_stack+0x18/0x2c
> > [ 244.756306] dump_stack+0xf8/0x164
> > [ 244.756807] print_address_description.constprop.0+0x9c/0x538
> > [ 244.757590] kasan_report+0x120/0x200
> > [ 244.758236] __asan_load8+0xa0/0xc4
> > [ 244.758756] binder_ioctl+0xb88/0x32e0
> > [ 244.759283] __arm64_sys_ioctl+0xd4/0x120
> > [ 244.759677] el0_svc_common.constprop.0+0xac/0x270
> > [ 244.760184] do_el0_svc+0x38/0xa0
> > [ 244.760540] el0_svc+0x1c/0x2c
> > [ 244.760898] el0_sync_handler+0xe8/0x114
> > [ 244.761419] el0_sync+0x180/0x1c0
> >
> > This second issue along with the reference to the commit fixing it was
> > first reported by Zi Fan.
> >
> > The other 3 commits included in this series are simply upstream fixes
> > for the main patches.
> >
> > I've tested this series applied to 5.10 and 5.4 which fixes the issues
> > above as expected. So please pick these up for 5.10 and 5.4 stable.
> >
> > [1] https://lore.kernel.org/all/20211130185152.437403-1-tkjos@google.com/
> >
> > Thanks,
> > Carlos Llamas
> >
> > Cc: Zi Fan Tan <zifantan@google.com>
> > Cc: Todd Kjos <tkjos@google.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> All now queued up, thanks.
>
> greg k-h
Thanks Greg. Could you also take this series for 5.4? I've also tested
the fixes for that release.
--
Carlos Llamas
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 5.10 0/6] binder: backports for data leak and UAF
2022-11-30 16:12 ` Carlos Llamas
@ 2022-11-30 16:33 ` Greg Kroah-Hartman
0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2022-11-30 16:33 UTC (permalink / raw)
To: Carlos Llamas; +Cc: stable, linux-kernel, kernel-team, Zi Fan Tan, Todd Kjos
On Wed, Nov 30, 2022 at 04:12:59PM +0000, Carlos Llamas wrote:
> On Wed, Nov 30, 2022 at 01:42:23PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 30, 2022 at 03:57:59AM +0000, Carlos Llamas wrote:
> > > This series of backports consists of 3 main patches from Todd submitted
> > > upstream in [1]. The intention is to avoid untranslated data from the
> > > senders to be visible to the target processes. More details of this
> > > issue can be found in the same thread.
> > >
> > > Furthermore, Todd's patches also fix a use-after-free issue introduced
> > > by commit 32e9f56a96d8 ("binder: don't detect sender/target during
> > > buffer cleanup"). In which invalid userspace input causes unprocessed
> > > objects to be incorrectly released. Any subsequent references to these
> > > objects will trigger a UAF as noted by the following KASAN trace:
> > >
> > > [ 244.748468] ==================================================================
> > > [ 244.750486] BUG: KASAN: use-after-free in binder_ioctl+0xb88/0x32e0
> > > [ 244.751276] Read of size 8 at addr ffff67b1865bea58 by task poc/593
> > > [ 244.752074]
> > > [ 244.752725] CPU: 0 PID: 593 Comm: poc Not tainted 5.10.156 #1
> > > [ 244.753683] Hardware name: linux,dummy-virt (DT)
> > > [ 244.754717] Call trace:
> > > [ 244.755216] dump_backtrace+0x0/0x2a0
> > > [ 244.755836] show_stack+0x18/0x2c
> > > [ 244.756306] dump_stack+0xf8/0x164
> > > [ 244.756807] print_address_description.constprop.0+0x9c/0x538
> > > [ 244.757590] kasan_report+0x120/0x200
> > > [ 244.758236] __asan_load8+0xa0/0xc4
> > > [ 244.758756] binder_ioctl+0xb88/0x32e0
> > > [ 244.759283] __arm64_sys_ioctl+0xd4/0x120
> > > [ 244.759677] el0_svc_common.constprop.0+0xac/0x270
> > > [ 244.760184] do_el0_svc+0x38/0xa0
> > > [ 244.760540] el0_svc+0x1c/0x2c
> > > [ 244.760898] el0_sync_handler+0xe8/0x114
> > > [ 244.761419] el0_sync+0x180/0x1c0
> > >
> > > This second issue along with the reference to the commit fixing it was
> > > first reported by Zi Fan.
> > >
> > > The other 3 commits included in this series are simply upstream fixes
> > > for the main patches.
> > >
> > > I've tested this series applied to 5.10 and 5.4 which fixes the issues
> > > above as expected. So please pick these up for 5.10 and 5.4 stable.
> > >
> > > [1] https://lore.kernel.org/all/20211130185152.437403-1-tkjos@google.com/
> > >
> > > Thanks,
> > > Carlos Llamas
> > >
> > > Cc: Zi Fan Tan <zifantan@google.com>
> > > Cc: Todd Kjos <tkjos@google.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > All now queued up, thanks.
> >
> > greg k-h
>
> Thanks Greg. Could you also take this series for 5.4? I've also tested
> the fixes for that release.
Sure, all now queued up there too.
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread