Re: [PATCH V4] binder: ipc namespace support for android binder

[Christian Brauner]

On November 12, 2018 8:45:07 AM PST, Todd Kjos <tkjos@xxxxxxxxxx> wrote:
>+christian@xxxxxxxxxx +Martijn Coenen
>
>Christian,
>
>Does this patch work for your container use-cases? If not, please
>comment on this thread. Let's discuss at LPC this week.

I have not received an answer to my questions in the last version of this patch set. Also it would be good if I could be Cc'ed by default. I can't hunt down all patches.
I do not know of any kernel entity, specifically devices, that change namespaces on open().
This seems like an invitation for all kinds of security bugs.
A device node belongs to one namespace only which is attached to the underlying kobject. Opening the device should never change that.
Please look at how mqueue or shm are doing this. They don't change namespaces on open either.
I have to say that is one of the main reasons why I disagree with that design.


>
>-Todd
>
>On Mon, Nov 12, 2018 at 1:38 AM chouryzhou(åå) <chouryzhou@xxxxxxxxxxx>
>wrote:
>>
>> Currently android's binder is not isolated by ipc namespace. Since
>binder
>> is a form of IPC and therefore should be tied to ipc namespace. With
>this
>> patch, we can run multiple instances of  android container on one
>host.
>>
>> This patch move "binder_procs" and "binder_context" into
>ipc_namespace,
>> driver will find the context from it when opening. For debugfs,
>binder_proc
>> is namespace-aware, but not for binder dead nodes, binder_stats and
>> binder_transaction_log_entry (we added ipc inum to trace it).
>>
>> Signed-off-by: chouryzhou <chouryzhou@xxxxxxxxxxx>
>> Reviewed-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>
>> ---
>>  drivers/android/binder.c      | 133
>++++++++++++++++++++++++++++++++----------
>>  include/linux/ipc_namespace.h |  15 +++++
>>  ipc/namespace.c               |  10 +++-
>>  3 files changed, 125 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index cb30a524d16d..453265505b04 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -67,6 +67,8 @@
>>  #include <linux/sched/mm.h>
>>  #include <linux/seq_file.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/proc_ns.h>
>> +#include <linux/ipc_namespace.h>
>>  #include <linux/pid_namespace.h>
>>  #include <linux/security.h>
>>  #include <linux/spinlock.h>
>> @@ -80,13 +82,21 @@
>>  #include "binder_alloc.h"
>>  #include "binder_trace.h"
>>
>> +
>> +#ifndef CONFIG_IPC_NS
>> +static struct ipc_namespace binder_ipc_ns = {
>> +       .ns.inum = PROC_IPC_INIT_INO,
>> +};
>> +
>> +#define ipcns  (&binder_ipc_ns)
>> +#else
>> +#define ipcns  (current->nsproxy->ipc_ns)
>> +#endif
>> +
>>  static HLIST_HEAD(binder_deferred_list);
>>  static DEFINE_MUTEX(binder_deferred_lock);
>>
>>  static HLIST_HEAD(binder_devices);
>> -static HLIST_HEAD(binder_procs);
>> -static DEFINE_MUTEX(binder_procs_lock);
>> -
>>  static HLIST_HEAD(binder_dead_nodes);
>>  static DEFINE_SPINLOCK(binder_dead_nodes_lock);
>>
>> @@ -233,6 +243,7 @@ struct binder_transaction_log_entry {
>>         uint32_t return_error;
>>         uint32_t return_error_param;
>>         const char *context_name;
>> +       unsigned int ipc_inum;
>>  };
>>  struct binder_transaction_log {
>>         atomic_t cur;
>> @@ -263,19 +274,66 @@ static struct binder_transaction_log_entry
>*binder_transaction_log_add(
>>  }
>>
>>  struct binder_context {
>> +       struct hlist_node hlist;
>>         struct binder_node *binder_context_mgr_node;
>>         struct mutex context_mgr_node_lock;
>>
>>         kuid_t binder_context_mgr_uid;
>> +       int   device;
>>         const char *name;
>>  };
>>
>>  struct binder_device {
>>         struct hlist_node hlist;
>>         struct miscdevice miscdev;
>> -       struct binder_context context;
>>  };
>>
>> +void binder_exit_ns(struct ipc_namespace *ns)
>> +{
>> +       struct binder_context *context;
>> +       struct hlist_node *tmp;
>> +
>> +       mutex_destroy(&ns->binder_procs_lock);
>> +       hlist_for_each_entry_safe(context, tmp, &ns->binder_contexts,
>hlist) {
>> +               mutex_destroy(&context->context_mgr_node_lock);
>> +               hlist_del(&context->hlist);
>> +               kfree(context);
>> +       }
>> +}
>> +
>> +int binder_init_ns(struct ipc_namespace *ns)
>> +{
>> +       int ret;
>> +       struct binder_device *device;
>> +
>> +       mutex_init(&ns->binder_procs_lock);
>> +       INIT_HLIST_HEAD(&ns->binder_procs);
>> +       INIT_HLIST_HEAD(&ns->binder_contexts);
>> +
>> +       hlist_for_each_entry(device, &binder_devices, hlist) {
>> +               struct binder_context *context;
>> +
>> +               context = kzalloc(sizeof(*context), GFP_KERNEL);
>> +               if (!context) {
>> +                       ret = -ENOMEM;
>> +                       goto err;
>> +               }
>> +
>> +               context->device = device->miscdev.minor;
>> +               context->name = device->miscdev.name;
>> +               context->binder_context_mgr_uid = INVALID_UID;
>> +               mutex_init(&context->context_mgr_node_lock);
>> +
>> +               hlist_add_head(&context->hlist,
>&ns->binder_contexts);
>> +       }
>> +
>> +       return 0;
>> +err:
>> +       binder_exit_ns(ns);
>> +       return ret;
>> +}
>> +
>> +
>>  /**
>>   * struct binder_work - work enqueued on a worklist
>>   * @entry:             node enqueued on list
>> @@ -2728,6 +2786,7 @@ static void binder_transaction(struct
>binder_proc *proc,
>>         e->data_size = tr->data_size;
>>         e->offsets_size = tr->offsets_size;
>>         e->context_name = proc->context->name;
>> +       e->ipc_inum = ipcns->ns.inum;
>>
>>         if (reply) {
>>                 binder_inner_proc_lock(proc);
>> @@ -4922,6 +4981,7 @@ static int binder_open(struct inode *nodp,
>struct file *filp)
>>  {
>>         struct binder_proc *proc;
>>         struct binder_device *binder_dev;
>> +       struct binder_context *context;
>>
>>         binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n",
>__func__,
>>                      current->group_leader->pid, current->pid);
>> @@ -4937,7 +4997,15 @@ static int binder_open(struct inode *nodp,
>struct file *filp)
>>         proc->default_priority = task_nice(current);
>>         binder_dev = container_of(filp->private_data, struct
>binder_device,
>>                                   miscdev);
>> -       proc->context = &binder_dev->context;
>> +       hlist_for_each_entry(context, &ipcns->binder_contexts, hlist)
>{
>> +               if (context->device == binder_dev->miscdev.minor) {
>> +                       proc->context = context;
>> +                       break;
>> +               }
>> +       }
>> +       if (!proc->context)
>> +               return -ENOENT;
>> +
>>         binder_alloc_init(&proc->alloc);
>>
>>         binder_stats_created(BINDER_STAT_PROC);
>> @@ -4946,9 +5014,9 @@ static int binder_open(struct inode *nodp,
>struct file *filp)
>>         INIT_LIST_HEAD(&proc->waiting_threads);
>>         filp->private_data = proc;
>>
>> -       mutex_lock(&binder_procs_lock);
>> -       hlist_add_head(&proc->proc_node, &binder_procs);
>> -       mutex_unlock(&binder_procs_lock);
>> +       mutex_lock(&ipcns->binder_procs_lock);
>> +       hlist_add_head(&proc->proc_node, &ipcns->binder_procs);
>> +       mutex_unlock(&ipcns->binder_procs_lock);
>>
>>         if (binder_debugfs_dir_entry_proc) {
>>                 char strbuf[11];
>> @@ -5082,9 +5150,9 @@ static void binder_deferred_release(struct
>binder_proc *proc)
>>         struct rb_node *n;
>>         int threads, nodes, incoming_refs, outgoing_refs,
>active_transactions;
>>
>> -       mutex_lock(&binder_procs_lock);
>> +       mutex_lock(&ipcns->binder_procs_lock);
>>         hlist_del(&proc->proc_node);
>> -       mutex_unlock(&binder_procs_lock);
>> +       mutex_unlock(&ipcns->binder_procs_lock);
>>
>>         mutex_lock(&context->context_mgr_node_lock);
>>         if (context->binder_context_mgr_node &&
>> @@ -5623,10 +5691,10 @@ static int binder_state_show(struct seq_file
>*m, void *unused)
>>         if (last_node)
>>                 binder_put_node(last_node);
>>
>> -       mutex_lock(&binder_procs_lock);
>> -       hlist_for_each_entry(proc, &binder_procs, proc_node)
>> +       mutex_lock(&ipcns->binder_procs_lock);
>> +       hlist_for_each_entry(proc, &ipcns->binder_procs, proc_node)
>>                 print_binder_proc(m, proc, 1);
>> -       mutex_unlock(&binder_procs_lock);
>> +       mutex_unlock(&ipcns->binder_procs_lock);
>>
>>         return 0;
>>  }
>> @@ -5639,10 +5707,10 @@ static int binder_stats_show(struct seq_file
>*m, void *unused)
>>
>>         print_binder_stats(m, "", &binder_stats);
>>
>> -       mutex_lock(&binder_procs_lock);
>> -       hlist_for_each_entry(proc, &binder_procs, proc_node)
>> +       mutex_lock(&ipcns->binder_procs_lock);
>> +       hlist_for_each_entry(proc, &ipcns->binder_procs, proc_node)
>>                 print_binder_proc_stats(m, proc);
>> -       mutex_unlock(&binder_procs_lock);
>> +       mutex_unlock(&ipcns->binder_procs_lock);
>>
>>         return 0;
>>  }
>> @@ -5652,10 +5720,10 @@ static int binder_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)
>> +       mutex_lock(&ipcns->binder_procs_lock);
>> +       hlist_for_each_entry(proc, &ipcns->binder_procs, proc_node)
>>                 print_binder_proc(m, proc, 0);
>> -       mutex_unlock(&binder_procs_lock);
>> +       mutex_unlock(&ipcns->binder_procs_lock);
>>
>>         return 0;
>>  }
>> @@ -5665,14 +5733,14 @@ static int binder_proc_show(struct seq_file
>*m, void *unused)
>>         struct binder_proc *itr;
>>         int pid = (unsigned long)m->private;
>>
>> -       mutex_lock(&binder_procs_lock);
>> -       hlist_for_each_entry(itr, &binder_procs, proc_node) {
>> +       mutex_lock(&ipcns->binder_procs_lock);
>> +       hlist_for_each_entry(itr, &ipcns->binder_procs, proc_node) {
>>                 if (itr->pid == pid) {
>>                         seq_puts(m, "binder proc state:\n");
>>                         print_binder_proc(m, itr, 1);
>>                 }
>>         }
>> -       mutex_unlock(&binder_procs_lock);
>> +       mutex_unlock(&ipcns->binder_procs_lock);
>>
>>         return 0;
>>  }
>> @@ -5687,12 +5755,12 @@ static void
>print_binder_transaction_log_entry(struct seq_file *m,
>>          */
>>         smp_rmb();
>>         seq_printf(m,
>> -                  "%d: %s from %d:%d to %d:%d context %s node %d
>handle %d size %d:%d ret %d/%d l=%d",
>> +                  "%d: %s from %d:%d to %d:%d ipc %d context %s node
>%d handle %d size %d:%d ret %d/%d l=%d",
>>                    e->debug_id, (e->call_type == 2) ? "reply" :
>>                    ((e->call_type == 1) ? "async" : "call "),
>e->from_proc,
>> -                  e->from_thread, e->to_proc, e->to_thread,
>e->context_name,
>> -                  e->to_node, e->target_handle, e->data_size,
>e->offsets_size,
>> -                  e->return_error, e->return_error_param,
>> +                  e->from_thread, e->to_proc, e->to_thread,
>e->ipc_inum,
>> +                  e->context_name, e->to_node, e->target_handle,
>e->data_size,
>> +                  e->offsets_size, e->return_error,
>e->return_error_param,
>>                    e->return_error_line);
>>         /*
>>          * read-barrier to guarantee read of debug_id_done after
>> @@ -5753,10 +5821,6 @@ static int __init init_binder_device(const
>char *name)
>>         binder_device->miscdev.minor = MISC_DYNAMIC_MINOR;
>>         binder_device->miscdev.name = name;
>>
>> -       binder_device->context.binder_context_mgr_uid = INVALID_UID;
>> -       binder_device->context.name = name;
>> -       mutex_init(&binder_device->context.context_mgr_node_lock);
>> -
>>         ret = misc_register(&binder_device->miscdev);
>>         if (ret < 0) {
>>                 kfree(binder_device);
>> @@ -5831,9 +5895,16 @@ static int __init binder_init(void)
>>                 if (ret)
>>                         goto err_init_binder_device_failed;
>>         }
>> +#ifdef CONFIG_IPC_NS
>> +       ret = binder_init_ns(&init_ipc_ns);
>> +#else
>> +       ret = binder_init_ns(&binder_ipc_ns);
>> +#endif
>> +       if (ret)
>> +               goto err_init_namespace_failed;
>>
>>         return ret;
>> -
>> +err_init_namespace_failed:
>>  err_init_binder_device_failed:
>>         hlist_for_each_entry_safe(device, tmp, &binder_devices,
>hlist) {
>>                 misc_deregister(&device->miscdev);
>> diff --git a/include/linux/ipc_namespace.h
>b/include/linux/ipc_namespace.h
>> index 6ab8c1bada3f..d7f850a2ded8 100644
>> --- a/include/linux/ipc_namespace.h
>> +++ b/include/linux/ipc_namespace.h
>> @@ -63,6 +63,13 @@ struct ipc_namespace {
>>         unsigned int    mq_msg_default;
>>         unsigned int    mq_msgsize_default;
>>
>> +#ifdef CONFIG_ANDROID_BINDER_IPC
>> +       /* next fields are for binder */
>> +       struct mutex      binder_procs_lock;
>> +       struct hlist_head binder_procs;
>> +       struct hlist_head binder_contexts;
>> +#endif
>> +
>>         /* user_ns which owns the ipc ns */
>>         struct user_namespace *user_ns;
>>         struct ucounts *ucounts;
>> @@ -118,6 +125,14 @@ extern int mq_init_ns(struct ipc_namespace *ns);
>>  static inline int mq_init_ns(struct ipc_namespace *ns) { return 0; }
>>  #endif
>>
>> +#ifdef CONFIG_ANDROID_BINDER_IPC
>> +extern int binder_init_ns(struct ipc_namespace *ns);
>> +extern void binder_exit_ns(struct ipc_namespace *ns);
>> +#else
>> +static inline int binder_init_ns(struct ipc_namespace *ns) { return
>0; }
>> +static inline void binder_exit_ns(struct ipc_namespace *ns) { }
>> +#endif
>> +
>>  #if defined(CONFIG_IPC_NS)
>>  extern struct ipc_namespace *copy_ipcs(unsigned long flags,
>>         struct user_namespace *user_ns, struct ipc_namespace *ns);
>> diff --git a/ipc/namespace.c b/ipc/namespace.c
>> index 21607791d62c..68c6e983b002 100644
>> --- a/ipc/namespace.c
>> +++ b/ipc/namespace.c
>> @@ -57,7 +57,10 @@ static struct ipc_namespace *create_ipc_ns(struct
>user_namespace *user_ns,
>>
>>         err = mq_init_ns(ns);
>>         if (err)
>> -               goto fail_put;
>> +               goto fail_init_mq;
>> +       err = binder_init_ns(ns);
>> +       if (err)
>> +               goto fail_init_binder;
>>
>>         sem_init_ns(ns);
>>         msg_init_ns(ns);
>> @@ -65,7 +68,9 @@ static struct ipc_namespace *create_ipc_ns(struct
>user_namespace *user_ns,
>>
>>         return ns;
>>
>> -fail_put:
>> +fail_init_binder:
>> +       mq_put_mnt(ns);
>> +fail_init_mq:
>>         put_user_ns(ns->user_ns);
>>         ns_free_inum(&ns->ns);
>>  fail_free:
>> @@ -120,6 +125,7 @@ static void free_ipc_ns(struct ipc_namespace *ns)
>>         sem_exit_ns(ns);
>>         msg_exit_ns(ns);
>>         shm_exit_ns(ns);
>> +       binder_exit_ns(ns);
>>
>>         dec_ipc_namespaces(ns->ucounts);
>>         put_user_ns(ns->user_ns);
>> --
>> 2.11.0