On Mon, Jan 14, 2019 at 10:50:24AM -0800, Todd Kjos wrote:
> On Mon, Jan 14, 2019 at 10:33 AM Joel Fernandes 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
> > > ---
> > > 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 {
> > > longsaved_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(>lock);
> > > INIT_LIST_HEAD(>work.entry);
> > > INIT_LIST_HEAD(>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(_last_id);
> > > + char *secctx = NULL;
> > > + u32 secctx_sz = 0;
> > >
> > > e = binder_transaction_log_add(_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, );
> > > + ret = security_secid_to_secctx(secid, , _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(_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(_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
