| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aBJvUOeznP_4PHDq@google.com>
Date: Wed, 30 Apr 2025 18:43:28 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: "Tiffany Y. Yang" <ynaffit@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joel@...lfernandes.org>,
Christian Brauner <brauner@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>,
linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH 2/2] binder: Create safe versions of binder log files
On Thu, Mar 27, 2025 at 10:02:26PM +0000, Tiffany Y. Yang wrote:
> Binder defines several seq_files that can be accessed via debugfs or
> binderfs. Some of these files (e.g., 'state' and 'transactions')
> contain more granular information about binder's internal state that
> is helpful for debugging, but they also leak userspace address data
> through user-defined 'cookie' or 'ptr' values. Consequently, access
> to these files must be heavily restricted.
>
> Add two new files, 'state_hashed' and 'transactions_hashed', that
> reproduce the information in the original files but use the kernel's
> raw pointer obfuscation to hash any potential user addresses. This
> approach allows systems to grant broader access to the new files
> without having to change the security policy around the existing ones.
>
> In practice, userspace populates these fields with user addresses, but
> within the driver, these values only serve as unique identifiers for
> their associated binder objects. Consequently, binder logs can
> obfuscate these values and still retain meaning. While this strategy
> prevents leaking information about the userspace memory layout in the
> existing log files, it also decouples log messages about binder
> objects from their user-defined identifiers.
>
> Use the kernel's raw pointer obfuscation to hash these values before
> they are output to the existing binder seq_files. Additionally, create
> a new binder log file, 'ptr_translations', to allow privileged users
> to access the map between the original values and their hashes.
Likely this last ptr_translation thing was leftover from a previous
version? If so, please drop on next version.
>
> Signed-off-by: Tiffany Y. Yang <ynaffit@...gle.com>
> ---
> drivers/android/binder.c | 103 +++++++++++++++++++++++++++++----------
> 1 file changed, 77 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index d963a7860aa3..a9e433ad12a7 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6380,7 +6380,7 @@ static void print_binder_work_ilocked(struct seq_file *m,
> struct binder_proc *proc,
> const char *prefix,
> const char *transaction_prefix,
> - struct binder_work *w)
> + struct binder_work *w, bool hash_ptrs)
> {
> struct binder_node *node;
> struct binder_transaction *t;
> @@ -6403,9 +6403,14 @@ static void print_binder_work_ilocked(struct seq_file *m,
> break;
> case BINDER_WORK_NODE:
> node = container_of(w, struct binder_node, work);
> - seq_printf(m, "%snode work %d: u%016llx c%016llx\n",
> + if (hash_ptrs)
> + seq_printf(m, "%snode work %d: u%p c%p\n",
> prefix, node->debug_id,
> - (u64)node->ptr, (u64)node->cookie);
> + (void *)node->ptr, (void *)node->cookie);
> + else
> + seq_printf(m, "%snode work %d: u%016llx c%016llx\n",
> + prefix, node->debug_id,
> + (u64)node->ptr, (u64)node->cookie);
> break;
> case BINDER_WORK_DEAD_BINDER:
> seq_printf(m, "%shas dead binder\n", prefix);
> @@ -6430,7 +6435,7 @@ static void print_binder_work_ilocked(struct seq_file *m,
>
> static void print_binder_thread_ilocked(struct seq_file *m,
> struct binder_thread *thread,
> - bool print_always)
> + bool print_always, bool hash_ptrs)
> {
> struct binder_transaction *t;
> struct binder_work *w;
> @@ -6460,14 +6465,16 @@ static void print_binder_thread_ilocked(struct seq_file *m,
> }
> list_for_each_entry(w, &thread->todo, entry) {
> print_binder_work_ilocked(m, thread->proc, " ",
> - " pending transaction", w);
> + " pending transaction",
> + w, hash_ptrs);
> }
> if (!print_always && m->count == header_pos)
> m->count = start_pos;
> }
>
> static void print_binder_node_nilocked(struct seq_file *m,
> - struct binder_node *node)
> + struct binder_node *node,
> + bool hash_ptrs)
> {
> struct binder_ref *ref;
> struct binder_work *w;
> @@ -6475,8 +6482,13 @@ static void print_binder_node_nilocked(struct seq_file *m,
>
> count = hlist_count_nodes(&node->refs);
>
> - seq_printf(m, " node %d: u%016llx c%016llx hs %d hw %d ls %d lw %d is %d iw %d tr %d",
> - node->debug_id, (u64)node->ptr, (u64)node->cookie,
> + if (hash_ptrs)
> + seq_printf(m, " node %d: u%p c%p", node->debug_id,
> + (void *)node->ptr, (void *)node->cookie);
> + else
> + seq_printf(m, " node %d: u%016llx c%016llx", node->debug_id,
> + (u64)node->ptr, (u64)node->cookie);
> + seq_printf(m, " hs %d hw %d ls %d lw %d is %d iw %d tr %d",
> node->has_strong_ref, node->has_weak_ref,
> node->local_strong_refs, node->local_weak_refs,
> node->internal_strong_refs, count, node->tmp_refs);
> @@ -6489,7 +6501,8 @@ static void print_binder_node_nilocked(struct seq_file *m,
> if (node->proc) {
> list_for_each_entry(w, &node->async_todo, entry)
> print_binder_work_ilocked(m, node->proc, " ",
> - " pending async transaction", w);
> + " pending async transaction",
> + w, hash_ptrs);
> }
> }
>
> @@ -6510,6 +6523,7 @@ static void print_binder_ref_olocked(struct seq_file *m,
> * @m: struct seq_file for output via seq_printf()
> * @node: struct binder_node to print fields of
> * @prev_node: struct binder_node we hold a temporary reference to (if any)
> + * @hash_ptrs: whether to hash @node's binder_uintptr_t fields
> *
> * Helper function to handle synchronization around printing a struct
> * binder_node while iterating through @node->proc->nodes or the dead nodes
> @@ -6520,7 +6534,7 @@ static void print_binder_ref_olocked(struct seq_file *m,
> */
> static struct binder_node *
> print_next_binder_node_ilocked(struct seq_file *m, struct binder_node *node,
> - struct binder_node *prev_node)
> + struct binder_node *prev_node, bool hash_ptrs)
> {
> /*
> * Take a temporary reference on the node so that isn't removed from
> @@ -6538,7 +6552,7 @@ print_next_binder_node_ilocked(struct seq_file *m, struct binder_node *node,
> if (prev_node)
> binder_put_node(prev_node);
> binder_node_inner_lock(node);
> - print_binder_node_nilocked(m, node);
> + print_binder_node_nilocked(m, node, hash_ptrs);
> binder_node_inner_unlock(node);
> if (node->proc)
> binder_inner_proc_lock(node->proc);
> @@ -6548,7 +6562,7 @@ print_next_binder_node_ilocked(struct seq_file *m, struct binder_node *node,
> }
>
> static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
> - bool print_all)
> + bool print_all, bool hash_ptrs)
> {
> struct binder_work *w;
> struct rb_node *n;
> @@ -6563,7 +6577,7 @@ static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
> binder_inner_proc_lock(proc);
> for (n = rb_first(&proc->threads); n; n = rb_next(n))
> print_binder_thread_ilocked(m, rb_entry(n, struct binder_thread,
> - rb_node), print_all);
> + rb_node), print_all, hash_ptrs);
>
> for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
> struct binder_node *node = rb_entry(n, struct binder_node,
> @@ -6571,7 +6585,8 @@ static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
> if (!print_all && !node->has_async_transaction)
> continue;
>
> - last_node = print_next_binder_node_ilocked(m, node, last_node);
> + last_node = print_next_binder_node_ilocked(m, node, last_node,
> + hash_ptrs);
> }
> binder_inner_proc_unlock(proc);
> if (last_node)
> @@ -6589,7 +6604,8 @@ static void print_binder_proc(struct seq_file *m, struct binder_proc *proc,
> binder_inner_proc_lock(proc);
> list_for_each_entry(w, &proc->todo, entry)
> print_binder_work_ilocked(m, proc, " ",
> - " pending transaction", w);
> + " pending transaction", w,
> + hash_ptrs);
> list_for_each_entry(w, &proc->delivered_death, entry) {
> seq_puts(m, " has delivered dead binder\n");
> break;
> @@ -6771,7 +6787,7 @@ static void print_binder_proc_stats(struct seq_file *m,
> print_binder_stats(m, " ", &proc->stats);
> }
>
> -static int state_show(struct seq_file *m, void *unused)
> +static void print_binder_state(struct seq_file *m, bool hash_ptrs)
> {
> struct binder_proc *proc;
> struct binder_node *node;
> @@ -6783,16 +6799,38 @@ static int state_show(struct seq_file *m, void *unused)
> if (!hlist_empty(&binder_dead_nodes))
> seq_puts(m, "dead nodes:\n");
> hlist_for_each_entry(node, &binder_dead_nodes, dead_node)
> - last_node = print_next_binder_node_ilocked(m, node, last_node);
> + last_node = print_next_binder_node_ilocked(m, node, last_node,
> + hash_ptrs);
> spin_unlock(&binder_dead_nodes_lock);
> if (last_node)
> binder_put_node(last_node);
>
> mutex_lock(&binder_procs_lock);
> hlist_for_each_entry(proc, &binder_procs, proc_node)
> - print_binder_proc(m, proc, true);
> + print_binder_proc(m, proc, true, hash_ptrs);
> mutex_unlock(&binder_procs_lock);
> +}
> +
> +static void print_binder_transactions(struct seq_file *m, bool hash_ptrs)
> +{
> + struct binder_proc *proc;
> +
> + seq_puts(m, "binder transactions:\n");
> + mutex_lock(&binder_procs_lock);
> + hlist_for_each_entry(proc, &binder_procs, proc_node)
> + print_binder_proc(m, proc, false, hash_ptrs);
> + mutex_unlock(&binder_procs_lock);
> +}
> +
> +static int state_show(struct seq_file *m, void *unused)
> +{
> + print_binder_state(m, false);
> + return 0;
> +}
>
> +static int state_hashed_show(struct seq_file *m, void *unused)
> +{
> + print_binder_state(m, true);
> return 0;
> }
>
> @@ -6814,14 +6852,13 @@ static int stats_show(struct seq_file *m, void *unused)
>
> static int transactions_show(struct seq_file *m, void *unused)
> {
> - struct binder_proc *proc;
> -
> - seq_puts(m, "binder transactions:\n");
> - mutex_lock(&binder_procs_lock);
> - hlist_for_each_entry(proc, &binder_procs, proc_node)
> - print_binder_proc(m, proc, false);
> - mutex_unlock(&binder_procs_lock);
> + print_binder_transactions(m, false);
> + return 0;
> +}
>
> +static int transactions_hashed_show(struct seq_file *m, void *unused)
> +{
> + print_binder_transactions(m, true);
> return 0;
> }
>
> @@ -6834,7 +6871,7 @@ static int proc_show(struct seq_file *m, void *unused)
> hlist_for_each_entry(itr, &binder_procs, proc_node) {
> if (itr->pid == pid) {
> seq_puts(m, "binder proc state:\n");
> - print_binder_proc(m, itr, true);
> + print_binder_proc(m, itr, true, false);
> }
> }
> mutex_unlock(&binder_procs_lock);
> @@ -6901,8 +6938,10 @@ const struct file_operations binder_fops = {
> };
>
> DEFINE_SHOW_ATTRIBUTE(state);
> +DEFINE_SHOW_ATTRIBUTE(state_hashed);
> DEFINE_SHOW_ATTRIBUTE(stats);
> DEFINE_SHOW_ATTRIBUTE(transactions);
> +DEFINE_SHOW_ATTRIBUTE(transactions_hashed);
> DEFINE_SHOW_ATTRIBUTE(transaction_log);
>
> const struct binder_debugfs_entry binder_debugfs_entries[] = {
> @@ -6912,6 +6951,12 @@ const struct binder_debugfs_entry binder_debugfs_entries[] = {
> .fops = &state_fops,
> .data = NULL,
> },
> + {
> + .name = "state_hashed",
> + .mode = 0444,
> + .fops = &state_hashed_fops,
> + .data = NULL,
> + },
> {
> .name = "stats",
> .mode = 0444,
> @@ -6924,6 +6969,12 @@ const struct binder_debugfs_entry binder_debugfs_entries[] = {
> .fops = &transactions_fops,
> .data = NULL,
> },
> + {
> + .name = "transactions_hashed",
> + .mode = 0444,
> + .fops = &transactions_hashed_fops,
> + .data = NULL,
> + },
> {
> .name = "transaction_log",
> .mode = 0444,
> --
> 2.49.0.395.g12beb8f557-goog
>
Other than minor nit, feel free to add:
Acked-by: Carlos Llamas <cmllamas@...gle.com>
Regards,
--
Carlos Llamas
Powered by blists - more mailing lists