On Mon, Oct 11, 2021 at 5:59 PM Casey Schaufler [off-list ref] wrote:

On 10/11/2021 2:33 PM, Paul Moore wrote:

quoted

On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos [off-list ref] wrote:

quoted

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 <redacted>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Reported-by: kernel test robot <redacted>
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