* [PATCH 5.4 1/3] binder: use euid from cred instead of using task
@ 2021-11-10 23:00 Todd Kjos
2021-11-10 23:00 ` [PATCH 5.4 2/3] binder: use cred instead of task for selinux checks Todd Kjos
2021-11-10 23:00 ` [PATCH 5.4 3/3] binder: use cred instead of task for getsecid Todd Kjos
0 siblings, 2 replies; 3+ messages in thread
From: Todd Kjos @ 2021-11-10 23:00 UTC (permalink / raw)
To: stable, gregkh, arve, tkjos, maco, christian, jmorris, serge,
paul, stephen.smalley.work, eparis, keescook, jannh, jeffv,
zohar, linux-security-module, selinux, devel
Cc: joel, kernel-team, Todd Kjos, Casey Schaufler
commit 29bc22ac5e5bc63275e850f0c8fc549e3d0e306b upstream.
Save the 'struct cred' associated with a binder process
at initial open to avoid potential race conditions
when converting to an euid.
Set a transaction's sender_euid from the 'struct cred'
saved at binder_open() instead of looking up the euid
from the binder proc's 'struct task'. This ensures
the euid is associated with the security context that
of the task that opened binder.
Cc: stable@vger.kernel.org # 4.4+
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Signed-off-by: Todd Kjos <tkjos@google.com>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Suggested-by: Jann Horn <jannh@google.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Change-Id: I91922e7f359df5901749f1b09094c3c68d45aed4
---
drivers/android/binder.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4eaef780844e..64f6fb3b1f66 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -424,6 +424,9 @@ enum binder_deferred_state {
* (invariant after initialized)
* @tsk task_struct for group_leader of process
* (invariant after initialized)
+ * @cred struct cred associated with the `struct file`
+ * in binder_open()
+ * (invariant after initialized)
* @deferred_work_node: element for binder_deferred_list
* (protected by binder_deferred_lock)
* @deferred_work: bitmap of deferred work to perform
@@ -469,6 +472,7 @@ struct binder_proc {
struct list_head waiting_threads;
int pid;
struct task_struct *tsk;
+ const struct cred *cred;
struct hlist_node deferred_work_node;
int deferred_work;
bool is_dead;
@@ -3092,7 +3096,7 @@ static void binder_transaction(struct binder_proc *proc,
t->from = thread;
else
t->from = NULL;
- t->sender_euid = task_euid(proc->tsk);
+ t->sender_euid = proc->cred->euid;
t->to_proc = target_proc;
t->to_thread = target_thread;
t->code = tr->code;
@@ -4707,6 +4711,7 @@ static void binder_free_proc(struct binder_proc *proc)
}
binder_alloc_deferred_release(&proc->alloc);
put_task_struct(proc->tsk);
+ put_cred(proc->cred);
binder_stats_deleted(BINDER_STAT_PROC);
kfree(proc);
}
@@ -5234,6 +5239,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
spin_lock_init(&proc->outer_lock);
get_task_struct(current->group_leader);
proc->tsk = current->group_leader;
+ proc->cred = get_cred(filp->f_cred);
INIT_LIST_HEAD(&proc->todo);
proc->default_priority = task_nice(current);
/* binderfs stashes devices in i_private */
--
2.34.0.rc0.344.g81b53c2807-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 5.4 2/3] binder: use cred instead of task for selinux checks
2021-11-10 23:00 [PATCH 5.4 1/3] binder: use euid from cred instead of using task Todd Kjos
@ 2021-11-10 23:00 ` Todd Kjos
2021-11-10 23:00 ` [PATCH 5.4 3/3] binder: use cred instead of task for getsecid Todd Kjos
1 sibling, 0 replies; 3+ messages in thread
From: Todd Kjos @ 2021-11-10 23:00 UTC (permalink / raw)
To: stable, gregkh, arve, tkjos, maco, christian, jmorris, serge,
paul, stephen.smalley.work, eparis, keescook, jannh, jeffv,
zohar, linux-security-module, selinux, devel
Cc: joel, kernel-team, Todd Kjos, Casey Schaufler
commit 52f88693378a58094c538662ba652aff0253c4fe upstream.
Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.
Fix by using the 'struct cred' saved during binder_open and pass
it to the selinux subsystem.
Cc: stable@vger.kernel.org # 5.14 (need backport for earlier stables)
Fixes: 79af73079d75 ("Add security hooks to binder and implement the hooks for SELinux.")
Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Change-Id: Id7157515d2b08f11683aeb8ad9b8f1da075d34e7
---
drivers/android/binder.c | 12 ++++++------
include/linux/lsm_hooks.h | 28 ++++++++++++++--------------
include/linux/security.h | 28 ++++++++++++++--------------
security/security.c | 14 +++++++-------
security/selinux/hooks.c | 36 +++++++++++++++---------------------
5 files changed, 56 insertions(+), 62 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 64f6fb3b1f66..cebb2cd1876c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2447,7 +2447,7 @@ static int binder_translate_binder(struct flat_binder_object *fp,
ret = -EINVAL;
goto done;
}
- if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) {
+ if (security_binder_transfer_binder(proc->cred, target_proc->cred)) {
ret = -EPERM;
goto done;
}
@@ -2493,7 +2493,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
proc->pid, thread->pid, fp->handle);
return -EINVAL;
}
- if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) {
+ if (security_binder_transfer_binder(proc->cred, target_proc->cred)) {
ret = -EPERM;
goto done;
}
@@ -2581,7 +2581,7 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
ret = -EBADF;
goto err_fget;
}
- ret = security_binder_transfer_file(proc->tsk, target_proc->tsk, file);
+ ret = security_binder_transfer_file(proc->cred, target_proc->cred, file);
if (ret < 0) {
ret = -EPERM;
goto err_security;
@@ -2980,8 +2980,8 @@ static void binder_transaction(struct binder_proc *proc,
return_error_line = __LINE__;
goto err_invalid_target_handle;
}
- if (security_binder_transaction(proc->tsk,
- target_proc->tsk) < 0) {
+ if (security_binder_transaction(proc->cred,
+ target_proc->cred) < 0) {
return_error = BR_FAILED_REPLY;
return_error_param = -EPERM;
return_error_line = __LINE__;
@@ -4922,7 +4922,7 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp,
ret = -EBUSY;
goto out;
}
- ret = security_binder_set_context_mgr(proc->tsk);
+ ret = security_binder_set_context_mgr(proc->cred);
if (ret < 0)
goto out;
if (uid_valid(context->binder_context_mgr_uid)) {
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a3763247547c..a21dc5413653 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1241,22 +1241,22 @@
*
* @binder_set_context_mgr:
* Check whether @mgr is allowed to be the binder context manager.
- * @mgr contains the task_struct for the task being registered.
+ * @mgr contains the struct cred for the current binder process.
* Return 0 if permission is granted.
* @binder_transaction:
* Check whether @from is allowed to invoke a binder transaction call
* to @to.
- * @from contains the task_struct for the sending task.
- * @to contains the task_struct for the receiving task.
+ * @from contains the struct cred for the sending process.
+ * @to contains the struct cred for the receiving process.
* @binder_transfer_binder:
* Check whether @from is allowed to transfer a binder reference to @to.
- * @from contains the task_struct for the sending task.
- * @to contains the task_struct for the receiving task.
+ * @from contains the struct cred for the sending process.
+ * @to contains the struct cred for the receiving process.
* @binder_transfer_file:
* Check whether @from is allowed to transfer @file to @to.
- * @from contains the task_struct for the sending task.
+ * @from contains the struct cred for the sending process.
* @file contains the struct file being transferred.
- * @to contains the task_struct for the receiving task.
+ * @to contains the struct cred for the receiving process.
*
* @ptrace_access_check:
* Check permission before allowing the current process to trace the
@@ -1456,13 +1456,13 @@
* @what: kernel feature being accessed
*/
union security_list_options {
- int (*binder_set_context_mgr)(struct task_struct *mgr);
- int (*binder_transaction)(struct task_struct *from,
- struct task_struct *to);
- int (*binder_transfer_binder)(struct task_struct *from,
- struct task_struct *to);
- int (*binder_transfer_file)(struct task_struct *from,
- struct task_struct *to,
+ int (*binder_set_context_mgr)(const struct cred *mgr);
+ int (*binder_transaction)(const struct cred *from,
+ const struct cred *to);
+ int (*binder_transfer_binder)(const struct cred *from,
+ const struct cred *to);
+ int (*binder_transfer_file)(const struct cred *from,
+ const struct cred *to,
struct file *file);
int (*ptrace_access_check)(struct task_struct *child,
diff --git a/include/linux/security.h b/include/linux/security.h
index df90399a8af9..0d4cb64cae1f 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -249,13 +249,13 @@ extern int security_init(void);
extern int early_security_init(void);
/* Security operations */
-int security_binder_set_context_mgr(struct task_struct *mgr);
-int security_binder_transaction(struct task_struct *from,
- struct task_struct *to);
-int security_binder_transfer_binder(struct task_struct *from,
- struct task_struct *to);
-int security_binder_transfer_file(struct task_struct *from,
- struct task_struct *to, struct file *file);
+int security_binder_set_context_mgr(const struct cred *mgr);
+int security_binder_transaction(const struct cred *from,
+ const struct cred *to);
+int security_binder_transfer_binder(const struct cred *from,
+ const struct cred *to);
+int security_binder_transfer_file(const struct cred *from,
+ const struct cred *to, struct file *file);
int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
int security_ptrace_traceme(struct task_struct *parent);
int security_capget(struct task_struct *target,
@@ -481,25 +481,25 @@ static inline int early_security_init(void)
return 0;
}
-static inline int security_binder_set_context_mgr(struct task_struct *mgr)
+static inline int security_binder_set_context_mgr(const struct cred *mgr)
{
return 0;
}
-static inline int security_binder_transaction(struct task_struct *from,
- struct task_struct *to)
+static inline int security_binder_transaction(const struct cred *from,
+ const struct cred *to)
{
return 0;
}
-static inline int security_binder_transfer_binder(struct task_struct *from,
- struct task_struct *to)
+static inline int security_binder_transfer_binder(const struct cred *from,
+ const struct cred *to)
{
return 0;
}
-static inline int security_binder_transfer_file(struct task_struct *from,
- struct task_struct *to,
+static inline int security_binder_transfer_file(const struct cred *from,
+ const struct cred *to,
struct file *file)
{
return 0;
diff --git a/security/security.c b/security/security.c
index 1bc000f834e2..c34ec4c7d98c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -670,25 +670,25 @@ static void __init lsm_early_task(struct task_struct *task)
/* Security operations */
-int security_binder_set_context_mgr(struct task_struct *mgr)
+int security_binder_set_context_mgr(const struct cred *mgr)
{
return call_int_hook(binder_set_context_mgr, 0, mgr);
}
-int security_binder_transaction(struct task_struct *from,
- struct task_struct *to)
+int security_binder_transaction(const struct cred *from,
+ const struct cred *to)
{
return call_int_hook(binder_transaction, 0, from, to);
}
-int security_binder_transfer_binder(struct task_struct *from,
- struct task_struct *to)
+int security_binder_transfer_binder(const struct cred *from,
+ const struct cred *to)
{
return call_int_hook(binder_transfer_binder, 0, from, to);
}
-int security_binder_transfer_file(struct task_struct *from,
- struct task_struct *to, struct file *file)
+int security_binder_transfer_file(const struct cred *from,
+ const struct cred *to, struct file *file)
{
return call_int_hook(binder_transfer_file, 0, from, to, file);
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 717a398ef4d0..8b9cbe1e8ee2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2050,22 +2050,19 @@ static inline u32 open_file_to_av(struct file *file)
/* Hook functions begin here. */
-static int selinux_binder_set_context_mgr(struct task_struct *mgr)
+static int selinux_binder_set_context_mgr(const struct cred *mgr)
{
- u32 mysid = current_sid();
- u32 mgrsid = task_sid(mgr);
-
return avc_has_perm(&selinux_state,
- mysid, mgrsid, SECCLASS_BINDER,
+ current_sid(), cred_sid(mgr), SECCLASS_BINDER,
BINDER__SET_CONTEXT_MGR, NULL);
}
-static int selinux_binder_transaction(struct task_struct *from,
- struct task_struct *to)
+static int selinux_binder_transaction(const struct cred *from,
+ const struct cred *to)
{
u32 mysid = current_sid();
- u32 fromsid = task_sid(from);
- u32 tosid = task_sid(to);
+ u32 fromsid = cred_sid(from);
+ u32 tosid = cred_sid(to);
int rc;
if (mysid != fromsid) {
@@ -2076,27 +2073,24 @@ static int selinux_binder_transaction(struct task_struct *from,
return rc;
}
- return avc_has_perm(&selinux_state,
- fromsid, tosid, SECCLASS_BINDER, BINDER__CALL,
- NULL);
+ return avc_has_perm(&selinux_state, fromsid, tosid,
+ SECCLASS_BINDER, BINDER__CALL, NULL);
}
-static int selinux_binder_transfer_binder(struct task_struct *from,
- struct task_struct *to)
+static int selinux_binder_transfer_binder(const struct cred *from,
+ const struct cred *to)
{
- u32 fromsid = task_sid(from);
- u32 tosid = task_sid(to);
-
return avc_has_perm(&selinux_state,
- fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
+ cred_sid(from), cred_sid(to),
+ SECCLASS_BINDER, BINDER__TRANSFER,
NULL);
}
-static int selinux_binder_transfer_file(struct task_struct *from,
- struct task_struct *to,
+static int selinux_binder_transfer_file(const struct cred *from,
+ const struct cred *to,
struct file *file)
{
- u32 sid = task_sid(to);
+ u32 sid = cred_sid(to);
struct file_security_struct *fsec = selinux_file(file);
struct dentry *dentry = file->f_path.dentry;
struct inode_security_struct *isec;
--
2.34.0.rc0.344.g81b53c2807-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 5.4 3/3] binder: use cred instead of task for getsecid
2021-11-10 23:00 [PATCH 5.4 1/3] binder: use euid from cred instead of using task Todd Kjos
2021-11-10 23:00 ` [PATCH 5.4 2/3] binder: use cred instead of task for selinux checks Todd Kjos
@ 2021-11-10 23:00 ` Todd Kjos
1 sibling, 0 replies; 3+ messages in thread
From: Todd Kjos @ 2021-11-10 23:00 UTC (permalink / raw)
To: stable, gregkh, arve, tkjos, maco, christian, jmorris, serge,
paul, stephen.smalley.work, eparis, keescook, jannh, jeffv,
zohar, linux-security-module, selinux, devel
Cc: joel, kernel-team, Todd Kjos, kernel test robot, Casey Schaufler
commit 4d5b5539742d2554591751b4248b0204d20dcc9d upstream.
Use the 'struct cred' saved at binder_open() to lookup
the security ID via security_cred_getsecid(). This
ensures that the security context that opened binder
is the one used to generate the secctx.
Cc: stable@vger.kernel.org # 5.4+
Fixes: ec74136ded79 ("binder: create node flag to request sender's security context")
Signed-off-by: Todd Kjos <tkjos@google.com>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Change-Id: Ia7b59804a0bdbd51191bbcca556414840307c623
---
drivers/android/binder.c | 2 +-
include/linux/security.h | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cebb2cd1876c..c2c411c458b2 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3107,7 +3107,7 @@ static void binder_transaction(struct binder_proc *proc,
u32 secid;
size_t added_size;
- security_task_getsecid(proc->tsk, &secid);
+ security_cred_getsecid(proc->cred, &secid);
ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
if (ret) {
return_error = BR_FAILED_REPLY;
diff --git a/include/linux/security.h b/include/linux/security.h
index 0d4cb64cae1f..3f6b8195ae9e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -985,6 +985,11 @@ static inline void security_transfer_creds(struct cred *new,
{
}
+static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
+{
+ *secid = 0;
+}
+
static inline int security_kernel_act_as(struct cred *cred, u32 secid)
{
return 0;
--
2.34.0.rc0.344.g81b53c2807-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-10 23:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 23:00 [PATCH 5.4 1/3] binder: use euid from cred instead of using task Todd Kjos
2021-11-10 23:00 ` [PATCH 5.4 2/3] binder: use cred instead of task for selinux checks Todd Kjos
2021-11-10 23:00 ` [PATCH 5.4 3/3] binder: use cred instead of task for getsecid Todd Kjos
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).