binder: fix possible UAF when freeing buffer [Linux 4.14.136]

This Linux kernel change "binder: fix possible UAF when freeing buffer" is included in the Linux 4.14.136 release. This change is authored by Todd Kjos <tkjos [at] android.com> on Wed Jun 12 13:29:27 2019 -0700. The commit for this change in Linux stable tree is a4a3c07 (patch) which is from upstream commit a370003. The same Linux upstream change may have been applied to various maintained Linux releases and you can find all Linux releases containing changes from upstream a370003.

binder: fix possible UAF when freeing buffer

commit a370003cc301d4361bae20c9ef615f89bf8d1e8a upstream.

There is a race between the binder driver cleaning
up a completed transaction via binder_free_transaction()
and a user calling binder_ioctl(BC_FREE_BUFFER) to
release a buffer. It doesn't matter which is first but
they need to be protected against running concurrently
which can result in a UAF.

Signed-off-by: Todd Kjos <tkjos@google.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

There are 16 lines of Linux source code added/deleted in this change. Code changes to Linux kernel are as follows.

 drivers/android/binder.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e694fd2..05e75d1 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1903,8 +1903,18 @@ static struct binder_thread *binder_get_txn_from_and_acq_inner(

 static void binder_free_transaction(struct binder_transaction *t)
 {
-   if (t->buffer)
-       t->buffer->transaction = NULL;
+   struct binder_proc *target_proc = t->to_proc;
+
+   if (target_proc) {
+       binder_inner_proc_lock(target_proc);
+       if (t->buffer)
+           t->buffer->transaction = NULL;
+       binder_inner_proc_unlock(target_proc);
+   }
+   /*
+    * If the transaction has no target_proc, then
+    * t->buffer->transaction has already been cleared.
+    */
    kfree(t);
    binder_stats_deleted(BINDER_STAT_TRANSACTION);
 }
@@ -3426,10 +3436,12 @@ static int binder_thread_write(struct binder_proc *proc,
                     buffer->debug_id,
                     buffer->transaction ? "active" : "finished");

+           binder_inner_proc_lock(proc);
            if (buffer->transaction) {
                buffer->transaction->buffer = NULL;
                buffer->transaction = NULL;
            }
+           binder_inner_proc_unlock(proc);
            if (buffer->async_transaction && buffer->target_node) {
                struct binder_node *buf_node;
                struct binder_work *w;

Leave a Reply

Your email address will not be published. Required fields are marked *