Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190114171021.86171-1-tkjos@google.com> <20190114183316.GA199154@google.com>
In-Reply-To: <20190114183316.GA199154@google.com>
From:   Todd Kjos <tkjos@google.com>
Date:   Mon, 14 Jan 2019 10:50:24 -0800
Message-ID: <CAHRSSEx7BMMZ1CFoQiOxUzPfO1+MFuSEF4Xo=oN5h+gUZb=4JQ@mail.gmail.com>
Subject: Re: [PATCH v3] binder: create node flag to request sender's security context
To:     Joel Fernandes <joelaf@google.com>
Cc:     Todd Kjos <tkjos@android.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Martijn Coenen <maco@google.com>, joel@joelfernandes.org,
        Android Kernel Team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 14, 2019 at 10:33 AM Joel Fernandes <joelaf@google.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@google.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.

>
> thanks,
>
>  - Joel
>
>