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.

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: Paolo Pisati <paolo.pisati@canonical.com>

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 5e5374966262a170a9a4f672c11087a2d9a91ba4..55939a02c5b4d0ef12d48ae62d952106ca6960b8 100644 (file)
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2055,7 +2055,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;
        }
@@ -2101,7 +2101,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;
        }
@@ -2189,7 +2189,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;
@@ -2593,8 +2593,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__;
@@ -4563,7 +4563,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_hook_defs.h b/include/linux/lsm_hook_defs.h
index 517013ece679faae3d087022bf6108f19325ed0e..4c10750865c2043458a95e3df81265f3a7b85b51 100644 (file)
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -26,13 +26,13 @@
  *   #undef LSM_HOOK
  * };
  */
-LSM_HOOK(int, 0, binder_set_context_mgr, struct task_struct *mgr)
-LSM_HOOK(int, 0, binder_transaction, struct task_struct *from,
-        struct task_struct *to)
-LSM_HOOK(int, 0, binder_transfer_binder, struct task_struct *from,
-        struct task_struct *to)
-LSM_HOOK(int, 0, binder_transfer_file, struct task_struct *from,
-        struct task_struct *to, struct file *file)
+LSM_HOOK(int, 0, binder_set_context_mgr, const struct cred *mgr)
+LSM_HOOK(int, 0, binder_transaction, const struct cred *from,
+        const struct cred *to)
+LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
+        const struct cred *to)
+LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
+        const struct cred *to, struct file *file)
 LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
         unsigned int mode)
 LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index df2be786f139fe1eb6a0f936e039c8efc8311d2d..48b45bcb7762142d27502d15338d8cc2f792d1f9 100644 (file)
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1313,22 +1313,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

diff --git a/include/linux/security.h b/include/linux/security.h
index 06efccb821096d4d4bce899e7294d6fd2d7c48e7..cfc53e5ad6346f49cfe08d508a5766a80457425d 100644 (file)
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -382,13 +382,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,
@@ -635,25 +635,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 3c2dfc53445923134fbacce1513e40ffbe8196dd..8ac2f8c9d37911ca8905d648daeca0c854423eff 100644 (file)
--- a/security/security.c
+++ b/security/security.c
@@ -884,7 +884,7 @@ static int lsm_superblock_alloc(struct super_block *sb)
 
 /* 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);
 }
@@ -895,17 +895,17 @@ EXPORT_SYMBOL(security_binder_set_context_mgr);
  * @from: source of the transaction
  * @to: destination of the transaction
  *
- * Verify that the tasks have the same LSM "display", then
+ * Verify that the creds have the same LSM "display", then
  * call the security module hooks.
  *
  * Returns -EINVAL if the displays don't match, or the
  * result of the security module checks.
  */
-int security_binder_transaction(struct task_struct *from,
-                               struct task_struct *to)
+int security_binder_transaction(const struct cred *from,
+                               const struct cred *to)
 {
-       int from_display = lsm_task_display(from);
-       int to_display = lsm_task_display(to);
+       int from_display = lsm_cred_display(from);
+       int to_display = lsm_cred_display(to);
 
        /*
         * If the display is LSMBLOB_INVALID the first module that has
@@ -926,15 +926,15 @@ int security_binder_transaction(struct task_struct *from,
 }
 EXPORT_SYMBOL(security_binder_transaction);
 
-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);
 }
 EXPORT_SYMBOL(security_binder_transfer_binder);
 
-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 c9ff394e03e67c486f602ac15aa7455559a5d59b..834ec59ed5d44cbe7ee2ecddabcaa8683ba1c5b3 100644 (file)
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -255,29 +255,6 @@ static inline u32 task_sid_obj(const struct task_struct *task)
        return sid;
 }
 
-/*
- * get the security ID of a task for use with binder
- */
-static inline u32 task_sid_binder(const struct task_struct *task)
-{
-       /*
-        * In many case where this function is used we should be using the
-        * task's subjective SID, but we can't reliably access the subjective
-        * creds of a task other than our own so we must use the objective
-        * creds/SID, which are safe to access.  The downside is that if a task
-        * is temporarily overriding it's creds it will not be reflected here;
-        * however, it isn't clear that binder would handle that case well
-        * anyway.
-        *
-        * If this ever changes and we can safely reference the subjective
-        * creds/SID of another task, this function will make it easier to
-        * identify the various places where we make use of the task SIDs in
-        * the binder code.  It is also likely that we will need to adjust
-        * the main drivers/android binder code as well.
-        */
-       return task_sid_obj(task);
-}
-
 static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dentry);
 
 /*
@@ -2066,18 +2043,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)
 {
        return avc_has_perm(&selinux_state,
-                           current_sid(), task_sid_binder(mgr), 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_binder(from);
+       u32 fromsid = cred_sid(from);
+       u32 tosid = cred_sid(to);
        int rc;
 
        if (mysid != fromsid) {
@@ -2088,24 +2066,24 @@ static int selinux_binder_transaction(struct task_struct *from,
                        return rc;
        }
 
-       return avc_has_perm(&selinux_state, fromsid, task_sid_binder(to),
+       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)
 {
        return avc_has_perm(&selinux_state,
-                           task_sid_binder(from), task_sid_binder(to),
+                           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_binder(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;