UPSTREAM: binder: fix race that allows malicious free of live buffer

commit 7bada55ab50697861eee6bb7d60b41e68a961a9c upstream

Malicious code can attempt to free buffers using the BC_FREE_BUFFER
ioctl to binder. There are protections against a user freeing a buffer
while in use by the kernel, however there was a window where
BC_FREE_BUFFER could be used to free a recently allocated buffer that
was not completely initialized. This resulted in a use-after-free
detected by KASAN with a malicious test program.

This window is closed by setting the buffer's allow_user_free attribute
to 0 when the buffer is allocated or when the user has previously freed
it instead of waiting for the caller to set it. The problem was that
when the struct buffer was recycled, allow_user_free was stale and set
to 1 allowing a free to go through.

Bug: 116855682
Change-Id: I0b38089f6fdb1adbf7e1102747e4119c9a05b191
Signed-off-by: Todd Kjos <tkjos@google.com>
Acked-by: Arve Hjønnevåg <arve@android.com>
Cc: stable <stable@vger.kernel.org> # 4.14
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4af5f51db08b6e7c931361dff5c95e9cf61b6541..e1284f51d60a25978c9bb692b4c08792c82fdecd 100644 (file)
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3143,7 +3143,6 @@ static void binder_transaction(struct binder_proc *proc,
                t->buffer = NULL;
                goto err_binder_alloc_buf_failed;
        }
-       t->buffer->allow_user_free = 0;
        t->buffer->debug_id = t->debug_id;
        t->buffer->transaction = t;
        t->buffer->target_node = target_node;
@@ -3639,14 +3638,18 @@ int binder_thread_write(struct binder_proc *proc,
 
                        buffer = binder_alloc_prepare_to_free(&proc->alloc,
                                                              data_ptr);
-                       if (buffer == NULL) {
-                               binder_user_error("%d:%d BC_FREE_BUFFER u%016llx no match\n",
-                                       proc->pid, thread->pid, (u64)data_ptr);
-                               break;
-                       }
-                       if (!buffer->allow_user_free) {
-                               binder_user_error("%d:%d BC_FREE_BUFFER u%016llx matched unreturned buffer\n",
-                                       proc->pid, thread->pid, (u64)data_ptr);
+                       if (IS_ERR_OR_NULL(buffer)) {
+                               if (PTR_ERR(buffer) == -EPERM) {
+                                       binder_user_error(
+                                               "%d:%d BC_FREE_BUFFER u%016llx matched unreturned or currently freeing buffer\n",
+                                               proc->pid, thread->pid,
+                                               (u64)data_ptr);
+                               } else {
+                                       binder_user_error(
+                                               "%d:%d BC_FREE_BUFFER u%016llx no match\n",
+                                               proc->pid, thread->pid,
+                                               (u64)data_ptr);
+                               }
                                break;
                        }
                        binder_debug(BINDER_DEBUG_FREE_BUFFER,

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index fe842a38b65f8567d5ef17e9ac1d65b22d440bae..a832bac20df70da3b6187617faa06884319b02b1 100644 (file)
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -146,14 +146,12 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked(
                else {
                        /*
                         * Guard against user threads attempting to
-                        * free the buffer twice
+                        * free the buffer when in use by kernel or
+                        * after it's already been freed.
                         */
-                       if (buffer->free_in_progress) {
-                               pr_err("%d:%d FREE_BUFFER u%016llx user freed buffer twice\n",
-                                      alloc->pid, current->pid, (u64)user_ptr);
-                               return NULL;
-                       }
-                       buffer->free_in_progress = 1;
+                       if (!buffer->allow_user_free)
+                               return ERR_PTR(-EPERM);
+                       buffer->allow_user_free = 0;
                        return buffer;
                }
        }
@@ -418,7 +416,7 @@ struct binder_buffer *binder_alloc_new_buf_locked(struct binder_alloc *alloc,
 
        rb_erase(best_fit, &alloc->free_buffers);
        buffer->free = 0;
-       buffer->free_in_progress = 0;
+       buffer->allow_user_free = 0;
        binder_insert_allocated_buffer_locked(alloc, buffer);
        binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC,
                     "%d: binder_alloc_buf size %zd got %pK\n",

diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index dd5649bf6469f4fe8d7cd0c7108f85801440f7d0..395d9b56bbf1721c2ab6b64b4ac1eebb836ad5af 100644 (file)
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -48,8 +48,7 @@ struct binder_buffer {
        unsigned free:1;
        unsigned allow_user_free:1;
        unsigned async_transaction:1;
-       unsigned free_in_progress:1;
-       unsigned debug_id:28;
+       unsigned debug_id:29;
 
        struct binder_transaction *transaction;