From 5cd1126dbe3f33e17465a8ee1ad9834719cd8d4f Mon Sep 17 00:00:00 2001 From: Todd Kjos Date: Fri, 20 Nov 2020 15:37:43 -0800 Subject: [PATCH] UPSTREAM: binder: add flag to clear buffer on txn complete Add a per-transaction flag to indicate that the buffer must be cleared when the transaction is complete to prevent copies of sensitive data from being preserved in memory. Signed-off-by: Todd Kjos Link: https://lore.kernel.org/r/20201120233743.3617529-1-tkjos@google.com Cc: stable Signed-off-by: Greg Kroah-Hartman Bug: 171501513 Change-Id: Ic9338c85cbe3b11ab6f2bda55dce9964bb48447a (cherry picked from commit 0f966cba95c78029f491b433ea95ff38f414a761) Signed-off-by: Todd Kjos Signed-off-by: Sami Tolvanen --- drivers/android/binder.c | 1 + drivers/android/binder_alloc.c | 48 +++++++++++++++++++++++++++++ drivers/android/binder_alloc.h | 4 ++- include/uapi/linux/android/binder.h | 1 + 4 files changed, 53 insertions(+), 1 deletion(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index ba51fc709dea..9185a7f10852 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -3305,6 +3305,7 @@ static void binder_transaction(struct binder_proc *proc, t->buffer->debug_id = t->debug_id; t->buffer->transaction = t; t->buffer->target_node = target_node; + t->buffer->clear_on_free = !!(t->flags & TF_CLEAR_BUF); trace_binder_transaction_alloc_buf(t->buffer); if (binder_alloc_copy_user_to_buffer( diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 28fededae3d3..5addcd56afb4 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -672,6 +672,8 @@ static void binder_free_buf_locked(struct binder_alloc *alloc, binder_insert_free_buffer(alloc, buffer); } +static void binder_alloc_clear_buf(struct binder_alloc *alloc, + struct binder_buffer *buffer); /** * binder_alloc_free_buf() - free a binder buffer * @alloc: binder_alloc for this proc @@ -682,6 +684,18 @@ static void binder_free_buf_locked(struct binder_alloc *alloc, void binder_alloc_free_buf(struct binder_alloc *alloc, struct binder_buffer *buffer) { + /* + * We could eliminate the call to binder_alloc_clear_buf() + * from binder_alloc_deferred_release() by moving this to + * binder_alloc_free_buf_locked(). However, that could + * increase contention for the alloc mutex if clear_on_free + * is used frequently for large buffers. The mutex is not + * needed for correctness here. + */ + if (buffer->clear_on_free) { + binder_alloc_clear_buf(alloc, buffer); + buffer->clear_on_free = false; + } mutex_lock(&alloc->mutex); binder_free_buf_locked(alloc, buffer); mutex_unlock(&alloc->mutex); @@ -777,6 +791,10 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc) /* Transaction should already have been freed */ BUG_ON(buffer->transaction); + if (buffer->clear_on_free) { + binder_alloc_clear_buf(alloc, buffer); + buffer->clear_on_free = false; + } binder_free_buf_locked(alloc, buffer); buffers++; } @@ -1103,6 +1121,36 @@ static struct page *binder_alloc_get_page(struct binder_alloc *alloc, return lru_page->page_ptr; } +/** + * binder_alloc_clear_buf() - zero out buffer + * @alloc: binder_alloc for this proc + * @buffer: binder buffer to be cleared + * + * memset the given buffer to 0 + */ +static void binder_alloc_clear_buf(struct binder_alloc *alloc, + struct binder_buffer *buffer) +{ + size_t bytes = binder_alloc_buffer_size(alloc, buffer); + binder_size_t buffer_offset = 0; + + while (bytes) { + unsigned long size; + struct page *page; + pgoff_t pgoff; + void *kptr; + + page = binder_alloc_get_page(alloc, buffer, + buffer_offset, &pgoff); + size = min_t(size_t, bytes, PAGE_SIZE - pgoff); + kptr = kmap(page) + pgoff; + memset(kptr, 0, size); + kunmap(page); + bytes -= size; + buffer_offset += size; + } +} + /** * binder_alloc_copy_user_to_buffer() - copy src user to tgt user * @alloc: binder_alloc for this proc diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h index 3daa3e211267..fdb220eb55d1 100644 --- a/drivers/android/binder_alloc.h +++ b/drivers/android/binder_alloc.h @@ -32,6 +32,7 @@ struct binder_transaction; * @entry: entry alloc->buffers * @rb_node: node for allocated_buffers/free_buffers rb trees * @free: %true if buffer is free + * @clear_on_free: %true if buffer must be zeroed after use * @allow_user_free: %true if user is allowed to free buffer * @async_transaction: %true if buffer is in use for an async txn * @debug_id: unique ID for debugging @@ -50,9 +51,10 @@ struct binder_buffer { struct rb_node rb_node; /* free entry by size or allocated entry */ /* by address */ unsigned free:1; + unsigned clear_on_free:1; unsigned allow_user_free:1; unsigned async_transaction:1; - unsigned debug_id:29; + unsigned debug_id:28; struct binder_transaction *transaction; diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h index 0631c500702c..0457ff02d616 100644 --- a/include/uapi/linux/android/binder.h +++ b/include/uapi/linux/android/binder.h @@ -294,6 +294,7 @@ enum transaction_flags { TF_ROOT_OBJECT = 0x04, /* contents are the component's root object */ TF_STATUS_CODE = 0x08, /* contents are a 32-bit status code */ TF_ACCEPT_FDS = 0x10, /* allow replies with file descriptors */ + TF_CLEAR_BUF = 0x20, /* clear buffer on txn complete */ }; struct binder_transaction_data { -- 2.20.1