Sherry, this was found by syzkaller, right? In that case, can you add
the <reported-by> tag so the issue gets closed automatically when this
gets merged?
On Tue, Aug 14, 2018 at 2:28 AM, Sherry Yang <sherryy@xxxxxxxxxxx> wrote:
> When a process dies, failed reply is sent to the sender of any transaction
> queued on a dead thread's todo list. The sender asserts that the
> received failed reply corresponds to the head of the transaction stack.
> This assert can fail if the dead thread is allowed to send outgoing
> transactions when there is already a transaction on its todo list,
> because this new transaction can end up on the transaction stack of the
> original sender. The following steps illustrate how this assertion can
> fail.
>
> 1. Thread1 sends txn19 to Thread2
> (T1->transaction_stack=txn19, T2->todo+=txn19)
> 2. Without processing todo list, Thread2 sends txn20 to Thread1
> (T1->todo+=txn20, T2->transaction_stack=txn20)
> 3. T1 processes txn20 on its todo list
> (T1->transaction_stack=txn20->txn19, T1->todo=<empty>)
> 4. T2 dies, T2->todo cleanup attempts to send failed reply for txn19, but
> T1->transaction_stack points to txn20 -- assertion failes
>
> Step 2. is the incorrect behavior. When there is a transaction on a
> thread's todo list, this thread should not be able to send any outgoing
> synchronous transactions. Only the head of the todo list needs to be
> checked because only threads that are waiting for proc work can directly
> receive work from another thread, and no work is allowed to be queued
> on such a thread without waking up the thread. This patch also enforces
> that a thread is not waiting for proc work when a work is directly
> enqueued to its todo list.
>
> Acked-by: Arve Hjønnevåg <arve@xxxxxxxxxxx>
> Signed-off-by: Sherry Yang <sherryy@xxxxxxxxxxx>
Reviewed-by: Martijn Coenen <maco@xxxxxxxxxxx>
Thanks,
Martijn
> ---
> drivers/android/binder.c | 44 +++++++++++++++++++++++++++++-----------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index d58763b6b009..f2081934eb8b 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -822,6 +822,7 @@ static void
> binder_enqueue_deferred_thread_work_ilocked(struct binder_thread *thread,
> struct binder_work *work)
> {
> + WARN_ON(!list_empty(&thread->waiting_thread_node));
> binder_enqueue_work_ilocked(work, &thread->todo);
> }
>
> @@ -839,6 +840,7 @@ static void
> binder_enqueue_thread_work_ilocked(struct binder_thread *thread,
> struct binder_work *work)
> {
> + WARN_ON(!list_empty(&thread->waiting_thread_node));
> binder_enqueue_work_ilocked(work, &thread->todo);
> thread->process_todo = true;
> }
> @@ -1270,19 +1272,12 @@ static int binder_inc_node_nilocked(struct binder_node *node, int strong,
> } else
> node->local_strong_refs++;
> if (!node->has_strong_ref && target_list) {
> + struct binder_thread *thread = container_of(target_list,
> + struct binder_thread, todo);
> binder_dequeue_work_ilocked(&node->work);
> - /*
> - * Note: this function is the only place where we queue
> - * directly to a thread->todo without using the
> - * corresponding binder_enqueue_thread_work() helper
> - * functions; in this case it's ok to not set the
> - * process_todo flag, since we know this node work will
> - * always be followed by other work that starts queue
> - * processing: in case of synchronous transactions, a
> - * BR_REPLY or BR_ERROR; in case of oneway
> - * transactions, a BR_TRANSACTION_COMPLETE.
> - */
> - binder_enqueue_work_ilocked(&node->work, target_list);
> + BUG_ON(&thread->todo != target_list);
> + binder_enqueue_deferred_thread_work_ilocked(thread,
> + &node->work);
> }
> } else {
> if (!internal)
> @@ -2723,6 +2718,7 @@ static void binder_transaction(struct binder_proc *proc,
> {
> int ret;
> struct binder_transaction *t;
> + struct binder_work *w;
> struct binder_work *tcomplete;
> binder_size_t *offp, *off_end, *off_start;
> binder_size_t off_min;
> @@ -2864,6 +2860,29 @@ static void binder_transaction(struct binder_proc *proc,
> goto err_invalid_target_handle;
> }
> binder_inner_proc_lock(proc);
> +
> + w = list_first_entry_or_null(&thread->todo,
> + struct binder_work, entry);
> + if (!(tr->flags & TF_ONE_WAY) && w &&
> + w->type == BINDER_WORK_TRANSACTION) {
> + /*
> + * Do not allow new outgoing transaction from a
> + * thread that has a transaction at the head of
> + * its todo list. Only need to check the head
> + * because binder_select_thread_ilocked picks a
> + * thread from proc->waiting_threads to enqueue
> + * the transaction, and nothing is queued to the
> + * todo list while the thread is on waiting_threads.
> + */
> + binder_user_error("%d:%d new transaction not allowed when there is a transaction on thread todo\n",
> + proc->pid, thread->pid);
> + binder_inner_proc_unlock(proc);
> + return_error = BR_FAILED_REPLY;
> + return_error_param = -EPROTO;
> + return_error_line = __LINE__;
> + goto err_bad_todo_list;
> + }
> +
> if (!(tr->flags & TF_ONE_WAY) && thread->transaction_stack) {
> struct binder_transaction *tmp;
>
> @@ -3247,6 +3266,7 @@ static void binder_transaction(struct binder_proc *proc,
> kfree(t);
> binder_stats_deleted(BINDER_STAT_TRANSACTION);
> err_alloc_t_failed:
> +err_bad_todo_list:
> err_bad_call_stack:
> err_empty_call_stack:
> err_dead_binder:
> --
> 2.18.0.597.ga71716f1ad-goog
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
