Re: [PATCH v3] binder: create node flag to request sender's security context

[Joel Fernandes]

On Mon, Jan 14, 2019 at 10:50:24AM -0800, Todd Kjos wrote:
> On Mon, Jan 14, 2019 at 10:33 AM Joel Fernandes <joelaf@xxxxxxxxxx> wrote:
> >
> > On Mon, Jan 14, 2019 at 09:10:21AM -0800, Todd Kjos wrote:
> > > To allow servers to verify client identity, allow a node
> > > flag to be set that causes the sender's security context
> > > to be delivered with the transaction. The BR_TRANSACTION
> > > command is extended in BR_TRANSACTION_SEC_CTX to
> > > contain a pointer to the security context string.
> > >
> > > Signed-off-by: Todd Kjos <tkjos@xxxxxxxxxx>
> > > ---
> > > v2: fix 32-bit build warning
> > > v3: fix smatch warning on unitialized struct element
> > >
> > >  drivers/android/binder.c            | 106 ++++++++++++++++++++++------
> > >  include/uapi/linux/android/binder.h |  19 +++++
> > >  2 files changed, 102 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index cdfc87629efb8..5f6ef5e63b91e 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -329,6 +329,8 @@ struct binder_error {
> > >   *                        (invariant after initialized)
> > >   * @min_priority:         minimum scheduling priority
> > >   *                        (invariant after initialized)
> > > + * @txn_security_ctx:     require sender's security context
> > > + *                        (invariant after initialized)
> > >   * @async_todo:           list of async work items
> > >   *                        (protected by @proc->inner_lock)
> > >   *
> > > @@ -365,6 +367,7 @@ struct binder_node {
> > >                * invariant after initialization
> > >                */
> > >               u8 accept_fds:1;
> > > +             u8 txn_security_ctx:1;
> > >               u8 min_priority;
> > >       };
> > >       bool has_async_transaction;
> > > @@ -615,6 +618,7 @@ struct binder_transaction {
> > >       long    saved_priority;
> > >       kuid_t  sender_euid;
> > >       struct list_head fd_fixups;
> > > +     binder_uintptr_t security_ctx;
> > >       /**
> > >        * @lock:  protects @from, @to_proc, and @to_thread
> > >        *
> > > @@ -1152,6 +1156,7 @@ static struct binder_node *binder_init_node_ilocked(
> > >       node->work.type = BINDER_WORK_NODE;
> > >       node->min_priority = flags & FLAT_BINDER_FLAG_PRIORITY_MASK;
> > >       node->accept_fds = !!(flags & FLAT_BINDER_FLAG_ACCEPTS_FDS);
> > > +     node->txn_security_ctx = !!(flags & FLAT_BINDER_FLAG_TXN_SECURITY_CTX);
> > >       spin_lock_init(&node->lock);
> > >       INIT_LIST_HEAD(&node->work.entry);
> > >       INIT_LIST_HEAD(&node->async_todo);
> > > @@ -2778,6 +2783,8 @@ static void binder_transaction(struct binder_proc *proc,
> > >       binder_size_t last_fixup_min_off = 0;
> > >       struct binder_context *context = proc->context;
> > >       int t_debug_id = atomic_inc_return(&binder_last_id);
> > > +     char *secctx = NULL;
> > > +     u32 secctx_sz = 0;
> > >
> > >       e = binder_transaction_log_add(&binder_transaction_log);
> > >       e->debug_id = t_debug_id;
> > > @@ -3020,6 +3027,20 @@ static void binder_transaction(struct binder_proc *proc,
> > >       t->flags = tr->flags;
> > >       t->priority = task_nice(current);
> > >
> > > +     if (target_node && target_node->txn_security_ctx) {
> > > +             u32 secid;
> > > +
> > > +             security_task_getsecid(proc->tsk, &secid);
> > > +             ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
> > > +             if (ret) {
> > > +                     return_error = BR_FAILED_REPLY;
> > > +                     return_error_param = ret;
> > > +                     return_error_line = __LINE__;
> > > +                     goto err_get_secctx_failed;
> > > +             }
> > > +             extra_buffers_size += ALIGN(secctx_sz, sizeof(u64));
> > > +     }
> > > +
> > >       trace_binder_transaction(reply, t, target_node);
> > >
> > >       t->buffer = binder_alloc_new_buf(&target_proc->alloc, tr->data_size,
> > > @@ -3036,6 +3057,19 @@ static void binder_transaction(struct binder_proc *proc,
> > >               t->buffer = NULL;
> > >               goto err_binder_alloc_buf_failed;
> > >       }
> > > +     if (secctx) {
> > > +             size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
> > > +                                 ALIGN(tr->offsets_size, sizeof(void *)) +
> > > +                                 ALIGN(extra_buffers_size, sizeof(void *)) -
> > > +                                 ALIGN(secctx_sz, sizeof(u64));
> > > +             char *kptr = t->buffer->data + buf_offset;
> > > +
> > > +             t->security_ctx = (uintptr_t)kptr +
> > > +                 binder_alloc_get_user_buffer_offset(&target_proc->alloc);
> > > +             memcpy(kptr, secctx, secctx_sz);
> >
> > Just for my clarification, instead of storing the string in the transaction
> > buffer, would it not be better to store the security context id in
> > t->security_ctx, and then do the conversion to string later, during
> > binder_thread_read? Then some space will also be saved in the transaction
> > buffer?
> 
> The string has to be somewhere in the address space of the target. We
> allocate the space in the binder buffer and copy it here. This is a
> convenient place to allocate the space since we know the size at the
> time of allocation. Presumably, in your proposal, we would copy it
> later into the output buffer...which is allocated by the userspace so
> we don't have complete control of it in the kernel. Doing that would
> make binder_thread_read() more complex since we'd need to make sure
> there is sizeof(tr) + secctx_sz always available in the output buffer
> (which is allocated by userspace). Saving a few bytes in the this
> buffer is of little value, but keeping the output buffer management
> simple is valuable.

Makes sense now, thanks. I agree its simpler this way.

Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>

thanks,

 - Joel