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;
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; }
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?
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.
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
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.
linux-stable-mirror@lists.linaro.org
-
Casey Schaufler -
Dan Carpenter -
Paul Moore -
Todd Kjos