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;
Date:   Mon, 14 Jan 2019 13:33:16 -0500
From:   Joel Fernandes <joelaf@google.com>
To:     Todd Kjos <tkjos@android.com>
Cc:     tkjos@google.com, gregkh@linuxfoundation.org, arve@android.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        maco@google.com, joel@joelfernandes.org, kernel-team@android.com
Subject: Re: [PATCH v3] binder: create node flag to request sender's security
 context
Message-ID: <20190114183316.GA199154@google.com>
References: <20190114171021.86171-1-tkjos@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190114171021.86171-1-tkjos@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?

thanks,

 - Joel