binder: use cred instead of task for selinux checks

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.

Mot-CRs-fixed: (CR)
CVE-Fixed: CVE-2021-39686
Bug: 200688826

Change-Id: I0c478e38a70defd40ac4590656af528ce0b90f35
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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Gajjala Chakradhar <gajjalac@motorola.com>
Reviewed-on: https://gerrit.mot.com/2197696
SME-Granted: SME Approvals Granted
SLTApproved: Slta Waiver
Tested-by: Jira Key
Reviewed-by: Xiangpo Zhao <zhaoxp3@motorola.com>
Submit-Approved: Jira Key

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9a4c0b992d98b2125cb5ecff26d9c19431e29921..d273f2c11403d423c9a7fbd2529f494020bb8b7e 100644 (file)
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2587,7 +2587,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;
        }
@@ -2633,7 +2633,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
                                  proc->pid, thread->pid, proc->tsk->comm, thread->task->comm, 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;
        }
@@ -2717,7 +2717,7 @@ static int binder_translate_fd(int fd,
                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;
@@ -3108,8 +3108,8 @@ static void binder_transaction(struct binder_proc *proc,
                        goto err_dead_binder;
                }
                e->to_node = target_node->debug_id;
-               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__;
@@ -4867,7 +4867,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 7161d8e7ee79246ffca220805826f883f26d7ddd..9a520f6f99dee53b8efca489cb4e8cf0f2063fa0 100644 (file)
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1170,22 +1170,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
@@ -1387,13 +1387,13 @@
  *
  */
 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 73f1ef625d40c900430778fab29f8bad6cd2e029..c99ab938129a2e76c0bd86f8caebae758aecda9a 100644 (file)
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -197,13 +197,13 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
 extern int 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,
@@ -424,25 +424,25 @@ static inline int 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 fb4910f0d0e258247bbf1567db414dc470166ca6..c4ab2587b27b2348288a10e1df2102566a5fa25a 100644 (file)
--- a/security/security.c
+++ b/security/security.c
@@ -223,25 +223,25 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
 
 /* 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 3bbfca4accd8445fe1e2e03fadc4f3a8713f7035..3f2614630db573cef93c9c65cc720b9d342322c4 100644 (file)
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2139,21 +2139,18 @@ 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(mysid, mgrsid, SECCLASS_BINDER,
+       return avc_has_perm(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) {
@@ -2167,21 +2164,19 @@ static int selinux_binder_transaction(struct task_struct *from,
                            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(fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
+       return avc_has_perm(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 = file->f_security;
        struct dentry *dentry = file->f_path.dentry;
        struct inode_security_struct *isec;