binder: fix race that allows malicious free of live buffer
Subsystems (2)
Todd Kjos
maintainerPATCH 1/1 binder: fix race that allows malicious free of live buffer
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.
--- drivers/android/binder.c | 21 ++++++++++++--------- drivers/android/binder_alloc.c | 16 ++++++---------- drivers/android/binder_alloc.h | 3 +-- 3 files changed, 19 insertions(+), 21 deletions(-)
drivers/android/binder.c +12 -9
@@ -2974,7 +2974,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;
@@ -3510,14 +3509,18 @@ static 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,
drivers/android/binder_alloc.c +6 -10
@@ -151,16 +151,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) { -binder_alloc_debug (BINDER_DEBUG_USER_ERROR , -"%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; } }
@@ -500,7 +496,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(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",
drivers/android/binder_alloc.h +1 -2
@@ -50,8 +50,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;
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
Greg KH
maintainerQuoted 17 lines · Todd Kjos
Malicious code can attempt to free buffers using the
BC_FREE_BUFFERioctl to binder. There are protections against a user freeing a buffer while in use by the kernel, however there was a window whereBC_FREE_BUFFERcould 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_freeattribute 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_freewas stale and set to 1 allowing a free to go through.
Quoted 2 lines
Signed-off-by Todd Kjos <[email protected]>Acked-by Arve Hjønnevåg <[email protected]>
No "stable" tag here? Any idea how far back the stable backporting should go, if any?
thanks,
greg k-h
Quoted 19 lines · Greg KH
Quoted 17 lines · Todd Kjos
Malicious code can attempt to free buffers using the
BC_FREE_BUFFERioctl to binder. There are protections against a user freeing a buffer while in use by the kernel, however there was a window whereBC_FREE_BUFFERcould 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_freeattribute 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_freewas stale and set to 1 allowing a free to go through.
Quoted 5 lines
Quoted 2 lines
Signed-off-by Todd Kjos <[email protected]>Acked-by Arve Hjønnevåg <[email protected]>No "stable" tag here? Any idea how far back the stable backporting should go, if any?
Sorry about that. It should be backported to 4.14 and later.
Quoted 4 lines
thanks,
greg k-h