* [PATCH v3] binder: create node flag to request sender's security context
@ 2019-01-14 17:10 Todd Kjos
2019-01-14 18:33 ` Joel Fernandes
0 siblings, 1 reply; 4+ messages in thread
From: Todd Kjos @ 2019-01-14 17:10 UTC (permalink / raw)
To: tkjos, gregkh, arve, devel, linux-kernel, maco; +Cc: joel, kernel-team
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);
+ security_release_secctx(secctx, secctx_sz);
+ secctx = NULL;
+ }
t->buffer->debug_id = t->debug_id;
t->buffer->transaction = t;
t->buffer->target_node = target_node;
@@ -3305,6 +3339,9 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer->transaction = NULL;
binder_alloc_free_buf(&target_proc->alloc, t->buffer);
err_binder_alloc_buf_failed:
+ if (secctx)
+ security_release_secctx(secctx, secctx_sz);
+err_get_secctx_failed:
kfree(tcomplete);
binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
err_alloc_tcomplete_failed:
@@ -4036,11 +4073,13 @@ static int binder_thread_read(struct binder_proc *proc,
while (1) {
uint32_t cmd;
- struct binder_transaction_data tr;
+ struct binder_transaction_data_secctx tr;
+ struct binder_transaction_data *trd = &tr.transaction_data;
struct binder_work *w = NULL;
struct list_head *list = NULL;
struct binder_transaction *t = NULL;
struct binder_thread *t_from;
+ size_t trsize = sizeof(*trd);
binder_inner_proc_lock(proc);
if (!binder_worklist_empty_ilocked(&thread->todo))
@@ -4240,8 +4279,8 @@ static int binder_thread_read(struct binder_proc *proc,
if (t->buffer->target_node) {
struct binder_node *target_node = t->buffer->target_node;
- tr.target.ptr = target_node->ptr;
- tr.cookie = target_node->cookie;
+ trd->target.ptr = target_node->ptr;
+ trd->cookie = target_node->cookie;
t->saved_priority = task_nice(current);
if (t->priority < target_node->min_priority &&
!(t->flags & TF_ONE_WAY))
@@ -4251,22 +4290,23 @@ static int binder_thread_read(struct binder_proc *proc,
binder_set_nice(target_node->min_priority);
cmd = BR_TRANSACTION;
} else {
- tr.target.ptr = 0;
- tr.cookie = 0;
+ trd->target.ptr = 0;
+ trd->cookie = 0;
cmd = BR_REPLY;
}
- tr.code = t->code;
- tr.flags = t->flags;
- tr.sender_euid = from_kuid(current_user_ns(), t->sender_euid);
+ trd->code = t->code;
+ trd->flags = t->flags;
+ trd->sender_euid = from_kuid(current_user_ns(), t->sender_euid);
t_from = binder_get_txn_from(t);
if (t_from) {
struct task_struct *sender = t_from->proc->tsk;
- tr.sender_pid = task_tgid_nr_ns(sender,
- task_active_pid_ns(current));
+ trd->sender_pid =
+ task_tgid_nr_ns(sender,
+ task_active_pid_ns(current));
} else {
- tr.sender_pid = 0;
+ trd->sender_pid = 0;
}
ret = binder_apply_fd_fixups(t);
@@ -4297,15 +4337,20 @@ static int binder_thread_read(struct binder_proc *proc,
}
continue;
}
- tr.data_size = t->buffer->data_size;
- tr.offsets_size = t->buffer->offsets_size;
- tr.data.ptr.buffer = (binder_uintptr_t)
+ trd->data_size = t->buffer->data_size;
+ trd->offsets_size = t->buffer->offsets_size;
+ trd->data.ptr.buffer = (binder_uintptr_t)
((uintptr_t)t->buffer->data +
binder_alloc_get_user_buffer_offset(&proc->alloc));
- tr.data.ptr.offsets = tr.data.ptr.buffer +
+ trd->data.ptr.offsets = trd->data.ptr.buffer +
ALIGN(t->buffer->data_size,
sizeof(void *));
+ tr.secctx = t->security_ctx;
+ if (t->security_ctx) {
+ cmd = BR_TRANSACTION_SEC_CTX;
+ trsize = sizeof(tr);
+ }
if (put_user(cmd, (uint32_t __user *)ptr)) {
if (t_from)
binder_thread_dec_tmpref(t_from);
@@ -4316,7 +4361,7 @@ static int binder_thread_read(struct binder_proc *proc,
return -EFAULT;
}
ptr += sizeof(uint32_t);
- if (copy_to_user(ptr, &tr, sizeof(tr))) {
+ if (copy_to_user(ptr, &tr, trsize)) {
if (t_from)
binder_thread_dec_tmpref(t_from);
@@ -4325,7 +4370,7 @@ static int binder_thread_read(struct binder_proc *proc,
return -EFAULT;
}
- ptr += sizeof(tr);
+ ptr += trsize;
trace_binder_transaction_received(t);
binder_stat_br(proc, thread, cmd);
@@ -4333,16 +4378,18 @@ static int binder_thread_read(struct binder_proc *proc,
"%d:%d %s %d %d:%d, cmd %d size %zd-%zd ptr %016llx-%016llx\n",
proc->pid, thread->pid,
(cmd == BR_TRANSACTION) ? "BR_TRANSACTION" :
- "BR_REPLY",
+ (cmd == BR_TRANSACTION_SEC_CTX) ?
+ "BR_TRANSACTION_SEC_CTX" : "BR_REPLY",
t->debug_id, t_from ? t_from->proc->pid : 0,
t_from ? t_from->pid : 0, cmd,
t->buffer->data_size, t->buffer->offsets_size,
- (u64)tr.data.ptr.buffer, (u64)tr.data.ptr.offsets);
+ (u64)trd->data.ptr.buffer,
+ (u64)trd->data.ptr.offsets);
if (t_from)
binder_thread_dec_tmpref(t_from);
t->buffer->allow_user_free = 1;
- if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) {
+ if (cmd != BR_REPLY && !(t->flags & TF_ONE_WAY)) {
binder_inner_proc_lock(thread->proc);
t->to_parent = thread->transaction_stack;
t->to_thread = thread;
@@ -4690,7 +4737,8 @@ static int binder_ioctl_write_read(struct file *filp,
return ret;
}
-static int binder_ioctl_set_ctx_mgr(struct file *filp)
+static int binder_ioctl_set_ctx_mgr(struct file *filp,
+ struct flat_binder_object *fbo)
{
int ret = 0;
struct binder_proc *proc = filp->private_data;
@@ -4719,7 +4767,7 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
} else {
context->binder_context_mgr_uid = curr_euid;
}
- new_node = binder_new_node(proc, NULL);
+ new_node = binder_new_node(proc, fbo);
if (!new_node) {
ret = -ENOMEM;
goto out;
@@ -4842,8 +4890,20 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
binder_inner_proc_unlock(proc);
break;
}
+ case BINDER_SET_CONTEXT_MGR_EXT: {
+ struct flat_binder_object fbo;
+
+ if (copy_from_user(&fbo, ubuf, sizeof(fbo))) {
+ ret = -EINVAL;
+ goto err;
+ }
+ ret = binder_ioctl_set_ctx_mgr(filp, &fbo);
+ if (ret)
+ goto err;
+ break;
+ }
case BINDER_SET_CONTEXT_MGR:
- ret = binder_ioctl_set_ctx_mgr(filp);
+ ret = binder_ioctl_set_ctx_mgr(filp, NULL);
if (ret)
goto err;
break;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index b9ba520f7e4bb..2832134e53971 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -41,6 +41,14 @@ enum {
enum {
FLAT_BINDER_FLAG_PRIORITY_MASK = 0xff,
FLAT_BINDER_FLAG_ACCEPTS_FDS = 0x100,
+
+ /**
+ * @FLAT_BINDER_FLAG_TXN_SECURITY_CTX: request security contexts
+ *
+ * Only when set, causes senders to include their security
+ * context
+ */
+ FLAT_BINDER_FLAG_TXN_SECURITY_CTX = 0x1000,
};
#ifdef BINDER_IPC_32BIT
@@ -218,6 +226,7 @@ struct binder_node_info_for_ref {
#define BINDER_VERSION _IOWR('b', 9, struct binder_version)
#define BINDER_GET_NODE_DEBUG_INFO _IOWR('b', 11, struct binder_node_debug_info)
#define BINDER_GET_NODE_INFO_FOR_REF _IOWR('b', 12, struct binder_node_info_for_ref)
+#define BINDER_SET_CONTEXT_MGR_EXT _IOW('b', 13, struct flat_binder_object)
/*
* NOTE: Two special error codes you should check for when calling
@@ -276,6 +285,11 @@ struct binder_transaction_data {
} data;
};
+struct binder_transaction_data_secctx {
+ struct binder_transaction_data transaction_data;
+ binder_uintptr_t secctx;
+};
+
struct binder_transaction_data_sg {
struct binder_transaction_data transaction_data;
binder_size_t buffers_size;
@@ -311,6 +325,11 @@ enum binder_driver_return_protocol {
BR_OK = _IO('r', 1),
/* No parameters! */
+ BR_TRANSACTION_SEC_CTX = _IOR('r', 2,
+ struct binder_transaction_data_secctx),
+ /*
+ * binder_transaction_data_secctx: the received command.
+ */
BR_TRANSACTION = _IOR('r', 2, struct binder_transaction_data),
BR_REPLY = _IOR('r', 3, struct binder_transaction_data),
/*
--
2.20.1.97.g81188d93c3-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] binder: create node flag to request sender's security context
2019-01-14 17:10 [PATCH v3] binder: create node flag to request sender's security context Todd Kjos
@ 2019-01-14 18:33 ` Joel Fernandes
2019-01-14 18:50 ` Todd Kjos
0 siblings, 1 reply; 4+ messages in thread
From: Joel Fernandes @ 2019-01-14 18:33 UTC (permalink / raw)
To: Todd Kjos
Cc: tkjos, gregkh, arve, devel, linux-kernel, maco, joel, kernel-team
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] binder: create node flag to request sender's security context
2019-01-14 18:33 ` Joel Fernandes
@ 2019-01-14 18:50 ` Todd Kjos
2019-01-14 19:55 ` Joel Fernandes
0 siblings, 1 reply; 4+ messages in thread
From: Todd Kjos @ 2019-01-14 18:50 UTC (permalink / raw)
To: Joel Fernandes
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen, joel,
Android Kernel Team
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] binder: create node flag to request sender's security context
2019-01-14 18:50 ` Todd Kjos
@ 2019-01-14 19:55 ` Joel Fernandes
0 siblings, 0 replies; 4+ messages in thread
From: Joel Fernandes @ 2019-01-14 19:55 UTC (permalink / raw)
To: Todd Kjos
Cc: Todd Kjos, Greg Kroah-Hartman, Arve Hjønnevåg,
open list:ANDROID DRIVERS, LKML, Martijn Coenen,
Android Kernel Team
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@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.
Makes sense now, thanks. I agree its simpler this way.
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-14 19:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-14 17:10 [PATCH v3] binder: create node flag to request sender's security context Todd Kjos
2019-01-14 18:33 ` Joel Fernandes
2019-01-14 18:50 ` Todd Kjos
2019-01-14 19:55 ` Joel Fernandes
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.