[PATCH v4 0/3] binder: use cred instead of task for security context

[PATCH v4 0/3] binder: use cred instead of task for security context

[Todd Kjos]

This series fixes the possible use of an incorrect security context
when checking selinux permissions, getting a security ID, or lookup
up the euid.

The previous behavior was to save the group_leader 'struct task_struct'
in binder_open() and using that to obtain security IDs or euids.

This has been shown to be unreliable, so this series instead saves the
'struct cred' of the task that called binder_open(). This cred is used
for these lookups instead of the task.

v1 and v2 of this series were a single patch "binder: use euid from"
cred instead of using task". During review, Stephen Smalley identified
two more related issues so the corresponding patches were added to
the series.

v3:
- add 2 patches to fix getsecid and euid

v4:
- fix minor checkpatch issues
- fix build-break for !CONFIG_SECURITY

Todd Kjos (3):
  binder: use cred instead of task for selinux checks
  binder: use cred instead of task for getsecid
  binder: use euid from cred instead of using task

 drivers/android/binder.c          | 14 ++++++++------
 drivers/android/binder_internal.h |  4 ++++
 include/linux/lsm_hook_defs.h     | 14 +++++++-------
 include/linux/lsm_hooks.h         | 14 +++++++-------
 include/linux/security.h          | 28 ++++++++++++++--------------
 security/security.c               | 14 +++++++-------
 security/selinux/hooks.c          | 48 +++++++++++++-----------------------------------
 7 files changed, 60 insertions(+), 76 deletions(-)

[PATCH v4 1/3] binder: use cred instead of task for selinux checks

[Todd Kjos]

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

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 saving the 'struct cred' during binder_open and pass
it to the selinux subsystem.

Fixes: 79af73079d75 ("Add security hooks to binder and implement the
hooks for SELinux.")
Suggested-by: Jann Horn <jannh@google.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Todd Kjos <tkjos@google.com>
Cc: stable@vger.kernel.org # 5.14 (need backport for earlier stables)
---
v2: updated comments as suggested by Paul Moore
v3: added 2 patches to fix related issues

 drivers/android/binder.c          | 14 +++++----
 drivers/android/binder_internal.h |  4 +++
 include/linux/lsm_hook_defs.h     | 14 ++++-----
 include/linux/lsm_hooks.h         | 14 ++++-----
 include/linux/security.h          | 28 +++++++++---------
 security/security.c               | 14 ++++-----
 security/selinux/hooks.c          | 48 +++++++++----------------------
 7 files changed, 60 insertions(+), 76 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9edacc8b9768..ca599ebdea4a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2056,7 +2056,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;
 	}
@@ -2102,7 +2102,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;
 	}
@@ -2190,7 +2190,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;
@@ -2595,8 +2595,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__;
@@ -4353,6 +4353,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);
 }
@@ -4564,7 +4565,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)) {
@@ -5055,6 +5056,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);
 	init_waitqueue_head(&proc->freeze_wait);
 	proc->default_priority = task_nice(current);
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 402c4d4362a8..d6b6b8cb7346 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -364,6 +364,9 @@ struct binder_ref {
  *                        (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
@@ -426,6 +429,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;
 	int outstanding_txns;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2adeea44c0d5..61590c1f2d33 100644
--- 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 5c4c5c0602cb..59024618554e 100644
--- 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 5b7288521300..6344d3362df7 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -258,13 +258,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,
@@ -508,25 +508,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 9ffa9e9c5c55..67264cb08fb3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -747,25 +747,25 @@ 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);
 }
 
-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 e7ebd45ca345..c8bf3db90c8b 100644
--- 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;
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 2/3] binder: use cred instead of task for getsecid

[Todd Kjos]

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.

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>
Cc: stable@vger.kernel.org # 5.4+
---
v3: added this patch to series
v4: fix build-break for !CONFIG_SECURITY

 drivers/android/binder.c | 11 +----------
 include/linux/security.h |  4 ++++
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ca599ebdea4a..989afd0804ca 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
 		u32 secid;
 		size_t added_size;
 
-		/*
-		 * Arguably this should be the task's subjective LSM secid but
-		 * we can't reliably access the subjective creds of a task
-		 * other than our own so we must use the objective creds, 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.
-		 */
-		security_task_getsecid_obj(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 6344d3362df7..f02cc0211b10 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
 {
 }
 
+static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
+{
+}
+
 static inline int security_kernel_act_as(struct cred *cred, u32 secid)
 {
 	return 0;
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 3/3] binder: use euid from cred instead of using task

[Todd Kjos]

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.

Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Signed-off-by: Todd Kjos <tkjos@google.com>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: stable@vger.kernel.org # 4.4+
---
v3: added this patch to series

 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 989afd0804ca..26382e982c5e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2711,7 +2711,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;
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Paul Moore]

On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
>
> 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.
>
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Signed-off-by: Todd Kjos <tkjos@google.com>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Cc: stable@vger.kernel.org # 4.4+
> ---
> v3: added this patch to series
>
>  drivers/android/binder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This is an interesting ordering of the patches.  Unless I'm missing
something I would have expected patch 3/3 to come first, followed by
2/3, with patch 1/3 at the end; basically the reverse of what was
posted here.

My reading of the previous thread was that Casey has made his peace
with these changes so unless anyone has any objections I'll plan on
merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
selinux/next.

> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 989afd0804ca..26382e982c5e 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2711,7 +2711,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;
> --
> 2.33.0.800.g4c38ced690-goog

--
paul moore
www.paul-moore.com

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Todd Kjos]

On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> >
> > 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.
> >
> > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > Signed-off-by: Todd Kjos <tkjos@google.com>
> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > Cc: stable@vger.kernel.org # 4.4+
> > ---
> > v3: added this patch to series
> >
> >  drivers/android/binder.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> This is an interesting ordering of the patches.  Unless I'm missing
> something I would have expected patch 3/3 to come first, followed by
> 2/3, with patch 1/3 at the end; basically the reverse of what was
> posted here.

2/3 and 3/3 both depend on 1/3 (add "cred" member of struct
binder_proc). I kept that in 1/3 to keep that patch the same as what
had already been reviewed. I didn't think much about the ordering
between 2/3 and 3/3 -- but I agree that it would have been sensible to
reverse their order.

>
> My reading of the previous thread was that Casey has made his peace
> with these changes so unless anyone has any objections I'll plan on
> merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
> selinux/next.

Thanks Paul. I'm not familiar with the branch structure, but you need
1/3 in selinux/stable-5.15 to resolve the dependency on proc->cred.

>
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 989afd0804ca..26382e982c5e 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -2711,7 +2711,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;
> > --
> > 2.33.0.800.g4c38ced690-goog
>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Casey Schaufler]

On 10/8/2021 2:12 PM, Paul Moore wrote:
> On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
>> 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.
>>
>> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
>> Signed-off-by: Todd Kjos <tkjos@google.com>
>> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>> Cc: stable@vger.kernel.org # 4.4+
>> ---
>> v3: added this patch to series
>>
>>  drivers/android/binder.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> This is an interesting ordering of the patches.  Unless I'm missing
> something I would have expected patch 3/3 to come first, followed by
> 2/3, with patch 1/3 at the end; basically the reverse of what was
> posted here.
>
> My reading of the previous thread was that Casey has made his peace
> with these changes

Yes. I will address the stacking concerns more directly.
I am still somewhat baffled by the intent of the hook, the data
passed to it, and the SELinux policy enforcement decisions, but
that's beyond my scope.

>  so unless anyone has any objections I'll plan on
> merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
> selinux/next.
>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 989afd0804ca..26382e982c5e 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2711,7 +2711,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;
>> --
>> 2.33.0.800.g4c38ced690-goog
> --
> paul moore
> www.paul-moore.com

Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid

[Paul Moore]

On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
>
> 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.
>
> 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>
> Cc: stable@vger.kernel.org # 5.4+
> ---
> v3: added this patch to series
> v4: fix build-break for !CONFIG_SECURITY
>
>  drivers/android/binder.c | 11 +----------
>  include/linux/security.h |  4 ++++
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index ca599ebdea4a..989afd0804ca 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
>                 u32 secid;
>                 size_t added_size;
>
> -               /*
> -                * Arguably this should be the task's subjective LSM secid but
> -                * we can't reliably access the subjective creds of a task
> -                * other than our own so we must use the objective creds, 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.
> -                */
> -               security_task_getsecid_obj(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 6344d3362df7..f02cc0211b10 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
>  {
>  }
>
> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +}

Since security_cred_getsecid() doesn't return an error code we should
probably set the secid to 0 in this case, for example:

  static inline void security_cred_getsecid(...)
  {
    *secid = 0;
  }

-- 
paul moore
www.paul-moore.com

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Paul Moore]

On Fri, Oct 8, 2021 at 5:25 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 10/8/2021 2:12 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> >> 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.
> >>
> >> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> >> Signed-off-by: Todd Kjos <tkjos@google.com>
> >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> >> Cc: stable@vger.kernel.org # 4.4+
> >> ---
> >> v3: added this patch to series
> >>
> >>  drivers/android/binder.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > This is an interesting ordering of the patches.  Unless I'm missing
> > something I would have expected patch 3/3 to come first, followed by
> > 2/3, with patch 1/3 at the end; basically the reverse of what was
> > posted here.
> >
> > My reading of the previous thread was that Casey has made his peace
> > with these changes
>
> Yes. I will address the stacking concerns more directly.
> I am still somewhat baffled by the intent of the hook, the data
> passed to it, and the SELinux policy enforcement decisions, but
> that's beyond my scope.

Okay, I just wanted to make sure there were no objections.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Paul Moore]

On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> > >
> > > 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.
> > >
> > > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > > Signed-off-by: Todd Kjos <tkjos@google.com>
> > > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > Cc: stable@vger.kernel.org # 4.4+
> > > ---
> > > v3: added this patch to series
> > >
> > >  drivers/android/binder.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > This is an interesting ordering of the patches.  Unless I'm missing
> > something I would have expected patch 3/3 to come first, followed by
> > 2/3, with patch 1/3 at the end; basically the reverse of what was
> > posted here.
>
> 2/3 and 3/3 both depend on 1/3 (add "cred" member of struct
> binder_proc). I kept that in 1/3 to keep that patch the same as what
> had already been reviewed. I didn't think much about the ordering
> between 2/3 and 3/3 -- but I agree that it would have been sensible to
> reverse their order.
>
> >
> > My reading of the previous thread was that Casey has made his peace
> > with these changes so unless anyone has any objections I'll plan on
> > merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
> > selinux/next.
>
> Thanks Paul. I'm not familiar with the branch structure, but you need
> 1/3 in selinux/stable-5.15 to resolve the dependency on proc->cred.

Yep, thanks.  My eyes kinda skipped over that part when looking at the
patchset but that would have fallen out as soon as I merged them.

Unfortunately that pretty much defeats the purpose of splitting this
into three patches.  While I suppose one could backport patches 2/3
and 3/3 individually, both of them have a very small footprint
especially considering their patch 1/3 dependency.  At the very least
it looks like patch 2/3 needs to be respun to address the
!CONFIG_SECURITY case and seeing the split patches now I think the
smart thing is to just combine them into a single patch.  I apologize
for the bad recommendation earlier, I should have followed that thread
a bit closer after the discussion with Casey and Stephen.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid

[Casey Schaufler]

On 10/11/2021 2:33 PM, Paul Moore wrote:
> On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
>> 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.
>>
>> 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>
>> Cc: stable@vger.kernel.org # 5.4+
>> ---
>> v3: added this patch to series
>> v4: fix build-break for !CONFIG_SECURITY
>>
>>  drivers/android/binder.c | 11 +----------
>>  include/linux/security.h |  4 ++++
>>  2 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index ca599ebdea4a..989afd0804ca 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
>>                 u32 secid;
>>                 size_t added_size;
>>
>> -               /*
>> -                * Arguably this should be the task's subjective LSM secid but
>> -                * we can't reliably access the subjective creds of a task
>> -                * other than our own so we must use the objective creds, 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.
>> -                */
>> -               security_task_getsecid_obj(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 6344d3362df7..f02cc0211b10 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
>>  {
>>  }
>>
>> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
>> +{
>> +}
> Since security_cred_getsecid() doesn't return an error code we should
> probably set the secid to 0 in this case, for example:
>
>   static inline void security_cred_getsecid(...)
>   {
>     *secid = 0;
>   }

If CONFIG_SECURITY is unset there shouldn't be any case where
the secid value is ever used for anything. Are you suggesting that
it be set out of an abundance of caution?

Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid

[Paul Moore]

On Mon, Oct 11, 2021 at 5:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/11/2021 2:33 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> >> 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.
> >>
> >> 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>
> >> Cc: stable@vger.kernel.org # 5.4+
> >> ---
> >> v3: added this patch to series
> >> v4: fix build-break for !CONFIG_SECURITY
> >>
> >>  drivers/android/binder.c | 11 +----------
> >>  include/linux/security.h |  4 ++++
> >>  2 files changed, 5 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >> index ca599ebdea4a..989afd0804ca 100644
> >> --- a/drivers/android/binder.c
> >> +++ b/drivers/android/binder.c
> >> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
> >>                 u32 secid;
> >>                 size_t added_size;
> >>
> >> -               /*
> >> -                * Arguably this should be the task's subjective LSM secid but
> >> -                * we can't reliably access the subjective creds of a task
> >> -                * other than our own so we must use the objective creds, 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.
> >> -                */
> >> -               security_task_getsecid_obj(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 6344d3362df7..f02cc0211b10 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
> >>  {
> >>  }
> >>
> >> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> >> +{
> >> +}
> >
> > Since security_cred_getsecid() doesn't return an error code we should
> > probably set the secid to 0 in this case, for example:
> >
> >   static inline void security_cred_getsecid(...)
> >   {
> >     *secid = 0;
> >   }
>
> If CONFIG_SECURITY is unset there shouldn't be any case where
> the secid value is ever used for anything. Are you suggesting that
> it be set out of an abundance of caution?

It follows a pattern with the other LSM hooks when !CONFIG_SECURITY,
and I'd much rather us keep things consistent.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Todd Kjos]

On Mon, Oct 11, 2021 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote:
> >
> > On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> > > >
> > > > 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.
> > > >
> > > > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > > > Signed-off-by: Todd Kjos <tkjos@google.com>
> > > > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > > Cc: stable@vger.kernel.org # 4.4+
> > > > ---
> > > > v3: added this patch to series
> > > >
> > > >  drivers/android/binder.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > This is an interesting ordering of the patches.  Unless I'm missing
> > > something I would have expected patch 3/3 to come first, followed by
> > > 2/3, with patch 1/3 at the end; basically the reverse of what was
> > > posted here.
> >
> > 2/3 and 3/3 both depend on 1/3 (add "cred" member of struct
> > binder_proc). I kept that in 1/3 to keep that patch the same as what
> > had already been reviewed. I didn't think much about the ordering
> > between 2/3 and 3/3 -- but I agree that it would have been sensible to
> > reverse their order.
> >
> > >
> > > My reading of the previous thread was that Casey has made his peace
> > > with these changes so unless anyone has any objections I'll plan on
> > > merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
> > > selinux/next.
> >
> > Thanks Paul. I'm not familiar with the branch structure, but you need
> > 1/3 in selinux/stable-5.15 to resolve the dependency on proc->cred.
>
> Yep, thanks.  My eyes kinda skipped over that part when looking at the
> patchset but that would have fallen out as soon as I merged them.
>
> Unfortunately that pretty much defeats the purpose of splitting this
> into three patches.  While I suppose one could backport patches 2/3
> and 3/3 individually, both of them have a very small footprint
> especially considering their patch 1/3 dependency.  At the very least
> it looks like patch 2/3 needs to be respun to address the
> !CONFIG_SECURITY case and seeing the split patches now I think the
> smart thing is to just combine them into a single patch.  I apologize
> for the bad recommendation earlier, I should have followed that thread
> a bit closer after the discussion with Casey and Stephen.

I'm happy to submit a single patch for all of this. Another part of
the rationale
for splitting it into 3 patches was correctly identify the patch that introduced
the patch that introduced the issue -- so each of the 3 had a different
"Fixes:" tag. Should I cite the oldest (binder introduction) with the "Fixes"
tag and perhaps mention the other two in the commit message?

-Todd

>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid

[Dan Carpenter]

On Mon, Oct 11, 2021 at 02:59:13PM -0700, Casey Schaufler wrote:
> On 10/11/2021 2:33 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> >> 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.
> >>
> >> 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>
> >> Cc: stable@vger.kernel.org # 5.4+
> >> ---
> >> v3: added this patch to series
> >> v4: fix build-break for !CONFIG_SECURITY
> >>
> >>  drivers/android/binder.c | 11 +----------
> >>  include/linux/security.h |  4 ++++
> >>  2 files changed, 5 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >> index ca599ebdea4a..989afd0804ca 100644
> >> --- a/drivers/android/binder.c
> >> +++ b/drivers/android/binder.c
> >> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
> >>                 u32 secid;
> >>                 size_t added_size;
> >>
> >> -               /*
> >> -                * Arguably this should be the task's subjective LSM secid but
> >> -                * we can't reliably access the subjective creds of a task
> >> -                * other than our own so we must use the objective creds, 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.
> >> -                */
> >> -               security_task_getsecid_obj(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 6344d3362df7..f02cc0211b10 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
> >>  {
> >>  }
> >>
> >> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> >> +{
> >> +}
> > Since security_cred_getsecid() doesn't return an error code we should
> > probably set the secid to 0 in this case, for example:
> >
> >   static inline void security_cred_getsecid(...)
> >   {
> >     *secid = 0;
> >   }
> 
> If CONFIG_SECURITY is unset there shouldn't be any case where
> the secid value is ever used for anything. Are you suggesting that
> it be set out of an abundance of caution?

The security_secid_to_secctx() function is probably inlined so probably
KMSan will not warn about this.  But Smatch will warn about passing
unitialized variables.  You probably wouldn't recieve and email about
it, and I would just add an exception that security_cred_getsecid()
should be ignored.

regards,
dan carpenter

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Stephen Smalley]

On Mon, Oct 11, 2021 at 7:39 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Mon, Oct 11, 2021 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote:
> > >
> > > On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote:
> > > >
> > > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> > > > >
> > > > > 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.
> > > > >
> > > > > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > > > > Signed-off-by: Todd Kjos <tkjos@google.com>
> > > > > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > > > Cc: stable@vger.kernel.org # 4.4+
> > > > > ---
> > > > > v3: added this patch to series
> > > > >
> > > > >  drivers/android/binder.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > This is an interesting ordering of the patches.  Unless I'm missing
> > > > something I would have expected patch 3/3 to come first, followed by
> > > > 2/3, with patch 1/3 at the end; basically the reverse of what was
> > > > posted here.
> > >
> > > 2/3 and 3/3 both depend on 1/3 (add "cred" member of struct
> > > binder_proc). I kept that in 1/3 to keep that patch the same as what
> > > had already been reviewed. I didn't think much about the ordering
> > > between 2/3 and 3/3 -- but I agree that it would have been sensible to
> > > reverse their order.
> > >
> > > >
> > > > My reading of the previous thread was that Casey has made his peace
> > > > with these changes so unless anyone has any objections I'll plan on
> > > > merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
> > > > selinux/next.
> > >
> > > Thanks Paul. I'm not familiar with the branch structure, but you need
> > > 1/3 in selinux/stable-5.15 to resolve the dependency on proc->cred.
> >
> > Yep, thanks.  My eyes kinda skipped over that part when looking at the
> > patchset but that would have fallen out as soon as I merged them.
> >
> > Unfortunately that pretty much defeats the purpose of splitting this
> > into three patches.  While I suppose one could backport patches 2/3
> > and 3/3 individually, both of them have a very small footprint
> > especially considering their patch 1/3 dependency.  At the very least
> > it looks like patch 2/3 needs to be respun to address the
> > !CONFIG_SECURITY case and seeing the split patches now I think the
> > smart thing is to just combine them into a single patch.  I apologize
> > for the bad recommendation earlier, I should have followed that thread
> > a bit closer after the discussion with Casey and Stephen.
>
> I'm happy to submit a single patch for all of this. Another part of
> the rationale
> for splitting it into 3 patches was correctly identify the patch that introduced
> the patch that introduced the issue -- so each of the 3 had a different
> "Fixes:" tag. Should I cite the oldest (binder introduction) with the "Fixes"
> tag and perhaps mention the other two in the commit message?

Couldn't you just split patch 1 into the "add cred to binder proc"
part and "use cred in LSM/SELinux hooks" part, combine patch 3 with
the "add cred to binder proc" part to create new patch 1, then "use
cred in LSM/SELinux hooks" part is patch 2, and "switch task_getsecid
to cred_getsecid" to patch 3? Then patch 1 can be cherry-picked/ported
all the way back to the introduction of binder, patch 2 all the way
back to the introduction of binder LSM/SELinux hooks, and patch 3 just
back to where passing the secctx across binder was introduced.

Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid

[Paul Moore]

On Tue, Oct 12, 2021 at 5:41 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Oct 11, 2021 at 02:59:13PM -0700, Casey Schaufler wrote:
> > On 10/11/2021 2:33 PM, Paul Moore wrote:
> > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> > >> 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.
> > >>
> > >> 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>
> > >> Cc: stable@vger.kernel.org # 5.4+
> > >> ---
> > >> v3: added this patch to series
> > >> v4: fix build-break for !CONFIG_SECURITY
> > >>
> > >>  drivers/android/binder.c | 11 +----------
> > >>  include/linux/security.h |  4 ++++
> > >>  2 files changed, 5 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > >> index ca599ebdea4a..989afd0804ca 100644
> > >> --- a/drivers/android/binder.c
> > >> +++ b/drivers/android/binder.c
> > >> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
> > >>                 u32 secid;
> > >>                 size_t added_size;
> > >>
> > >> -               /*
> > >> -                * Arguably this should be the task's subjective LSM secid but
> > >> -                * we can't reliably access the subjective creds of a task
> > >> -                * other than our own so we must use the objective creds, 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.
> > >> -                */
> > >> -               security_task_getsecid_obj(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 6344d3362df7..f02cc0211b10 100644
> > >> --- a/include/linux/security.h
> > >> +++ b/include/linux/security.h
> > >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
> > >>  {
> > >>  }
> > >>
> > >> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> > >> +{
> > >> +}
> > > Since security_cred_getsecid() doesn't return an error code we should
> > > probably set the secid to 0 in this case, for example:
> > >
> > >   static inline void security_cred_getsecid(...)
> > >   {
> > >     *secid = 0;
> > >   }
> >
> > If CONFIG_SECURITY is unset there shouldn't be any case where
> > the secid value is ever used for anything. Are you suggesting that
> > it be set out of an abundance of caution?
>
> The security_secid_to_secctx() function is probably inlined so probably
> KMSan will not warn about this.  But Smatch will warn about passing
> unitialized variables.  You probably wouldn't recieve and email about
> it, and I would just add an exception that security_cred_getsecid()
> should be ignored.

I'd much rather just see the secid set to zero in the !CONFIG_SECURITY case.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v4 3/3] binder: use euid from cred instead of using task

[Todd Kjos]

On Tue, Oct 12, 2021 at 5:24 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Oct 11, 2021 at 7:39 PM Todd Kjos <tkjos@google.com> wrote:
> >
> > On Mon, Oct 11, 2021 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote:
> > > >
> > > > On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > >
> > > > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> wrote:
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > > > > > Signed-off-by: Todd Kjos <tkjos@google.com>
> > > > > > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > > > > Cc: stable@vger.kernel.org # 4.4+
> > > > > > ---
> > > > > > v3: added this patch to series
> > > > > >
> > > > > >  drivers/android/binder.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > This is an interesting ordering of the patches.  Unless I'm missing
> > > > > something I would have expected patch 3/3 to come first, followed by
> > > > > 2/3, with patch 1/3 at the end; basically the reverse of what was
> > > > > posted here.
> > > >
> > > > 2/3 and 3/3 both depend on 1/3 (add "cred" member of struct
> > > > binder_proc). I kept that in 1/3 to keep that patch the same as what
> > > > had already been reviewed. I didn't think much about the ordering
> > > > between 2/3 and 3/3 -- but I agree that it would have been sensible to
> > > > reverse their order.
> > > >
> > > > >
> > > > > My reading of the previous thread was that Casey has made his peace
> > > > > with these changes so unless anyone has any objections I'll plan on
> > > > > merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
> > > > > selinux/next.
> > > >
> > > > Thanks Paul. I'm not familiar with the branch structure, but you need
> > > > 1/3 in selinux/stable-5.15 to resolve the dependency on proc->cred.
> > >
> > > Yep, thanks.  My eyes kinda skipped over that part when looking at the
> > > patchset but that would have fallen out as soon as I merged them.
> > >
> > > Unfortunately that pretty much defeats the purpose of splitting this
> > > into three patches.  While I suppose one could backport patches 2/3
> > > and 3/3 individually, both of them have a very small footprint
> > > especially considering their patch 1/3 dependency.  At the very least
> > > it looks like patch 2/3 needs to be respun to address the
> > > !CONFIG_SECURITY case and seeing the split patches now I think the
> > > smart thing is to just combine them into a single patch.  I apologize
> > > for the bad recommendation earlier, I should have followed that thread
> > > a bit closer after the discussion with Casey and Stephen.
> >
> > I'm happy to submit a single patch for all of this. Another part of
> > the rationale
> > for splitting it into 3 patches was correctly identify the patch that introduced
> > the patch that introduced the issue -- so each of the 3 had a different
> > "Fixes:" tag. Should I cite the oldest (binder introduction) with the "Fixes"
> > tag and perhaps mention the other two in the commit message?
>
> Couldn't you just split patch 1 into the "add cred to binder proc"
> part and "use cred in LSM/SELinux hooks" part, combine patch 3 with
> the "add cred to binder proc" part to create new patch 1, then "use
> cred in LSM/SELinux hooks" part is patch 2, and "switch task_getsecid
> to cred_getsecid" to patch 3? Then patch 1 can be cherry-picked/ported
> all the way back to the introduction of binder, patch 2 all the way
> back to the introduction of binder LSM/SELinux hooks, and patch 3 just
> back to where passing the secctx across binder was introduced.

Sending a v5 with this refactoring and the !CONFIG_SECURITY fix