From b773f91910d78e22fd797edc079d88e7ec72174c Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Wed, 30 Nov 2022 17:32:55 +0100 Subject: [PATCH] 5.4-stable patches added patches: binder-address-corner-cases-in-deferred-copy-and-fixup.patch binder-avoid-potential-data-leakage-when-copying-txn.patch binder-defer-copies-of-pre-patched-txn-data.patch binder-fix-pointer-cast-warning.patch binder-gracefully-handle-binder_type_fda-objects-with-num_fds-0.patch binder-read-pre-translated-fds-from-sender-buffer.patch --- ...ner-cases-in-deferred-copy-and-fixup.patch | 61 +++ ...ential-data-leakage-when-copying-txn.patch | 231 ++++++++++ ...defer-copies-of-pre-patched-txn-data.patch | 425 ++++++++++++++++++ .../binder-fix-pointer-cast-warning.patch | 45 ++ ...nder_type_fda-objects-with-num_fds-0.patch | 53 +++ ...re-translated-fds-from-sender-buffer.patch | 120 +++++ queue-5.4/series | 6 + 7 files changed, 941 insertions(+) create mode 100644 queue-5.4/binder-address-corner-cases-in-deferred-copy-and-fixup.patch create mode 100644 queue-5.4/binder-avoid-potential-data-leakage-when-copying-txn.patch create mode 100644 queue-5.4/binder-defer-copies-of-pre-patched-txn-data.patch create mode 100644 queue-5.4/binder-fix-pointer-cast-warning.patch create mode 100644 queue-5.4/binder-gracefully-handle-binder_type_fda-objects-with-num_fds-0.patch create mode 100644 queue-5.4/binder-read-pre-translated-fds-from-sender-buffer.patch diff --git a/queue-5.4/binder-address-corner-cases-in-deferred-copy-and-fixup.patch b/queue-5.4/binder-address-corner-cases-in-deferred-copy-and-fixup.patch new file mode 100644 index 00000000000..af6fa279aa8 --- /dev/null +++ b/queue-5.4/binder-address-corner-cases-in-deferred-copy-and-fixup.patch @@ -0,0 +1,61 @@ +From foo@baz Wed Nov 30 05:32:15 PM CET 2022 +From: Carlos Llamas +Date: Wed, 30 Nov 2022 03:58:04 +0000 +Subject: binder: Address corner cases in deferred copy and fixup +To: stable@kernel.org, "Greg Kroah-Hartman" , "Arve Hjønnevåg" , "Todd Kjos" , "Martijn Coenen" , "Joel Fernandes" , "Christian Brauner" , "Hridya Valsaraju" , "Suren Baghdasaryan" +Cc: linux-kernel@vger.kernel.org, kernel-team@android.com, Alessandro Astone , Todd Kjos , Carlos Llamas +Message-ID: <20221130035805.1823970-6-cmllamas@google.com> + +From: Alessandro Astone + +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 +Cc: stable +Signed-off-by: Alessandro Astone +Link: https://lore.kernel.org/r/20220415120015.52684-2-ales.astone@gmail.com +Signed-off-by: Greg Kroah-Hartman +Signed-off-by: Carlos Llamas +Signed-off-by: Greg Kroah-Hartman +--- + drivers/android/binder.c | 7 ++++++- + 1 file changed, 6 insertions(+), 1 deletion(-) + +--- a/drivers/android/binder.c ++++ b/drivers/android/binder.c +@@ -2698,6 +2698,7 @@ static int binder_do_deferred_txn_copies + { + 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); +@@ -2752,7 +2753,11 @@ static int binder_do_deferred_txn_copies + 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; diff --git a/queue-5.4/binder-avoid-potential-data-leakage-when-copying-txn.patch b/queue-5.4/binder-avoid-potential-data-leakage-when-copying-txn.patch new file mode 100644 index 00000000000..0626e36d801 --- /dev/null +++ b/queue-5.4/binder-avoid-potential-data-leakage-when-copying-txn.patch @@ -0,0 +1,231 @@ +From foo@baz Wed Nov 30 05:32:15 PM CET 2022 +From: Carlos Llamas +Date: Wed, 30 Nov 2022 03:58:00 +0000 +Subject: binder: avoid potential data leakage when copying txn +To: stable@kernel.org, "Greg Kroah-Hartman" , "Arve Hjønnevåg" , "Todd Kjos" , "Martijn Coenen" , "Joel Fernandes" , "Christian Brauner" , "Hridya Valsaraju" , "Suren Baghdasaryan" , "Brian Swetland" +Cc: linux-kernel@vger.kernel.org, kernel-team@android.com, Todd Kjos , Christian Brauner , Carlos Llamas , Greg Kroah-Hartman +Message-ID: <20221130035805.1823970-2-cmllamas@google.com> + +From: Todd Kjos + +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 +Acked-by: Christian Brauner +Signed-off-by: Todd Kjos +Link: https://lore.kernel.org/r/20211130185152.437403-3-tkjos@google.com +Signed-off-by: Greg Kroah-Hartman +[cmllamas: fix trivial merge conflict] +Signed-off-by: Carlos Llamas +Signed-off-by: Greg Kroah-Hartman +--- + drivers/android/binder.c | 94 +++++++++++++++++++++++++++++++++++------------ + 1 file changed, 70 insertions(+), 24 deletions(-) + +--- a/drivers/android/binder.c ++++ b/drivers/android/binder.c +@@ -2013,15 +2013,21 @@ static void binder_cleanup_transaction(s + /** + * 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) +@@ -2031,10 +2037,16 @@ static size_t binder_get_object(struct b + 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; +@@ -2107,7 +2119,7 @@ static struct binder_buffer_object *bind + 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) +@@ -2172,7 +2184,8 @@ static bool binder_validate_fixup(struct + 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; +@@ -2285,7 +2298,7 @@ static void binder_transaction_buffer_re + 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", +@@ -2852,6 +2865,7 @@ static void binder_transaction(struct bi + 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; +@@ -2866,6 +2880,8 @@ static void binder_transaction(struct bi + 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; +@@ -3179,19 +3195,6 @@ static void binder_transaction(struct bi + + 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, + ALIGN(tr->data_size, sizeof(void *)), + (const void __user *) +@@ -3234,6 +3237,7 @@ static void binder_transaction(struct bi + 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, +@@ -3245,8 +3249,27 @@ static void binder_transaction(struct bi + 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, +@@ -3258,6 +3281,11 @@ static void binder_transaction(struct bi + 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; +@@ -3353,9 +3381,14 @@ static void binder_transaction(struct bi + } + 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; + } +@@ -3425,6 +3458,19 @@ static void binder_transaction(struct bi + 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; + diff --git a/queue-5.4/binder-defer-copies-of-pre-patched-txn-data.patch b/queue-5.4/binder-defer-copies-of-pre-patched-txn-data.patch new file mode 100644 index 00000000000..7ad04db33c4 --- /dev/null +++ b/queue-5.4/binder-defer-copies-of-pre-patched-txn-data.patch @@ -0,0 +1,425 @@ +From foo@baz Wed Nov 30 05:32:15 PM CET 2022 +From: Carlos Llamas +Date: Wed, 30 Nov 2022 03:58:02 +0000 +Subject: binder: defer copies of pre-patched txn data +To: stable@kernel.org, "Greg Kroah-Hartman" , "Arve Hjønnevåg" , "Todd Kjos" , "Martijn Coenen" , "Joel Fernandes" , "Christian Brauner" , "Hridya Valsaraju" , "Suren Baghdasaryan" +Cc: linux-kernel@vger.kernel.org, kernel-team@android.com, Todd Kjos , Carlos Llamas +Message-ID: <20221130035805.1823970-4-cmllamas@google.com> + +From: Todd Kjos + +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 +Signed-off-by: Todd Kjos +Link: https://lore.kernel.org/r/20211130185152.437403-5-tkjos@google.com +Signed-off-by: Greg Kroah-Hartman +[cmllamas: fix trivial merge conflict] +Signed-off-by: Carlos Llamas +Signed-off-by: Greg Kroah-Hartman +--- + drivers/android/binder.c | 299 +++++++++++++++++++++++++++++++++++++++++++---- + 1 file changed, 274 insertions(+), 25 deletions(-) + +--- a/drivers/android/binder.c ++++ b/drivers/android/binder.c +@@ -2636,7 +2636,246 @@ err_fd_not_accepted: + 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, +@@ -2648,6 +2887,7 @@ static int binder_translate_fd_array(str + 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)) { +@@ -2679,9 +2919,12 @@ static int binder_translate_fd_array(str + 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); + +@@ -2695,7 +2938,8 @@ static int binder_translate_fd_array(str + 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, +@@ -2741,14 +2985,7 @@ static int binder_fixup_parent(struct bi + } + 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); + } + + /** +@@ -2884,8 +3121,12 @@ static void binder_transaction(struct bi + 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; +@@ -3402,8 +3643,8 @@ static void binder_transaction(struct bi + 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) +@@ -3435,19 +3676,14 @@ static void binder_transaction(struct bi + 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) +@@ -3456,7 +3692,8 @@ static void binder_transaction(struct bi + + 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, +@@ -3496,6 +3733,17 @@ static void binder_transaction(struct bi + 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; + +@@ -3562,6 +3810,7 @@ err_bad_object_type: + 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, diff --git a/queue-5.4/binder-fix-pointer-cast-warning.patch b/queue-5.4/binder-fix-pointer-cast-warning.patch new file mode 100644 index 00000000000..346bd598f6c --- /dev/null +++ b/queue-5.4/binder-fix-pointer-cast-warning.patch @@ -0,0 +1,45 @@ +From foo@baz Wed Nov 30 05:32:15 PM CET 2022 +From: Carlos Llamas +Date: Wed, 30 Nov 2022 03:58:03 +0000 +Subject: binder: fix pointer cast warning +To: stable@kernel.org, "Greg Kroah-Hartman" , "Arve Hjønnevåg" , "Todd Kjos" , "Martijn Coenen" , "Joel Fernandes" , "Christian Brauner" , "Hridya Valsaraju" , "Suren Baghdasaryan" +Cc: linux-kernel@vger.kernel.org, kernel-team@android.com, Arnd Bergmann , Todd Kjos , Randy Dunlap , Christian Brauner , Carlos Llamas +Message-ID: <20221130035805.1823970-5-cmllamas@google.com> + +From: Arnd Bergmann + +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 +Acked-by: Randy Dunlap # build-tested +Acked-by: Christian Brauner +Signed-off-by: Arnd Bergmann +Link: https://lore.kernel.org/r/20211207122448.1185769-1-arnd@kernel.org +Signed-off-by: Greg Kroah-Hartman +Signed-off-by: Carlos Llamas +Signed-off-by: Greg Kroah-Hartman +--- + drivers/android/binder.c | 3 ++- + 1 file changed, 2 insertions(+), 1 deletion(-) + +--- a/drivers/android/binder.c ++++ b/drivers/android/binder.c +@@ -2911,7 +2911,8 @@ static int binder_translate_fd_array(str + */ + 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))) { diff --git a/queue-5.4/binder-gracefully-handle-binder_type_fda-objects-with-num_fds-0.patch b/queue-5.4/binder-gracefully-handle-binder_type_fda-objects-with-num_fds-0.patch new file mode 100644 index 00000000000..65e1d05f6ca --- /dev/null +++ b/queue-5.4/binder-gracefully-handle-binder_type_fda-objects-with-num_fds-0.patch @@ -0,0 +1,53 @@ +From foo@baz Wed Nov 30 05:32:15 PM CET 2022 +From: Carlos Llamas +Date: Wed, 30 Nov 2022 03:58:05 +0000 +Subject: binder: Gracefully handle BINDER_TYPE_FDA objects with num_fds=0 +To: stable@kernel.org, "Greg Kroah-Hartman" , "Arve Hjønnevåg" , "Todd Kjos" , "Martijn Coenen" , "Joel Fernandes" , "Christian Brauner" , "Hridya Valsaraju" , "Suren Baghdasaryan" +Cc: linux-kernel@vger.kernel.org, kernel-team@android.com, Alessandro Astone , Todd Kjos , Carlos Llamas +Message-ID: <20221130035805.1823970-7-cmllamas@google.com> + +From: Alessandro Astone + +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 +Cc: stable +Signed-off-by: Alessandro Astone +Link: https://lore.kernel.org/r/20220415120015.52684-1-ales.astone@gmail.com +Signed-off-by: Greg Kroah-Hartman +Signed-off-by: Carlos Llamas +Signed-off-by: Greg Kroah-Hartman +--- + drivers/android/binder.c | 3 +++ + 1 file changed, 3 insertions(+) + +--- a/drivers/android/binder.c ++++ b/drivers/android/binder.c +@@ -2894,6 +2894,9 @@ static int binder_translate_fd_array(str + 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", diff --git a/queue-5.4/binder-read-pre-translated-fds-from-sender-buffer.patch b/queue-5.4/binder-read-pre-translated-fds-from-sender-buffer.patch new file mode 100644 index 00000000000..95971b62a05 --- /dev/null +++ b/queue-5.4/binder-read-pre-translated-fds-from-sender-buffer.patch @@ -0,0 +1,120 @@ +From foo@baz Wed Nov 30 05:32:15 PM CET 2022 +From: Carlos Llamas +Date: Wed, 30 Nov 2022 03:58:01 +0000 +Subject: binder: read pre-translated fds from sender buffer +To: stable@kernel.org, "Greg Kroah-Hartman" , "Arve Hjønnevåg" , "Todd Kjos" , "Martijn Coenen" , "Joel Fernandes" , "Christian Brauner" , "Hridya Valsaraju" , "Suren Baghdasaryan" +Cc: linux-kernel@vger.kernel.org, kernel-team@android.com, Todd Kjos , Christian Brauner , Carlos Llamas +Message-ID: <20221130035805.1823970-3-cmllamas@google.com> + +From: Todd Kjos + +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 +Acked-by: Christian Brauner +Signed-off-by: Todd Kjos +Link: https://lore.kernel.org/r/20211130185152.437403-4-tkjos@google.com +Signed-off-by: Greg Kroah-Hartman +Signed-off-by: Carlos Llamas +Signed-off-by: Greg Kroah-Hartman +--- + drivers/android/binder.c | 39 ++++++++++++++++++++++++++++++++------- + 1 file changed, 32 insertions(+), 7 deletions(-) + +--- a/drivers/android/binder.c ++++ b/drivers/android/binder.c +@@ -2637,15 +2637,17 @@ err_fd_not_accepted: + } + + 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)) { +@@ -2669,7 +2671,10 @@ static int binder_translate_fd_array(str + */ + 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; +@@ -2678,10 +2683,9 @@ static int binder_translate_fd_array(str + 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); +@@ -3348,6 +3352,8 @@ static void binder_transaction(struct bi + 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) / +@@ -3379,8 +3385,27 @@ static void binder_transaction(struct bi + 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, diff --git a/queue-5.4/series b/queue-5.4/series index c626670e231..7dc6b11a62e 100644 --- a/queue-5.4/series +++ b/queue-5.4/series @@ -72,3 +72,9 @@ platform-x86-hp-wmi-ignore-smart-experience-app-even.patch tcp-configurable-source-port-perturb-table-size.patch net-usb-qmi_wwan-add-telit-0x103a-composition.patch dm-integrity-flush-the-journal-on-suspend.patch +binder-avoid-potential-data-leakage-when-copying-txn.patch +binder-read-pre-translated-fds-from-sender-buffer.patch +binder-defer-copies-of-pre-patched-txn-data.patch +binder-fix-pointer-cast-warning.patch +binder-address-corner-cases-in-deferred-copy-and-fixup.patch +binder-gracefully-handle-binder_type_fda-objects-with-num_fds-0.patch -- 2.47.3