| 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: <20190114195516.GA227294@google.com>
Date: Mon, 14 Jan 2019 14:55:16 -0500
From: Joel Fernandes <joel@...lfernandes.org>
To: Todd Kjos <tkjos@...gle.com>
Cc: Todd Kjos <tkjos@...roid.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
"open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
LKML <linux-kernel@...r.kernel.org>,
Martijn Coenen <maco@...gle.com>,
Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH v3] binder: create node flag to request sender's security
context
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@...gle.com> 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@...gle.com>
> > > ---
> > > 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@...lfernandes.org>
thanks,
- Joel
Powered by blists - more mailing lists