* Re: [PATCH v12 09/25] LSM: Use lsmblob in security_task_getsecid
From: Casey Schaufler @ 2019-12-17 18:26 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <cb38eba1-1fb1-13df-e396-ee620794c375@tycho.nsa.gov>
On 12/17/2019 10:11 AM, Stephen Smalley wrote:
> On 12/16/19 5:36 PM, Casey Schaufler wrote:
>> Change the security_task_getsecid() interface to fill in
>> a lsmblob structure instead of a u32 secid in support of
>> LSM stacking. Audit interfaces will need to collect all
>> possible secids for possible reporting.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: John Johansen <john.johansen@canonical.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> cc: linux-integrity@vger.kernel.org
>> ---
>> drivers/android/binder.c | 4 +--
>> include/linux/security.h | 7 +++--
>> kernel/audit.c | 11 +++----
>> kernel/auditfilter.c | 4 +--
>> kernel/auditsc.c | 18 ++++++++----
>> net/netlabel/netlabel_unlabeled.c | 5 +++-
>> net/netlabel/netlabel_user.h | 6 +++-
>> security/integrity/ima/ima_appraise.c | 4 ++-
>> security/integrity/ima/ima_main.c | 42 +++++++++++++++------------
>> security/security.c | 12 ++++++--
>> 10 files changed, 69 insertions(+), 44 deletions(-)
>>
>
>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> index 300c8d2943c5..69e549164949 100644
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -49,11 +49,13 @@ bool is_ima_appraise_enabled(void)
>> int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
>> {
>> u32 secid;
>> + struct lsmblob blob;
>> if (!ima_appraise)
>> return 0;
>> - security_task_getsecid(current, &secid);
>> + security_task_getsecid(current, &blob);
>> + lsmblob_secid(&blob, &secid);
>> return ima_match_policy(inode, current_cred(), secid, func, mask,
>> IMA_APPRAISE | IMA_HASH, NULL, NULL);
>> }
>
> I missed where lsmblob_secid() is defined? Looks like it is later deleted by patch 12/25. Leftover from an earlier version of the series? Have you checked that it compiles after each patch?
Bugger. Yes, this is a straight up botch. lsmblb_secid() is never defined in
this version.
>
>
^ permalink raw reply
* Re: [PATCH v12 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Casey Schaufler @ 2019-12-17 22:01 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux, keescook
Cc: john.johansen, penguin-kernel, paul
In-Reply-To: <5d2d0621-5156-28af-7c08-0f9daac6ea6e@tycho.nsa.gov>
On 12/17/2019 9:34 AM, Stephen Smalley wrote:
> On 12/16/19 5:35 PM, Casey Schaufler wrote:
>> Change the secid parameter of security_audit_rule_match
>> to a lsmblob structure pointer. Pass the entry from the
>> lsmblob structure for the approprite slot to the LSM hook.
>>
>> Change the users of security_audit_rule_match to use the
>> lsmblob instead of a u32. In some cases this requires a
>> temporary conversion using lsmblob_init() that will go
>> away when other interfaces get converted.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: John Johansen <john.johansen@canonical.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> include/linux/security.h | 7 ++++---
>> kernel/auditfilter.c | 7 +++++--
>> kernel/auditsc.c | 14 ++++++++++----
>> security/integrity/ima/ima.h | 4 ++--
>> security/integrity/ima/ima_policy.c | 7 +++++--
>> security/security.c | 18 +++++++++++++++---
>> 6 files changed, 41 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index b74dc70088ca..9c6dbe248eaf 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1837,7 +1837,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
>> #ifdef CONFIG_SECURITY
>> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
>> int security_audit_rule_known(struct audit_krule *krule);
>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
>> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
>> + void *lsmrule);
>> void security_audit_rule_free(void *lsmrule);
>> #else
>> @@ -1853,8 +1854,8 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
>> return 0;
>> }
>> -static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
>> - void *lsmrule)
>> +static inline int security_audit_rule_match(struct lsmblob *blob, u32 field,
>> + u32 op, void *lsmrule)
>> {
>> return 0;
>> }
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index b0126e9c0743..356db1dd276c 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -1325,6 +1325,7 @@ int audit_filter(int msgtype, unsigned int listtype)
>> struct audit_field *f = &e->rule.fields[i];
>> pid_t pid;
>> u32 sid;
>> + struct lsmblob blob;
>> switch (f->type) {
>> case AUDIT_PID:
>> @@ -1355,8 +1356,10 @@ int audit_filter(int msgtype, unsigned int listtype)
>> case AUDIT_SUBJ_CLR:
>> if (f->lsm_rule) {
>> security_task_getsecid(current, &sid);
>> - result = security_audit_rule_match(sid,
>> - f->type, f->op, f->lsm_rule);
>> + lsmblob_init(&blob, sid);
>> + result = security_audit_rule_match(
>> + &blob, f->type,
>> + f->op, f->lsm_rule);
>> }
>> break;
>> case AUDIT_EXE:
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 4effe01ebbe2..7566e5b1c419 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -445,6 +445,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>> const struct cred *cred;
>> int i, need_sid = 1;
>> u32 sid;
>> + struct lsmblob blob;
>> unsigned int sessionid;
>> cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
>> @@ -643,7 +644,9 @@ static int audit_filter_rules(struct task_struct *tsk,
>> security_task_getsecid(tsk, &sid);
>> need_sid = 0;
>> }
>> - result = security_audit_rule_match(sid, f->type,
>> + lsmblob_init(&blob, sid);
>> + result = security_audit_rule_match(&blob,
>> + f->type,
>> f->op,
>> f->lsm_rule);
>> }
>> @@ -658,15 +661,17 @@ static int audit_filter_rules(struct task_struct *tsk,
>> if (f->lsm_rule) {
>> /* Find files that match */
>> if (name) {
>> + lsmblob_init(&blob, name->osid);
>> result = security_audit_rule_match(
>> - name->osid,
>> + &blob,
>> f->type,
>> f->op,
>> f->lsm_rule);
>> } else if (ctx) {
>> list_for_each_entry(n, &ctx->names_list, list) {
>> + lsmblob_init(&blob, n->osid);
>> if (security_audit_rule_match(
>> - n->osid,
>> + &blob,
>> f->type,
>> f->op,
>> f->lsm_rule)) {
>> @@ -678,7 +683,8 @@ static int audit_filter_rules(struct task_struct *tsk,
>> /* Find ipc objects that match */
>> if (!ctx || ctx->type != AUDIT_IPC)
>> break;
>> - if (security_audit_rule_match(ctx->ipc.osid,
>> + lsmblob_init(&blob, ctx->ipc.osid);
>> + if (security_audit_rule_match(&blob,
>> f->type, f->op,
>> f->lsm_rule))
>> ++result;
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index df4ca482fb53..d95b0ece7434 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -381,8 +381,8 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
>> return -EINVAL;
>> }
>> -static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
>> - void *lsmrule)
>> +static inline int security_filter_rule_match(struct lsmblob *blob, u32 field,
>> + u32 op, void *lsmrule)
>> {
>> return -EINVAL;
>> }
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index f19a895ad7cd..193ddd55420b 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -414,6 +414,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>> for (i = 0; i < MAX_LSM_RULES; i++) {
>> int rc = 0;
>> u32 osid;
>> + struct lsmblob blob;
>> if (!rule->lsm[i].rule)
>> continue;
>> @@ -423,7 +424,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>> case LSM_OBJ_ROLE:
>> case LSM_OBJ_TYPE:
>> security_inode_getsecid(inode, &osid);
>> - rc = security_filter_rule_match(osid,
>> + lsmblob_init(&blob, osid);
>> + rc = security_filter_rule_match(&blob,
>> rule->lsm[i].type,
>> Audit_equal,
>> rule->lsm[i].rule);
>> @@ -431,7 +433,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>> case LSM_SUBJ_USER:
>> case LSM_SUBJ_ROLE:
>> case LSM_SUBJ_TYPE:
>> - rc = security_filter_rule_match(secid,
>> + lsmblob_init(&blob, secid);
>> + rc = security_filter_rule_match(&blob,
>> rule->lsm[i].type,
>> Audit_equal,
>> rule->lsm[i].rule);
>> diff --git a/security/security.c b/security/security.c
>> index a89634af639a..bfea9739c084 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -439,7 +439,7 @@ static int lsm_append(const char *new, char **result)
>> /*
>> * Current index to use while initializing the lsmblob secid list.
>> */
>> -static int lsm_slot __initdata;
>> +static int lsm_slot __lsm_ro_after_init;
>> /**
>> * security_add_hooks - Add a modules hooks to the hook lists.
>> @@ -2412,9 +2412,21 @@ void security_audit_rule_free(void *lsmrule)
>> call_void_hook(audit_rule_free, lsmrule);
>> }
>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
>> + void *lsmrule)
>> {
>> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>> + struct security_hook_list *hp;
>> + int rc;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>> + continue;
>
> Do you think we really need to retain these WARN_ON()s?
Kees was especially keen on having the WARN_ON().
I'd be fine with removing it.
> If not, then you could dispense with it now and leave lsm_slot as __initdata? Otherwise,
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>
>> + rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
>> + field, op, lsmrule);
>> + if (rc != 0)
>> + return rc;
>> + }
>> + return 0;
>> }
>> #endif /* CONFIG_AUDIT */
>>
>
^ permalink raw reply
* Re: [PATCH] integrity: Expose data structures required for include/linux/integrity.h
From: Mimi Zohar @ 2019-12-17 23:08 UTC (permalink / raw)
To: Casey Schaufler, Florent Revest, linux-integrity
Cc: jmorris, serge, revest, allison, armijn, bauerman, linux-kernel,
linux-security-module
In-Reply-To: <e9e366d3-6c5d-743b-ffde-6b95b85884a2@schaufler-ca.com>
On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote:
> On 12/17/2019 5:47 AM, Florent Revest wrote:
> > From: Florent Revest <revest@google.com>
> >
> > include/linux/integrity.h exposes the prototype of integrity_inode_get().
> > However, it relies on struct integrity_iint_cache which is currently
> > defined in an internal header, security/integrity/integrity.h.
> >
> > To allow the rest of the kernel to use integrity_inode_get,
>
> Why do you want to do this?
ditto
>
> > this patch
> > moves the definition of the necessary structures from a private header
> > to a global kernel header.
> >
^ permalink raw reply
* Re: [PATCH v12 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Kees Cook @ 2019-12-17 23:47 UTC (permalink / raw)
To: Casey Schaufler
Cc: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux, john.johansen, penguin-kernel, paul
In-Reply-To: <5dca060d-da34-3460-ecf2-54d4a31266c4@schaufler-ca.com>
On Tue, Dec 17, 2019 at 02:01:19PM -0800, Casey Schaufler wrote:
> On 12/17/2019 9:34 AM, Stephen Smalley wrote:
> > On 12/16/19 5:35 PM, Casey Schaufler wrote:
> >> Change the secid parameter of security_audit_rule_match
> >> to a lsmblob structure pointer. Pass the entry from the
> >> lsmblob structure for the approprite slot to the LSM hook.
> >>
> >> Change the users of security_audit_rule_match to use the
> >> lsmblob instead of a u32. In some cases this requires a
> >> temporary conversion using lsmblob_init() that will go
> >> away when other interfaces get converted.
> >>
> >> Reviewed-by: Kees Cook <keescook@chromium.org>
> >> Reviewed-by: John Johansen <john.johansen@canonical.com>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> ---
> >> include/linux/security.h | 7 ++++---
> >> kernel/auditfilter.c | 7 +++++--
> >> kernel/auditsc.c | 14 ++++++++++----
> >> security/integrity/ima/ima.h | 4 ++--
> >> security/integrity/ima/ima_policy.c | 7 +++++--
> >> security/security.c | 18 +++++++++++++++---
> >> 6 files changed, 41 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/include/linux/security.h b/include/linux/security.h
> >> index b74dc70088ca..9c6dbe248eaf 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -1837,7 +1837,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
> >> #ifdef CONFIG_SECURITY
> >> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> >> int security_audit_rule_known(struct audit_krule *krule);
> >> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> >> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
> >> + void *lsmrule);
> >> void security_audit_rule_free(void *lsmrule);
> >> #else
> >> @@ -1853,8 +1854,8 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
> >> return 0;
> >> }
> >> -static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
> >> - void *lsmrule)
> >> +static inline int security_audit_rule_match(struct lsmblob *blob, u32 field,
> >> + u32 op, void *lsmrule)
> >> {
> >> return 0;
> >> }
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index b0126e9c0743..356db1dd276c 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -1325,6 +1325,7 @@ int audit_filter(int msgtype, unsigned int listtype)
> >> struct audit_field *f = &e->rule.fields[i];
> >> pid_t pid;
> >> u32 sid;
> >> + struct lsmblob blob;
> >> switch (f->type) {
> >> case AUDIT_PID:
> >> @@ -1355,8 +1356,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> >> case AUDIT_SUBJ_CLR:
> >> if (f->lsm_rule) {
> >> security_task_getsecid(current, &sid);
> >> - result = security_audit_rule_match(sid,
> >> - f->type, f->op, f->lsm_rule);
> >> + lsmblob_init(&blob, sid);
> >> + result = security_audit_rule_match(
> >> + &blob, f->type,
> >> + f->op, f->lsm_rule);
> >> }
> >> break;
> >> case AUDIT_EXE:
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index 4effe01ebbe2..7566e5b1c419 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -445,6 +445,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> >> const struct cred *cred;
> >> int i, need_sid = 1;
> >> u32 sid;
> >> + struct lsmblob blob;
> >> unsigned int sessionid;
> >> cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
> >> @@ -643,7 +644,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> >> security_task_getsecid(tsk, &sid);
> >> need_sid = 0;
> >> }
> >> - result = security_audit_rule_match(sid, f->type,
> >> + lsmblob_init(&blob, sid);
> >> + result = security_audit_rule_match(&blob,
> >> + f->type,
> >> f->op,
> >> f->lsm_rule);
> >> }
> >> @@ -658,15 +661,17 @@ static int audit_filter_rules(struct task_struct *tsk,
> >> if (f->lsm_rule) {
> >> /* Find files that match */
> >> if (name) {
> >> + lsmblob_init(&blob, name->osid);
> >> result = security_audit_rule_match(
> >> - name->osid,
> >> + &blob,
> >> f->type,
> >> f->op,
> >> f->lsm_rule);
> >> } else if (ctx) {
> >> list_for_each_entry(n, &ctx->names_list, list) {
> >> + lsmblob_init(&blob, n->osid);
> >> if (security_audit_rule_match(
> >> - n->osid,
> >> + &blob,
> >> f->type,
> >> f->op,
> >> f->lsm_rule)) {
> >> @@ -678,7 +683,8 @@ static int audit_filter_rules(struct task_struct *tsk,
> >> /* Find ipc objects that match */
> >> if (!ctx || ctx->type != AUDIT_IPC)
> >> break;
> >> - if (security_audit_rule_match(ctx->ipc.osid,
> >> + lsmblob_init(&blob, ctx->ipc.osid);
> >> + if (security_audit_rule_match(&blob,
> >> f->type, f->op,
> >> f->lsm_rule))
> >> ++result;
> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >> index df4ca482fb53..d95b0ece7434 100644
> >> --- a/security/integrity/ima/ima.h
> >> +++ b/security/integrity/ima/ima.h
> >> @@ -381,8 +381,8 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
> >> return -EINVAL;
> >> }
> >> -static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> >> - void *lsmrule)
> >> +static inline int security_filter_rule_match(struct lsmblob *blob, u32 field,
> >> + u32 op, void *lsmrule)
> >> {
> >> return -EINVAL;
> >> }
> >> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> >> index f19a895ad7cd..193ddd55420b 100644
> >> --- a/security/integrity/ima/ima_policy.c
> >> +++ b/security/integrity/ima/ima_policy.c
> >> @@ -414,6 +414,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> >> for (i = 0; i < MAX_LSM_RULES; i++) {
> >> int rc = 0;
> >> u32 osid;
> >> + struct lsmblob blob;
> >> if (!rule->lsm[i].rule)
> >> continue;
> >> @@ -423,7 +424,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> >> case LSM_OBJ_ROLE:
> >> case LSM_OBJ_TYPE:
> >> security_inode_getsecid(inode, &osid);
> >> - rc = security_filter_rule_match(osid,
> >> + lsmblob_init(&blob, osid);
> >> + rc = security_filter_rule_match(&blob,
> >> rule->lsm[i].type,
> >> Audit_equal,
> >> rule->lsm[i].rule);
> >> @@ -431,7 +433,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> >> case LSM_SUBJ_USER:
> >> case LSM_SUBJ_ROLE:
> >> case LSM_SUBJ_TYPE:
> >> - rc = security_filter_rule_match(secid,
> >> + lsmblob_init(&blob, secid);
> >> + rc = security_filter_rule_match(&blob,
> >> rule->lsm[i].type,
> >> Audit_equal,
> >> rule->lsm[i].rule);
> >> diff --git a/security/security.c b/security/security.c
> >> index a89634af639a..bfea9739c084 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -439,7 +439,7 @@ static int lsm_append(const char *new, char **result)
> >> /*
> >> * Current index to use while initializing the lsmblob secid list.
> >> */
> >> -static int lsm_slot __initdata;
> >> +static int lsm_slot __lsm_ro_after_init;
> >> /**
> >> * security_add_hooks - Add a modules hooks to the hook lists.
> >> @@ -2412,9 +2412,21 @@ void security_audit_rule_free(void *lsmrule)
> >> call_void_hook(audit_rule_free, lsmrule);
> >> }
> >> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> >> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
> >> + void *lsmrule)
> >> {
> >> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> >> + struct security_hook_list *hp;
> >> + int rc;
> >> +
> >> + hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
> >> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> >> + continue;
> >
> > Do you think we really need to retain these WARN_ON()s?
>
> Kees was especially keen on having the WARN_ON().
> I'd be fine with removing it.
It should really really never happen, so I like the WARN_ON staying.
-Kees
>
>
> > If not, then you could dispense with it now and leave lsm_slot as __initdata? Otherwise,
> > Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> >
> >> + rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
> >> + field, op, lsmrule);
> >> + if (rc != 0)
> >> + return rc;
> >> + }
> >> + return 0;
> >> }
> >> #endif /* CONFIG_AUDIT */
> >>
> >
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v12 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Casey Schaufler @ 2019-12-18 0:28 UTC (permalink / raw)
To: Kees Cook
Cc: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <201912171547.7B4FED2@keescook>
On 12/17/2019 3:47 PM, Kees Cook wrote:
> On Tue, Dec 17, 2019 at 02:01:19PM -0800, Casey Schaufler wrote:
>> On 12/17/2019 9:34 AM, Stephen Smalley wrote:
>>> On 12/16/19 5:35 PM, Casey Schaufler wrote:
>>>> Change the secid parameter of security_audit_rule_match
>>>> to a lsmblob structure pointer. Pass the entry from the
>>>> lsmblob structure for the approprite slot to the LSM hook.
>>>>
>>>> Change the users of security_audit_rule_match to use the
>>>> lsmblob instead of a u32. In some cases this requires a
>>>> temporary conversion using lsmblob_init() that will go
>>>> away when other interfaces get converted.
>>>>
>>>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>>> Reviewed-by: John Johansen <john.johansen@canonical.com>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> ---
>>>> include/linux/security.h | 7 ++++---
>>>> kernel/auditfilter.c | 7 +++++--
>>>> kernel/auditsc.c | 14 ++++++++++----
>>>> security/integrity/ima/ima.h | 4 ++--
>>>> security/integrity/ima/ima_policy.c | 7 +++++--
>>>> security/security.c | 18 +++++++++++++++---
>>>> 6 files changed, 41 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>>> index b74dc70088ca..9c6dbe248eaf 100644
>>>> --- a/include/linux/security.h
>>>> +++ b/include/linux/security.h
>>>> @@ -1837,7 +1837,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
>>>> #ifdef CONFIG_SECURITY
>>>> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
>>>> int security_audit_rule_known(struct audit_krule *krule);
>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
>>>> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
>>>> + void *lsmrule);
>>>> void security_audit_rule_free(void *lsmrule);
>>>> #else
>>>> @@ -1853,8 +1854,8 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
>>>> return 0;
>>>> }
>>>> -static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
>>>> - void *lsmrule)
>>>> +static inline int security_audit_rule_match(struct lsmblob *blob, u32 field,
>>>> + u32 op, void *lsmrule)
>>>> {
>>>> return 0;
>>>> }
>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>>> index b0126e9c0743..356db1dd276c 100644
>>>> --- a/kernel/auditfilter.c
>>>> +++ b/kernel/auditfilter.c
>>>> @@ -1325,6 +1325,7 @@ int audit_filter(int msgtype, unsigned int listtype)
>>>> struct audit_field *f = &e->rule.fields[i];
>>>> pid_t pid;
>>>> u32 sid;
>>>> + struct lsmblob blob;
>>>> switch (f->type) {
>>>> case AUDIT_PID:
>>>> @@ -1355,8 +1356,10 @@ int audit_filter(int msgtype, unsigned int listtype)
>>>> case AUDIT_SUBJ_CLR:
>>>> if (f->lsm_rule) {
>>>> security_task_getsecid(current, &sid);
>>>> - result = security_audit_rule_match(sid,
>>>> - f->type, f->op, f->lsm_rule);
>>>> + lsmblob_init(&blob, sid);
>>>> + result = security_audit_rule_match(
>>>> + &blob, f->type,
>>>> + f->op, f->lsm_rule);
>>>> }
>>>> break;
>>>> case AUDIT_EXE:
>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>> index 4effe01ebbe2..7566e5b1c419 100644
>>>> --- a/kernel/auditsc.c
>>>> +++ b/kernel/auditsc.c
>>>> @@ -445,6 +445,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>> const struct cred *cred;
>>>> int i, need_sid = 1;
>>>> u32 sid;
>>>> + struct lsmblob blob;
>>>> unsigned int sessionid;
>>>> cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
>>>> @@ -643,7 +644,9 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>> security_task_getsecid(tsk, &sid);
>>>> need_sid = 0;
>>>> }
>>>> - result = security_audit_rule_match(sid, f->type,
>>>> + lsmblob_init(&blob, sid);
>>>> + result = security_audit_rule_match(&blob,
>>>> + f->type,
>>>> f->op,
>>>> f->lsm_rule);
>>>> }
>>>> @@ -658,15 +661,17 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>> if (f->lsm_rule) {
>>>> /* Find files that match */
>>>> if (name) {
>>>> + lsmblob_init(&blob, name->osid);
>>>> result = security_audit_rule_match(
>>>> - name->osid,
>>>> + &blob,
>>>> f->type,
>>>> f->op,
>>>> f->lsm_rule);
>>>> } else if (ctx) {
>>>> list_for_each_entry(n, &ctx->names_list, list) {
>>>> + lsmblob_init(&blob, n->osid);
>>>> if (security_audit_rule_match(
>>>> - n->osid,
>>>> + &blob,
>>>> f->type,
>>>> f->op,
>>>> f->lsm_rule)) {
>>>> @@ -678,7 +683,8 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>> /* Find ipc objects that match */
>>>> if (!ctx || ctx->type != AUDIT_IPC)
>>>> break;
>>>> - if (security_audit_rule_match(ctx->ipc.osid,
>>>> + lsmblob_init(&blob, ctx->ipc.osid);
>>>> + if (security_audit_rule_match(&blob,
>>>> f->type, f->op,
>>>> f->lsm_rule))
>>>> ++result;
>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>> index df4ca482fb53..d95b0ece7434 100644
>>>> --- a/security/integrity/ima/ima.h
>>>> +++ b/security/integrity/ima/ima.h
>>>> @@ -381,8 +381,8 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
>>>> return -EINVAL;
>>>> }
>>>> -static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
>>>> - void *lsmrule)
>>>> +static inline int security_filter_rule_match(struct lsmblob *blob, u32 field,
>>>> + u32 op, void *lsmrule)
>>>> {
>>>> return -EINVAL;
>>>> }
>>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>>> index f19a895ad7cd..193ddd55420b 100644
>>>> --- a/security/integrity/ima/ima_policy.c
>>>> +++ b/security/integrity/ima/ima_policy.c
>>>> @@ -414,6 +414,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>>> for (i = 0; i < MAX_LSM_RULES; i++) {
>>>> int rc = 0;
>>>> u32 osid;
>>>> + struct lsmblob blob;
>>>> if (!rule->lsm[i].rule)
>>>> continue;
>>>> @@ -423,7 +424,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>>> case LSM_OBJ_ROLE:
>>>> case LSM_OBJ_TYPE:
>>>> security_inode_getsecid(inode, &osid);
>>>> - rc = security_filter_rule_match(osid,
>>>> + lsmblob_init(&blob, osid);
>>>> + rc = security_filter_rule_match(&blob,
>>>> rule->lsm[i].type,
>>>> Audit_equal,
>>>> rule->lsm[i].rule);
>>>> @@ -431,7 +433,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>>> case LSM_SUBJ_USER:
>>>> case LSM_SUBJ_ROLE:
>>>> case LSM_SUBJ_TYPE:
>>>> - rc = security_filter_rule_match(secid,
>>>> + lsmblob_init(&blob, secid);
>>>> + rc = security_filter_rule_match(&blob,
>>>> rule->lsm[i].type,
>>>> Audit_equal,
>>>> rule->lsm[i].rule);
>>>> diff --git a/security/security.c b/security/security.c
>>>> index a89634af639a..bfea9739c084 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -439,7 +439,7 @@ static int lsm_append(const char *new, char **result)
>>>> /*
>>>> * Current index to use while initializing the lsmblob secid list.
>>>> */
>>>> -static int lsm_slot __initdata;
>>>> +static int lsm_slot __lsm_ro_after_init;
>>>> /**
>>>> * security_add_hooks - Add a modules hooks to the hook lists.
>>>> @@ -2412,9 +2412,21 @@ void security_audit_rule_free(void *lsmrule)
>>>> call_void_hook(audit_rule_free, lsmrule);
>>>> }
>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>>> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
>>>> + void *lsmrule)
>>>> {
>>>> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>>>> + struct security_hook_list *hp;
>>>> + int rc;
>>>> +
>>>> + hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>>>> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>> + continue;
>>> Do you think we really need to retain these WARN_ON()s?
>> Kees was especially keen on having the WARN_ON().
>> I'd be fine with removing it.
> It should really really never happen, so I like the WARN_ON staying.
>
> -Kees
Given that Mr. Hardening likes it the way it is, I'm inclined to leave
it as is. Would that prevent an Ack?
>
>>
>>> If not, then you could dispense with it now and leave lsm_slot as __initdata? Otherwise,
>>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>
>>>> + rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
>>>> + field, op, lsmrule);
>>>> + if (rc != 0)
>>>> + return rc;
>>>> + }
>>>> + return 0;
>>>> }
>>>> #endif /* CONFIG_AUDIT */
>>>>
^ permalink raw reply
* RE: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Ravi Kumar Siddojigari @ 2019-12-18 5:58 UTC (permalink / raw)
To: 'Stephen Smalley', selinux; +Cc: paul, linux-security-module
In-Reply-To: <0f6b6f32-e4bc-1ec0-dc27-2f4214ea479a@tycho.nsa.gov>
Yes this is the first time that we are getting this stress tested done on v4.19 kernel .
We had not tested this prior version of kernel though . Current proposed changes seems to really help and testing is still going on .
As per the delta it looks change 6b6bc620 seem to be missing in earlier version of kernel not sure if this was the cause.
Br ,
Ravi.
-----Original Message-----
From: Stephen Smalley <sds@tycho.nsa.gov>
Sent: Tuesday, December 17, 2019 9:54 PM
To: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>; selinux@vger.kernel.org
Cc: paul@paul-moore.com; linux-security-module@vger.kernel.org
Subject: Re: Looks like issue in handling active_nodes count in 4.19 kernel .
On 12/17/19 10:52 AM, Stephen Smalley wrote:
> On 12/17/19 10:40 AM, Ravi Kumar Siddojigari wrote:
>> Yes indeed this is a stress test on ARM64 device with multicore
>> where most of the cores /tasks are stuck in avc_reclaim_node .
>> We still see this issue even after picking the earlier patch "
>> selinux: ensure we cleanup the internal AVC counters on error in
>> avc_insert() commit: d8db60cb23e4"
>> Where selinux_state during issue was as below where all the slots
>> are NULL and the count was more than threshold.
>> Which seem to be calling avc_reclaim_node always and as the all the
>> slots are empty its going for full for- loop with locks and unlock
>> and taking too long .
>> Not sure what could make the slots null , for sure its not due to
>> flush() /Reset(). We think that still we need to call avc_kill_node
>> in update_node function .
>> Adding the patch below can you please review or correct the following
>> patch .
>>
>>
>> selinux_state = (
>> disabled = FALSE,
>> enforcing = TRUE,
>> checkreqprot = FALSE,
>> initialized = TRUE,
>> policycap = (TRUE, TRUE, TRUE, FALSE, FALSE, TRUE),
>> avc = 0xFFFFFF9BEFF1E890 -> (
>> avc_cache_threshold = 512, /* <<<<<not configured and its
>> with default*/
>> avc_cache = (
>> slots = ((first = 0x0), (first = 0x0), (first = 0x0), (first
>> = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0),
>> (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first
>> /*<<<< all are NULL */
>> slots_lock = ((rlock = (raw_lock = (val = (counter = 0),
>> locked = 0, pending = 0, locked_pending = 0, tail = 0), magic =
>> 3735899821, owner_cpu = 4294967295, owner = 0xFFFFFFFFFFFFFFFF,
>> dep_map = (key = 0xFFFFFF9BEFF298A8, cla
>> lru_hint = (counter = 616831529),
>> active_nodes = (counter = 547), /*<<<<< increased more
>> than 512*/
>> latest_notif = 1)),
>> ss = 0xFFFFFF9BEFF2E578)
>>
>>
>> --
>> In AVC update we don't call avc_node_kill() when
>> avc_xperms_populate() fails, resulting in the
>> avc->avc_cache.active_nodes counter having a false value.In last patch this changes was missed , so correcting it.
>>
>> Change-Id: Ic0298162cc766c0f21be7ab232e259766654dad3
>> Signed-off-by: Jaihind Yadav<jaihindyadav@codeaurora.org>
>> ---
>> security/selinux/avc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c index
>> 91d24c2..3d1cff2 100644
>> --- a/security/selinux/avc.c
>> +++ b/security/selinux/avc.c
>> @@ -913,7 +913,7 @@ static int avc_update_node(struct selinux_avc
>> *avc,
>> if (orig->ae.xp_node) {
>> rc = avc_xperms_populate(node, orig->ae.xp_node);
>> if (rc) {
>> - kmem_cache_free(avc_node_cachep, node);
>> + avc_node_kill(avc, node);
>> goto out_unlock;
>> }
>> }
>> --
>
> That looks correct to me; I guess that one got missed by the prior fix.
> Still not sure how your AVC got into that state though...
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
BTW, have you been running these stress tests on earlier kernels too?
If so, what version(s) are known to pass them? I ask because this code has been present since v4.3 and this is the first such report.
^ permalink raw reply
* [PATCH v4 0/7] Introduce CAP_SYS_PERFMON to secure system performance monitoring and observability
From: Alexey Budankov @ 2019-12-18 9:16 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
Currently access to perf_events, i915_perf and other performance monitoring and
observability subsystems of the kernel is open for a privileged process [1] with
CAP_SYS_ADMIN capability enabled in the process effective set [2].
This patch set introduces CAP_SYS_PERFMON capability devoted to secure system
performance monitoring and observability operations so that CAP_SYS_PERFMON would
assist CAP_SYS_ADMIN capability in its governing role for perf_events, i915_perf
and other performance monitoring and observability subsystems of the kernel.
CAP_SYS_PERFMON intends to meet the demand to secure system performance monitoring
and observability operations in security sensitive, restricted, production
environments (e.g. HPC clusters, cloud and virtual compute environments) where root
or CAP_SYS_ADMIN credentials are not available to mass users of a system because
of security considerations.
CAP_SYS_PERFMON intends to harden system security and integrity during system
performance monitoring and observability operations by decreasing attack surface
that is available to CAP_SYS_ADMIN privileged processes [2].
CAP_SYS_PERFMON intends to take over CAP_SYS_ADMIN credentials related to system
performance monitoring and observability operations and balance amount of
CAP_SYS_ADMIN credentials following the recommendations in the capabilities man
page [2] for CAP_SYS_ADMIN: "Note: this capability is overloaded; see Notes to
kernel developers, below."
For backward compatibility reasons access to system performance monitoring and
observability subsystems of the kernel remains open for CAP_SYS_ADMIN privileged
processes but CAP_SYS_ADMIN capability usage for secure system performance
monitoring and observability operations is discouraged with respect to the
introduced CAP_SYS_PERFMON capability.
The patch set is for tip perf/core repository:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip perf/core
sha1: ceb9e77324fa661b1001a0ae66f061b5fcb4e4e6
---
Changes in v4:
- converted perfmon_capable() into an inline function
- made perf_events kprobes, uprobes, hw breakpoints and namespaces data available
to CAP_SYS_PERFMON privileged processes
- applied perfmon_capable() to drivers/perf and drivers/oprofile
- extended __cmd_ftrace() with support of CAP_SYS_PERFMON
Changes in v3:
- implemented perfmon_capable() macros aggregating required capabilities checks
Changes in v2:
- made perf_events trace points available to CAP_SYS_PERFMON privileged processes
- made perf_event_paranoid_check() treat CAP_SYS_PERFMON equally to CAP_SYS_ADMIN
- applied CAP_SYS_PERFMON to i915_perf, bpf_trace, powerpc and parisc system
performance monitoring and observability related subsystems
---
Alexey Budankov (9):
capabilities: introduce CAP_SYS_PERFMON to kernel and user space
perf/core: open access for CAP_SYS_PERFMON privileged process
perf tool: extend Perf tool with CAP_SYS_PERFMON capability support
drm/i915/perf: open access for CAP_SYS_PERFMON privileged process
trace/bpf_trace: open access for CAP_SYS_PERFMON privileged process
powerpc/perf: open access for CAP_SYS_PERFMON privileged process
parisc/perf: open access for CAP_SYS_PERFMON privileged process
drivers/perf: open access for CAP_SYS_PERFMON privileged process
drivers/oprofile: open access for CAP_SYS_PERFMON privileged process
arch/parisc/kernel/perf.c | 2 +-
arch/powerpc/perf/imc-pmu.c | 4 ++--
drivers/gpu/drm/i915/i915_perf.c | 13 ++++++-------
drivers/oprofile/event_buffer.c | 2 +-
drivers/perf/arm_spe_pmu.c | 4 ++--
include/linux/capability.h | 4 ++++
include/linux/perf_event.h | 6 +++---
include/uapi/linux/capability.h | 8 +++++++-
kernel/events/core.c | 6 +++---
kernel/trace/bpf_trace.c | 2 +-
security/selinux/include/classmap.h | 4 ++--
tools/perf/builtin-ftrace.c | 5 +++--
tools/perf/design.txt | 3 ++-
tools/perf/util/cap.h | 4 ++++
tools/perf/util/evsel.c | 10 +++++-----
tools/perf/util/util.c | 1 +
16 files changed, 47 insertions(+), 31 deletions(-)
---
Testing and validation (Intel Skylake, 8 cores, Fedora 29, 5.4.0-rc8+, x86_64):
libcap library [3], [4] and Perf tool can be used to apply CAP_SYS_PERFMON
capability for secure system performance monitoring and observability beyond the
scope permitted by the system wide perf_event_paranoid kernel setting [5] and
below are the steps for evaluation:
- patch, build and boot the kernel
- patch, build Perf tool e.g. to /home/user/perf
...
# git clone git://git.kernel.org/pub/scm/libs/libcap/libcap.git libcap
# pushd libcap
# patch libcap/include/uapi/linux/capabilities.h with [PATCH 1]
# make
# pushd progs
# ./setcap "cap_sys_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
# ./setcap -v "cap_sys_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
/home/user/perf: OK
# ./getcap /home/user/perf
/home/user/perf = cap_sys_ptrace,cap_syslog,cap_sys_perfmon+ep
# echo 2 > /proc/sys/kernel/perf_event_paranoid
# cat /proc/sys/kernel/perf_event_paranoid
2
...
$ /home/user/perf top
... works as expected ...
$ cat /proc/`pidof perf`/status
Name: perf
Umask: 0002
State: S (sleeping)
Tgid: 2958
Ngid: 0
Pid: 2958
PPid: 9847
TracerPid: 0
Uid: 500 500 500 500
Gid: 500 500 500 500
FDSize: 256
...
CapInh: 0000000000000000
CapPrm: 0000004400080000
CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
cap_sys_perfmon,cap_sys_ptrace,cap_syslog
CapBnd: 0000007fffffffff
CapAmb: 0000000000000000
NoNewPrivs: 0
Seccomp: 0
Speculation_Store_Bypass: thread vulnerable
Cpus_allowed: ff
Cpus_allowed_list: 0-7
...
Usage of cap_sys_perfmon effectively avoids unused credentials excess:
- with cap_sys_admin:
CapEff: 0000007fffffffff => 01111111 11111111 11111111 11111111 11111111
- with cap_sys_perfmon:
CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
38 34 19
sys_perfmon syslog sys_ptrace
---
[1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
[2] http://man7.org/linux/man-pages/man7/capabilities.7.html
[3] http://man7.org/linux/man-pages/man8/setcap.8.html
[4] https://git.kernel.org/pub/scm/libs/libcap/libcap.git
[5] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
[6] https://sites.google.com/site/fullycapable/, posix_1003.1e-990310.pdf
--
2.20.1
^ permalink raw reply
* [PATCH v4 1/9] capabilities: introduce CAP_SYS_PERFMON to kernel and user space
From: Alexey Budankov @ 2019-12-18 9:24 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Introduce CAP_SYS_PERFMON capability devoted to secure system performance
monitoring and observability operations so that CAP_SYS_PERFMON would
assist CAP_SYS_ADMIN capability in its governing role for perf_events,
i915_perf and other subsystems of the kernel.
CAP_SYS_PERFMON intends to harden system security and integrity during
system performance monitoring and observability operations by decreasing
attack surface that is available to CAP_SYS_ADMIN privileged processes.
CAP_SYS_PERFMON intends to take over CAP_SYS_ADMIN credentials related
to system performance monitoring and observability operations and balance
amount of CAP_SYS_ADMIN credentials in accordance with the recommendations
provided in the man page for CAP_SYS_ADMIN [1]: "Note: this capability
is overloaded; see Notes to kernel developers, below."
[1] http://man7.org/linux/man-pages/man7/capabilities.7.html
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
include/linux/capability.h | 4 ++++
include/uapi/linux/capability.h | 8 +++++++-
security/selinux/include/classmap.h | 4 ++--
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index ecce0f43c73a..883c879baa4b 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -251,6 +251,10 @@ extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct
extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
+static inline bool perfmon_capable(void)
+{
+ return capable(CAP_SYS_PERFMON) || capable(CAP_SYS_ADMIN);
+}
/* audit system wants to get cap info from files as well */
extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 240fdb9a60f6..98e03cc76c7c 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -366,8 +366,14 @@ struct vfs_ns_cap_data {
#define CAP_AUDIT_READ 37
+/*
+ * Allow system performance and observability privileged operations
+ * using perf_events, i915_perf and other kernel subsystems
+ */
+
+#define CAP_SYS_PERFMON 38
-#define CAP_LAST_CAP CAP_AUDIT_READ
+#define CAP_LAST_CAP CAP_SYS_PERFMON
#define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 7db24855e12d..bae602c623b0 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -27,9 +27,9 @@
"audit_control", "setfcap"
#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
- "wake_alarm", "block_suspend", "audit_read"
+ "wake_alarm", "block_suspend", "audit_read", "sys_perfmon"
-#if CAP_LAST_CAP > CAP_AUDIT_READ
+#if CAP_LAST_CAP > CAP_SYS_PERFMON
#error New capability defined, please update COMMON_CAP2_PERMS.
#endif
--
2.20.1
^ permalink raw reply related
* [PATCH v4 2/9] perf/core: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2019-12-18 9:25 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Open access to perf_events monitoring for CAP_SYS_PERFMON privileged
processes. For backward compatibility reasons access to perf_events
subsystem remains open for CAP_SYS_ADMIN privileged processes but
CAP_SYS_ADMIN usage for secure perf_events monitoring is discouraged
with respect to CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
include/linux/perf_event.h | 6 +++---
kernel/events/core.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 34c7c6910026..f46acd69425f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1285,7 +1285,7 @@ static inline int perf_is_paranoid(void)
static inline int perf_allow_kernel(struct perf_event_attr *attr)
{
- if (sysctl_perf_event_paranoid > 1 && !capable(CAP_SYS_ADMIN))
+ if (sysctl_perf_event_paranoid > 1 && !perfmon_capable())
return -EACCES;
return security_perf_event_open(attr, PERF_SECURITY_KERNEL);
@@ -1293,7 +1293,7 @@ static inline int perf_allow_kernel(struct perf_event_attr *attr)
static inline int perf_allow_cpu(struct perf_event_attr *attr)
{
- if (sysctl_perf_event_paranoid > 0 && !capable(CAP_SYS_ADMIN))
+ if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
return -EACCES;
return security_perf_event_open(attr, PERF_SECURITY_CPU);
@@ -1301,7 +1301,7 @@ static inline int perf_allow_cpu(struct perf_event_attr *attr)
static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
{
- if (sysctl_perf_event_paranoid > -1 && !capable(CAP_SYS_ADMIN))
+ if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
return -EPERM;
return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 059ee7116008..d9db414f2197 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9056,7 +9056,7 @@ static int perf_kprobe_event_init(struct perf_event *event)
if (event->attr.type != perf_kprobe.type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
/*
@@ -9116,7 +9116,7 @@ static int perf_uprobe_event_init(struct perf_event *event)
if (event->attr.type != perf_uprobe.type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
/*
@@ -11157,7 +11157,7 @@ SYSCALL_DEFINE5(perf_event_open,
}
if (attr.namespaces) {
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
}
--
2.20.1
^ permalink raw reply related
* [PATCH v4 3/9] perf tool: extend Perf tool with CAP_SYS_PERFMON capability support
From: Alexey Budankov @ 2019-12-18 9:26 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Extend error messages to mention CAP_SYS_PERFMON capability as an option
to substitute CAP_SYS_ADMIN capability for secure system performance
monitoring and observability operations. Make perf_event_paranoid_check()
and __cmd_ftrace() to be aware of CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
tools/perf/builtin-ftrace.c | 5 +++--
tools/perf/design.txt | 3 ++-
tools/perf/util/cap.h | 4 ++++
tools/perf/util/evsel.c | 10 +++++-----
tools/perf/util/util.c | 1 +
5 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index d5adc417a4ca..8096e9b5f4f9 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -284,10 +284,11 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int argc, const char **argv)
.events = POLLIN,
};
- if (!perf_cap__capable(CAP_SYS_ADMIN)) {
+ if (!(perf_cap__capable(CAP_SYS_PERFMON) ||
+ perf_cap__capable(CAP_SYS_ADMIN))) {
pr_err("ftrace only works for %s!\n",
#ifdef HAVE_LIBCAP_SUPPORT
- "users with the SYS_ADMIN capability"
+ "users with the CAP_SYS_PERFMON or CAP_SYS_ADMIN capability"
#else
"root"
#endif
diff --git a/tools/perf/design.txt b/tools/perf/design.txt
index 0453ba26cdbd..71755b3e1303 100644
--- a/tools/perf/design.txt
+++ b/tools/perf/design.txt
@@ -258,7 +258,8 @@ gets schedule to. Per task counters can be created by any user, for
their own tasks.
A 'pid == -1' and 'cpu == x' counter is a per CPU counter that counts
-all events on CPU-x. Per CPU counters need CAP_SYS_ADMIN privilege.
+all events on CPU-x. Per CPU counters need CAP_SYS_PERFMON or
+CAP_SYS_ADMIN privilege.
The 'flags' parameter is currently unused and must be zero.
diff --git a/tools/perf/util/cap.h b/tools/perf/util/cap.h
index 051dc590ceee..0f79fbf6638b 100644
--- a/tools/perf/util/cap.h
+++ b/tools/perf/util/cap.h
@@ -29,4 +29,8 @@ static inline bool perf_cap__capable(int cap __maybe_unused)
#define CAP_SYSLOG 34
#endif
+#ifndef CAP_SYS_PERFMON
+#define CAP_SYS_PERFMON 38
+#endif
+
#endif /* __PERF_CAP_H */
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index f4dea055b080..3a46325e3702 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2468,14 +2468,14 @@ int perf_evsel__open_strerror(struct evsel *evsel, struct target *target,
"You may not have permission to collect %sstats.\n\n"
"Consider tweaking /proc/sys/kernel/perf_event_paranoid,\n"
"which controls use of the performance events system by\n"
- "unprivileged users (without CAP_SYS_ADMIN).\n\n"
+ "unprivileged users (without CAP_SYS_PERFMON or CAP_SYS_ADMIN).\n\n"
"The current value is %d:\n\n"
" -1: Allow use of (almost) all events by all users\n"
" Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK\n"
- ">= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN\n"
- " Disallow raw tracepoint access by users without CAP_SYS_ADMIN\n"
- ">= 1: Disallow CPU event access by users without CAP_SYS_ADMIN\n"
- ">= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN\n\n"
+ ">= 0: Disallow ftrace function tracepoint by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN\n"
+ " Disallow raw tracepoint access by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN\n"
+ ">= 1: Disallow CPU event access by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN\n"
+ ">= 2: Disallow kernel profiling by users without CAP_SYS_PERFMON or CAP_SYS_ADMIN\n\n"
"To make this setting permanent, edit /etc/sysctl.conf too, e.g.:\n\n"
" kernel.perf_event_paranoid = -1\n" ,
target->system_wide ? "system-wide " : "",
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 969ae560dad9..9981db0d8d09 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -272,6 +272,7 @@ int perf_event_paranoid(void)
bool perf_event_paranoid_check(int max_level)
{
return perf_cap__capable(CAP_SYS_ADMIN) ||
+ perf_cap__capable(CAP_SYS_PERFMON) ||
perf_event_paranoid() <= max_level;
}
--
2.20.1
^ permalink raw reply related
* [PATCH v4 4/9] drm/i915/perf: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2019-12-18 9:27 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Open access to i915_perf monitoring for CAP_SYS_PERFMON privileged
processes. For backward compatibility reasons access to i915_perf
subsystem remains open for CAP_SYS_ADMIN privileged processes but
CAP_SYS_ADMIN usage for secure i915_perf monitoring is discouraged
with respect to CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
drivers/gpu/drm/i915/i915_perf.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index e42b86827d6b..e2697f8d04de 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2748,10 +2748,10 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
/* Similar to perf's kernel.perf_paranoid_cpu sysctl option
* we check a dev.i915.perf_stream_paranoid sysctl option
* to determine if it's ok to access system wide OA counters
- * without CAP_SYS_ADMIN privileges.
+ * without CAP_SYS_PERFMON or CAP_SYS_ADMIN privileges.
*/
if (privileged_op &&
- i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
+ i915_perf_stream_paranoid && !perfmon_capable()) {
DRM_DEBUG("Insufficient privileges to open system-wide i915 perf stream\n");
ret = -EACCES;
goto err_ctx;
@@ -2939,9 +2939,8 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
} else
oa_freq_hz = 0;
- if (oa_freq_hz > i915_oa_max_sample_rate &&
- !capable(CAP_SYS_ADMIN)) {
- DRM_DEBUG("OA exponent would exceed the max sampling frequency (sysctl dev.i915.oa_max_sample_rate) %uHz without root privileges\n",
+ if (oa_freq_hz > i915_oa_max_sample_rate && !perfmon_capable()) {
+ DRM_DEBUG("OA exponent would exceed the max sampling frequency (sysctl dev.i915.oa_max_sample_rate) %uHz without CAP_SYS_PERFMON or CAP_SYS_ADMIN privileges\n",
i915_oa_max_sample_rate);
return -EACCES;
}
@@ -3328,7 +3327,7 @@ int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
return -EINVAL;
}
- if (i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
+ if (i915_perf_stream_paranoid && !perfmon_capable()) {
DRM_DEBUG("Insufficient privileges to add i915 OA config\n");
return -EACCES;
}
@@ -3474,7 +3473,7 @@ int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
return -ENOTSUPP;
}
- if (i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
+ if (i915_perf_stream_paranoid && !perfmon_capable()) {
DRM_DEBUG("Insufficient privileges to remove i915 OA config\n");
return -EACCES;
}
--
2.20.1
^ permalink raw reply related
* [PATCH v4 5/9] trace/bpf_trace: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2019-12-18 9:28 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Open access to bpf_trace monitoring for CAP_SYS_PERFMON privileged
processes. For backward compatibility reasons access to bpf_trace
monitoring remains open for CAP_SYS_ADMIN privileged processes but
CAP_SYS_ADMIN usage for secure bpf_trace monitoring is discouraged
with respect to CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
kernel/trace/bpf_trace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 44bd08f2443b..bafe21ac6d92 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1272,7 +1272,7 @@ int perf_event_query_prog_array(struct perf_event *event, void __user *info)
u32 *ids, prog_cnt, ids_len;
int ret;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EPERM;
if (event->attr.type != PERF_TYPE_TRACEPOINT)
return -EINVAL;
--
2.20.1
^ permalink raw reply related
* [PATCH v4 6/9] powerpc/perf: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2019-12-18 9:28 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Open access to monitoring for CAP_SYS_PERFMON privileged processes.
For backward compatibility reasons access to the monitoring remains open
for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage for secure
monitoring is discouraged with respect to CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
arch/powerpc/perf/imc-pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index cb50a9e1fd2d..e837717492e4 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -898,7 +898,7 @@ static int thread_imc_event_init(struct perf_event *event)
if (event->attr.type != event->pmu->type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
/* Sampling not supported */
@@ -1307,7 +1307,7 @@ static int trace_imc_event_init(struct perf_event *event)
if (event->attr.type != event->pmu->type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
/* Return if this is a couting event */
--
2.20.1
^ permalink raw reply related
* [PATCH v4 7/9] parisc/perf: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2019-12-18 9:29 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Open access to monitoring for CAP_SYS_PERFMON privileged processes.
For backward compatibility reasons access to the monitoring remains open
for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage for secure
monitoring is discouraged with respect to CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
arch/parisc/kernel/perf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/parisc/kernel/perf.c b/arch/parisc/kernel/perf.c
index 676683641d00..c4208d027794 100644
--- a/arch/parisc/kernel/perf.c
+++ b/arch/parisc/kernel/perf.c
@@ -300,7 +300,7 @@ static ssize_t perf_write(struct file *file, const char __user *buf,
else
return -EFAULT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
if (count != sizeof(uint32_t))
--
2.20.1
^ permalink raw reply related
* [PATCH v4 8/9] drivers/perf: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2019-12-18 9:30 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Open access to monitoring for CAP_SYS_PERFMON privileged processes.
For backward compatibility reasons access to the monitoring remains open
for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage for secure
monitoring is discouraged with respect to CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
drivers/perf/arm_spe_pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 4e4984a55cd1..5dff81bc3324 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -274,7 +274,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
if (!attr->exclude_kernel)
reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
- if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && capable(CAP_SYS_ADMIN))
+ if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
return reg;
@@ -700,7 +700,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
return -EOPNOTSUPP;
reg = arm_spe_event_to_pmscr(event);
- if (!capable(CAP_SYS_ADMIN) &&
+ if (!perfmon_capable() &&
(reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
BIT(SYS_PMSCR_EL1_CX_SHIFT) |
BIT(SYS_PMSCR_EL1_PCT_SHIFT))))
--
2.20.1
^ permalink raw reply related
* [PATCH v4 9/9] drivers/oprofile: open access for CAP_SYS_PERFMON privileged process
From: Alexey Budankov @ 2019-12-18 9:31 UTC (permalink / raw)
To: Peter Zijlstra, Arnaldo Carvalho de Melo, Ingo Molnar,
jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
rodrigo.vivi@intel.com, Alexei Starovoitov,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <c0460c78-b1a6-b5f7-7119-d97e5998f308@linux.intel.com>
Open access to monitoring for CAP_SYS_PERFMON privileged processes.
For backward compatibility reasons access to the monitoring remains open
for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage for secure
monitoring is discouraged with respect to CAP_SYS_PERFMON capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
drivers/oprofile/event_buffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/oprofile/event_buffer.c b/drivers/oprofile/event_buffer.c
index 12ea4a4ad607..6c9edc8bbc95 100644
--- a/drivers/oprofile/event_buffer.c
+++ b/drivers/oprofile/event_buffer.c
@@ -113,7 +113,7 @@ static int event_buffer_open(struct inode *inode, struct file *file)
{
int err = -EPERM;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EPERM;
if (test_and_set_bit_lock(0, &buffer_opened))
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] integrity: Expose data structures required for include/linux/integrity.h
From: Florent Revest @ 2019-12-18 11:03 UTC (permalink / raw)
To: Mimi Zohar, Casey Schaufler, linux-integrity
Cc: jmorris, serge, revest, allison, armijn, bauerman, linux-kernel,
linux-security-module, kpsingh
In-Reply-To: <1576624105.4579.379.camel@linux.ibm.com>
On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote:
> On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote:
> > On 12/17/2019 5:47 AM, Florent Revest wrote:
> > > From: Florent Revest <revest@google.com>
> > >
> > > include/linux/integrity.h exposes the prototype of
> > > integrity_inode_get().
> > > However, it relies on struct integrity_iint_cache which is
> > > currently
> > > defined in an internal header, security/integrity/integrity.h.
> > >
> > > To allow the rest of the kernel to use integrity_inode_get,
> >
> > Why do you want to do this?
>
> ditto
My team works on KRSI (eBPF MAC policies presented at LSS by KP Singh).
https://lkml.org/lkml/2019/9/10/393 We identified file hashes gathered
from the integrity subsystem as an interesting field that we could
potentially someday expose to eBPF programs through helpers.
One of the reason behind writing KRSI is to replace a custom kernel
auditing module that currently needs to redefine those structures to
access them. I imagine other kernel modules could benefit from a file
hash API too.
This is the least intrusive patch I could come up with that allows us
to lookup a hash from an inode. I was surprised to find that
integrity_inode_get was exposed but not the structures it returns.
If the community is interested in a different file hash API, I'd be
happy to iterate on this patch based on your feedback.
> > > this patch
> > > moves the definition of the necessary structures from a private
> > > header
> > > to a global kernel header.
^ permalink raw reply
* Re: [PATCH v12 03/25] LSM: Use lsmblob in security_audit_rule_match
From: Stephen Smalley @ 2019-12-18 13:16 UTC (permalink / raw)
To: Casey Schaufler, Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul
In-Reply-To: <752bb0c9-6e3b-9a63-3dd9-e2cc81641e09@schaufler-ca.com>
On 12/17/19 7:28 PM, Casey Schaufler wrote:
> On 12/17/2019 3:47 PM, Kees Cook wrote:
>> On Tue, Dec 17, 2019 at 02:01:19PM -0800, Casey Schaufler wrote:
>>> On 12/17/2019 9:34 AM, Stephen Smalley wrote:
>>>> On 12/16/19 5:35 PM, Casey Schaufler wrote:
>>>>> Change the secid parameter of security_audit_rule_match
>>>>> to a lsmblob structure pointer. Pass the entry from the
>>>>> lsmblob structure for the approprite slot to the LSM hook.
>>>>>
>>>>> Change the users of security_audit_rule_match to use the
>>>>> lsmblob instead of a u32. In some cases this requires a
>>>>> temporary conversion using lsmblob_init() that will go
>>>>> away when other interfaces get converted.
>>>>>
>>>>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>>>> Reviewed-by: John Johansen <john.johansen@canonical.com>
>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>>> ---
>>>>> include/linux/security.h | 7 ++++---
>>>>> kernel/auditfilter.c | 7 +++++--
>>>>> kernel/auditsc.c | 14 ++++++++++----
>>>>> security/integrity/ima/ima.h | 4 ++--
>>>>> security/integrity/ima/ima_policy.c | 7 +++++--
>>>>> security/security.c | 18 +++++++++++++++---
>>>>> 6 files changed, 41 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>>>> index b74dc70088ca..9c6dbe248eaf 100644
>>>>> --- a/include/linux/security.h
>>>>> +++ b/include/linux/security.h
>>>>> @@ -1837,7 +1837,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
>>>>> #ifdef CONFIG_SECURITY
>>>>> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
>>>>> int security_audit_rule_known(struct audit_krule *krule);
>>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
>>>>> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
>>>>> + void *lsmrule);
>>>>> void security_audit_rule_free(void *lsmrule);
>>>>> #else
>>>>> @@ -1853,8 +1854,8 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
>>>>> return 0;
>>>>> }
>>>>> -static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
>>>>> - void *lsmrule)
>>>>> +static inline int security_audit_rule_match(struct lsmblob *blob, u32 field,
>>>>> + u32 op, void *lsmrule)
>>>>> {
>>>>> return 0;
>>>>> }
>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>>>> index b0126e9c0743..356db1dd276c 100644
>>>>> --- a/kernel/auditfilter.c
>>>>> +++ b/kernel/auditfilter.c
>>>>> @@ -1325,6 +1325,7 @@ int audit_filter(int msgtype, unsigned int listtype)
>>>>> struct audit_field *f = &e->rule.fields[i];
>>>>> pid_t pid;
>>>>> u32 sid;
>>>>> + struct lsmblob blob;
>>>>> switch (f->type) {
>>>>> case AUDIT_PID:
>>>>> @@ -1355,8 +1356,10 @@ int audit_filter(int msgtype, unsigned int listtype)
>>>>> case AUDIT_SUBJ_CLR:
>>>>> if (f->lsm_rule) {
>>>>> security_task_getsecid(current, &sid);
>>>>> - result = security_audit_rule_match(sid,
>>>>> - f->type, f->op, f->lsm_rule);
>>>>> + lsmblob_init(&blob, sid);
>>>>> + result = security_audit_rule_match(
>>>>> + &blob, f->type,
>>>>> + f->op, f->lsm_rule);
>>>>> }
>>>>> break;
>>>>> case AUDIT_EXE:
>>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>>> index 4effe01ebbe2..7566e5b1c419 100644
>>>>> --- a/kernel/auditsc.c
>>>>> +++ b/kernel/auditsc.c
>>>>> @@ -445,6 +445,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>>> const struct cred *cred;
>>>>> int i, need_sid = 1;
>>>>> u32 sid;
>>>>> + struct lsmblob blob;
>>>>> unsigned int sessionid;
>>>>> cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
>>>>> @@ -643,7 +644,9 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>>> security_task_getsecid(tsk, &sid);
>>>>> need_sid = 0;
>>>>> }
>>>>> - result = security_audit_rule_match(sid, f->type,
>>>>> + lsmblob_init(&blob, sid);
>>>>> + result = security_audit_rule_match(&blob,
>>>>> + f->type,
>>>>> f->op,
>>>>> f->lsm_rule);
>>>>> }
>>>>> @@ -658,15 +661,17 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>>> if (f->lsm_rule) {
>>>>> /* Find files that match */
>>>>> if (name) {
>>>>> + lsmblob_init(&blob, name->osid);
>>>>> result = security_audit_rule_match(
>>>>> - name->osid,
>>>>> + &blob,
>>>>> f->type,
>>>>> f->op,
>>>>> f->lsm_rule);
>>>>> } else if (ctx) {
>>>>> list_for_each_entry(n, &ctx->names_list, list) {
>>>>> + lsmblob_init(&blob, n->osid);
>>>>> if (security_audit_rule_match(
>>>>> - n->osid,
>>>>> + &blob,
>>>>> f->type,
>>>>> f->op,
>>>>> f->lsm_rule)) {
>>>>> @@ -678,7 +683,8 @@ static int audit_filter_rules(struct task_struct *tsk,
>>>>> /* Find ipc objects that match */
>>>>> if (!ctx || ctx->type != AUDIT_IPC)
>>>>> break;
>>>>> - if (security_audit_rule_match(ctx->ipc.osid,
>>>>> + lsmblob_init(&blob, ctx->ipc.osid);
>>>>> + if (security_audit_rule_match(&blob,
>>>>> f->type, f->op,
>>>>> f->lsm_rule))
>>>>> ++result;
>>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>>> index df4ca482fb53..d95b0ece7434 100644
>>>>> --- a/security/integrity/ima/ima.h
>>>>> +++ b/security/integrity/ima/ima.h
>>>>> @@ -381,8 +381,8 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
>>>>> return -EINVAL;
>>>>> }
>>>>> -static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
>>>>> - void *lsmrule)
>>>>> +static inline int security_filter_rule_match(struct lsmblob *blob, u32 field,
>>>>> + u32 op, void *lsmrule)
>>>>> {
>>>>> return -EINVAL;
>>>>> }
>>>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>>>> index f19a895ad7cd..193ddd55420b 100644
>>>>> --- a/security/integrity/ima/ima_policy.c
>>>>> +++ b/security/integrity/ima/ima_policy.c
>>>>> @@ -414,6 +414,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>>>> for (i = 0; i < MAX_LSM_RULES; i++) {
>>>>> int rc = 0;
>>>>> u32 osid;
>>>>> + struct lsmblob blob;
>>>>> if (!rule->lsm[i].rule)
>>>>> continue;
>>>>> @@ -423,7 +424,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>>>> case LSM_OBJ_ROLE:
>>>>> case LSM_OBJ_TYPE:
>>>>> security_inode_getsecid(inode, &osid);
>>>>> - rc = security_filter_rule_match(osid,
>>>>> + lsmblob_init(&blob, osid);
>>>>> + rc = security_filter_rule_match(&blob,
>>>>> rule->lsm[i].type,
>>>>> Audit_equal,
>>>>> rule->lsm[i].rule);
>>>>> @@ -431,7 +433,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>>>> case LSM_SUBJ_USER:
>>>>> case LSM_SUBJ_ROLE:
>>>>> case LSM_SUBJ_TYPE:
>>>>> - rc = security_filter_rule_match(secid,
>>>>> + lsmblob_init(&blob, secid);
>>>>> + rc = security_filter_rule_match(&blob,
>>>>> rule->lsm[i].type,
>>>>> Audit_equal,
>>>>> rule->lsm[i].rule);
>>>>> diff --git a/security/security.c b/security/security.c
>>>>> index a89634af639a..bfea9739c084 100644
>>>>> --- a/security/security.c
>>>>> +++ b/security/security.c
>>>>> @@ -439,7 +439,7 @@ static int lsm_append(const char *new, char **result)
>>>>> /*
>>>>> * Current index to use while initializing the lsmblob secid list.
>>>>> */
>>>>> -static int lsm_slot __initdata;
>>>>> +static int lsm_slot __lsm_ro_after_init;
>>>>> /**
>>>>> * security_add_hooks - Add a modules hooks to the hook lists.
>>>>> @@ -2412,9 +2412,21 @@ void security_audit_rule_free(void *lsmrule)
>>>>> call_void_hook(audit_rule_free, lsmrule);
>>>>> }
>>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>>>> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
>>>>> + void *lsmrule)
>>>>> {
>>>>> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>>>>> + struct security_hook_list *hp;
>>>>> + int rc;
>>>>> +
>>>>> + hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>>>>> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>>> + continue;
>>>> Do you think we really need to retain these WARN_ON()s?
>>> Kees was especially keen on having the WARN_ON().
>>> I'd be fine with removing it.
>> It should really really never happen, so I like the WARN_ON staying.
>>
>> -Kees
>
> Given that Mr. Hardening likes it the way it is, I'm inclined to leave
> it as is. Would that prevent an Ack?
No, I already acked it in my reply, just thought I'd ask about the WARN_ON.
>
>
>>
>>>
>>>> If not, then you could dispense with it now and leave lsm_slot as __initdata? Otherwise,
>>>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>>
>>>>> + rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
>>>>> + field, op, lsmrule);
>>>>> + if (rc != 0)
>>>>> + return rc;
>>>>> + }
>>>>> + return 0;
>>>>> }
>>>>> #endif /* CONFIG_AUDIT */
>>>>>
^ permalink raw reply
* Re: [PATCH] integrity: Expose data structures required for include/linux/integrity.h
From: Mimi Zohar @ 2019-12-18 13:34 UTC (permalink / raw)
To: Florent Revest, Casey Schaufler, linux-integrity
Cc: jmorris, serge, revest, allison, armijn, bauerman, linux-kernel,
linux-security-module, kpsingh
In-Reply-To: <2ae5127d76cbf78140fb2d6108c9ec70c7d8ae5d.camel@chromium.org>
On Wed, 2019-12-18 at 12:03 +0100, Florent Revest wrote:
> On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote:
> > On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote:
> > > On 12/17/2019 5:47 AM, Florent Revest wrote:
> > > > From: Florent Revest <revest@google.com>
> > > >
> > > > include/linux/integrity.h exposes the prototype of
> > > > integrity_inode_get().
> > > > However, it relies on struct integrity_iint_cache which is
> > > > currently
> > > > defined in an internal header, security/integrity/integrity.h.
> > > >
> > > > To allow the rest of the kernel to use integrity_inode_get,
> > >
> > > Why do you want to do this?
> >
> > ditto
>
> My team works on KRSI (eBPF MAC policies presented at LSS by KP Singh).
> https://lkml.org/lkml/2019/9/10/393 We identified file hashes gathered
> from the integrity subsystem as an interesting field that we could
> potentially someday expose to eBPF programs through helpers.
>
> One of the reason behind writing KRSI is to replace a custom kernel
> auditing module that currently needs to redefine those structures to
> access them. I imagine other kernel modules could benefit from a file
> hash API too.
>
> This is the least intrusive patch I could come up with that allows us
> to lookup a hash from an inode. I was surprised to find that
> integrity_inode_get was exposed but not the structures it returns.
>
> If the community is interested in a different file hash API, I'd be
> happy to iterate on this patch based on your feedback.
There's a major difference between returning just the file hash and
making the integrity_iint_cache structure public. Peter Moody's
original code queried the cache[1]. Why do you need access to the
structure itself?
FYI, if/when we get to IMA namespacing, the cache structure will
change.
Mimi
[1] ima: add the ability to query ima for the hash of a given file.
>
> > > > this patch
> > > > moves the definition of the necessary structures from a private
> > > > header
> > > > to a global kernel header.
>
^ permalink raw reply
* Re: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Stephen Smalley @ 2019-12-18 13:53 UTC (permalink / raw)
To: Ravi Kumar Siddojigari, selinux; +Cc: paul, linux-security-module
In-Reply-To: <002101d5b568$393887d0$aba99770$@codeaurora.org>
On 12/18/19 12:58 AM, Ravi Kumar Siddojigari wrote:
> Yes this is the first time that we are getting this stress tested done on v4.19 kernel .
> We had not tested this prior version of kernel though . Current proposed changes seems to really help and testing is still going on .
> As per the delta it looks change 6b6bc620 seem to be missing in earlier version of kernel not sure if this was the cause.
6b6bc620 shouldn't have altered any behavior; it was purely an
encapsulation of the data structures. Both of the bugs you've
identified were introduced by the xperms support in fa1aa143ac4a68.
Maybe they were harder to trigger when the AVC was still using
GFP_ATOMIC instead of GFP_NOWAIT, but they were bugs nonetheless.
>
> Br ,
> Ravi.
> -----Original Message-----
> From: Stephen Smalley <sds@tycho.nsa.gov>
> Sent: Tuesday, December 17, 2019 9:54 PM
> To: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>; selinux@vger.kernel.org
> Cc: paul@paul-moore.com; linux-security-module@vger.kernel.org
> Subject: Re: Looks like issue in handling active_nodes count in 4.19 kernel .
>
> On 12/17/19 10:52 AM, Stephen Smalley wrote:
>> On 12/17/19 10:40 AM, Ravi Kumar Siddojigari wrote:
>>> Yes indeed this is a stress test on ARM64 device with multicore
>>> where most of the cores /tasks are stuck in avc_reclaim_node .
>>> We still see this issue even after picking the earlier patch "
>>> selinux: ensure we cleanup the internal AVC counters on error in
>>> avc_insert() commit: d8db60cb23e4"
>>> Where selinux_state during issue was as below where all the slots
>>> are NULL and the count was more than threshold.
>>> Which seem to be calling avc_reclaim_node always and as the all the
>>> slots are empty its going for full for- loop with locks and unlock
>>> and taking too long .
>>> Not sure what could make the slots null , for sure its not due to
>>> flush() /Reset(). We think that still we need to call avc_kill_node
>>> in update_node function .
>>> Adding the patch below can you please review or correct the following
>>> patch .
>>>
>>>
>>> selinux_state = (
>>> disabled = FALSE,
>>> enforcing = TRUE,
>>> checkreqprot = FALSE,
>>> initialized = TRUE,
>>> policycap = (TRUE, TRUE, TRUE, FALSE, FALSE, TRUE),
>>> avc = 0xFFFFFF9BEFF1E890 -> (
>>> avc_cache_threshold = 512, /* <<<<<not configured and its
>>> with default*/
>>> avc_cache = (
>>> slots = ((first = 0x0), (first = 0x0), (first = 0x0), (first
>>> = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0),
>>> (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first
>>> /*<<<< all are NULL */
>>> slots_lock = ((rlock = (raw_lock = (val = (counter = 0),
>>> locked = 0, pending = 0, locked_pending = 0, tail = 0), magic =
>>> 3735899821, owner_cpu = 4294967295, owner = 0xFFFFFFFFFFFFFFFF,
>>> dep_map = (key = 0xFFFFFF9BEFF298A8, cla
>>> lru_hint = (counter = 616831529),
>>> active_nodes = (counter = 547), /*<<<<< increased more
>>> than 512*/
>>> latest_notif = 1)),
>>> ss = 0xFFFFFF9BEFF2E578)
>>>
>>>
>>> --
>>> In AVC update we don't call avc_node_kill() when
>>> avc_xperms_populate() fails, resulting in the
>>> avc->avc_cache.active_nodes counter having a false value.In last patch this changes was missed , so correcting it.
>>>
>>> Change-Id: Ic0298162cc766c0f21be7ab232e259766654dad3
>>> Signed-off-by: Jaihind Yadav<jaihindyadav@codeaurora.org>
>>> ---
>>> security/selinux/avc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c index
>>> 91d24c2..3d1cff2 100644
>>> --- a/security/selinux/avc.c
>>> +++ b/security/selinux/avc.c
>>> @@ -913,7 +913,7 @@ static int avc_update_node(struct selinux_avc
>>> *avc,
>>> if (orig->ae.xp_node) {
>>> rc = avc_xperms_populate(node, orig->ae.xp_node);
>>> if (rc) {
>>> - kmem_cache_free(avc_node_cachep, node);
>>> + avc_node_kill(avc, node);
>>> goto out_unlock;
>>> }
>>> }
>>> --
>>
>> That looks correct to me; I guess that one got missed by the prior fix.
>> Still not sure how your AVC got into that state though...
>>
>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>
> BTW, have you been running these stress tests on earlier kernels too?
> If so, what version(s) are known to pass them? I ask because this code has been present since v4.3 and this is the first such report.
>
^ permalink raw reply
* Re: [PATCH] integrity: Expose data structures required for include/linux/integrity.h
From: Mimi Zohar @ 2019-12-18 14:28 UTC (permalink / raw)
To: Florent Revest, Casey Schaufler, linux-integrity, Matthew Garrett
Cc: jmorris, serge, revest, allison, armijn, bauerman, linux-kernel,
linux-security-module, kpsingh
In-Reply-To: <1576676087.4579.396.camel@linux.ibm.com>
[Cc'ing Matthew]
On Wed, 2019-12-18 at 08:34 -0500, Mimi Zohar wrote:
> On Wed, 2019-12-18 at 12:03 +0100, Florent Revest wrote:
> > On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote:
> > > On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote:
> > > > On 12/17/2019 5:47 AM, Florent Revest wrote:
> > > > > From: Florent Revest <revest@google.com>
> > > > >
> > > > > include/linux/integrity.h exposes the prototype of
> > > > > integrity_inode_get().
> > > > > However, it relies on struct integrity_iint_cache which is
> > > > > currently
> > > > > defined in an internal header, security/integrity/integrity.h.
> > > > >
> > > > > To allow the rest of the kernel to use integrity_inode_get,
> > > >
> > > > Why do you want to do this?
> > >
> > > ditto
> >
> > My team works on KRSI (eBPF MAC policies presented at LSS by KP Singh).
> > https://lkml.org/lkml/2019/9/10/393 We identified file hashes gathered
> > from the integrity subsystem as an interesting field that we could
> > potentially someday expose to eBPF programs through helpers.
> >
> > One of the reason behind writing KRSI is to replace a custom kernel
> > auditing module that currently needs to redefine those structures to
> > access them. I imagine other kernel modules could benefit from a file
> > hash API too.
> >
> > This is the least intrusive patch I could come up with that allows us
> > to lookup a hash from an inode. I was surprised to find that
> > integrity_inode_get was exposed but not the structures it returns.
> >
> > If the community is interested in a different file hash API, I'd be
> > happy to iterate on this patch based on your feedback.
>
> There's a major difference between returning just the file hash and
> making the integrity_iint_cache structure public. Peter Moody's
> original code queried the cache[1]. Why do you need access to the
> structure itself?
>
> FYI, if/when we get to IMA namespacing, the cache structure will
> change.
>
> [1] ima: add the ability to query ima for the hash of a given file.
If you're using Peter's patch, or something similar, I'd appreciate
your taking the time to upstream it.
Mimi
>
> >
> > > > > this patch
> > > > > moves the definition of the necessary structures from a private
> > > > > header
> > > > > to a global kernel header.
> >
>
^ permalink raw reply
* Re: [PATCH v12 13/25] LSM: Specify which LSM to display
From: Stephen Smalley @ 2019-12-18 15:17 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-14-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Create a new entry "display" in the procfs attr directory for
> controlling which LSM security information is displayed for a
> process. A process can only read or write its own display value.
>
> The name of an active LSM that supplies hooks for
> human readable data may be written to "display" to set the
> value. The name of the LSM currently in use can be read from
> "display". At this point there can only be one LSM capable
> of display active. A helper function lsm_task_display() is
> provided to get the display slot for a task_struct.
>
> Setting the "display" requires that all security modules using
> setprocattr hooks allow the action. Each security module is
> responsible for defining its policy.
>
> AppArmor hook provided by John Johansen <john.johansen@canonical.com>
> SELinux hook provided by Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/proc/base.c | 1 +
> include/linux/lsm_hooks.h | 15 +++
> security/apparmor/include/apparmor.h | 3 +-
> security/apparmor/lsm.c | 32 +++++
> security/security.c | 169 ++++++++++++++++++++++++---
> security/selinux/hooks.c | 11 ++
> security/selinux/include/classmap.h | 2 +-
> security/smack/smack_lsm.c | 7 ++
> 8 files changed, 221 insertions(+), 19 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ebea9501afb8..950c200cb9ad 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2652,6 +2652,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "fscreate", 0666),
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate", 0666),
> + ATTR(NULL, "display", 0666),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack", 0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7eb808cde051..2bf82e1cf347 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2186,4 +2186,19 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
>
> extern int lsm_inode_alloc(struct inode *inode);
>
> +/**
> + * lsm_task_display - the "display" LSM for this task
> + * @task: The task to report on
> + *
> + * Returns the task's display LSM slot.
> + */
> +static inline int lsm_task_display(struct task_struct *task)
> +{
> + int *display = task->security;
> +
> + if (display)
> + return *display;
> + return LSMBLOB_INVALID;
> +}
> +
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h
> index 1fbabdb565a8..b1622fcb4394 100644
> --- a/security/apparmor/include/apparmor.h
> +++ b/security/apparmor/include/apparmor.h
> @@ -28,8 +28,9 @@
> #define AA_CLASS_SIGNAL 10
> #define AA_CLASS_NET 14
> #define AA_CLASS_LABEL 16
> +#define AA_CLASS_DISPLAY_LSM 17
>
> -#define AA_CLASS_LAST AA_CLASS_LABEL
> +#define AA_CLASS_LAST AA_CLASS_DISPLAY_LSM
>
> /* Control parameters settable through module/boot flags */
> extern enum audit_mode aa_g_audit;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 146d75e5e021..16b992235c11 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -612,6 +612,25 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
> return error;
> }
>
> +
> +static int profile_display_lsm(struct aa_profile *profile,
> + struct common_audit_data *sa)
> +{
> + struct aa_perms perms = { };
> + unsigned int state;
> +
> + state = PROFILE_MEDIATES(profile, AA_CLASS_DISPLAY_LSM);
> + if (state) {
> + aa_compute_perms(profile->policy.dfa, state, &perms);
> + aa_apply_modes_to_perms(profile, &perms);
> + aad(sa)->label = &profile->label;
> +
> + return aa_check_perms(profile, &perms, AA_MAY_WRITE, sa, NULL);
> + }
> +
> + return 0;
> +}
> +
> static int apparmor_setprocattr(const char *name, void *value,
> size_t size)
> {
> @@ -623,6 +642,19 @@ static int apparmor_setprocattr(const char *name, void *value,
> if (size == 0)
> return -EINVAL;
>
> + /* LSM infrastructure does actual setting of display if allowed */
> + if (!strcmp(name, "display")) {
> + struct aa_profile *profile;
> + struct aa_label *label;
> +
> + aad(&sa)->info = "set display lsm";
> + label = begin_current_label_crit_section();
> + error = fn_for_each_confined(label, profile,
> + profile_display_lsm(profile, &sa));
> + end_current_label_crit_section(label);
> + return error;
> + }
> +
> /* AppArmor requires that the buffer must be null terminated atm */
> if (args[size - 1] != '\0') {
> /* null terminate */
> diff --git a/security/security.c b/security/security.c
> index 32354942b7e8..aaac748e4d83 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -27,6 +27,7 @@
> #include <linux/backing-dev.h>
> #include <linux/string.h>
> #include <linux/msg.h>
> +#include <linux/binfmts.h>
> #include <net/flow.h>
> #include <net/sock.h>
>
> @@ -43,7 +44,14 @@ static struct kmem_cache *lsm_file_cache;
> static struct kmem_cache *lsm_inode_cache;
>
> char *lsm_names;
> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
> +
> +/*
> + * The task blob includes the "display" slot used for
> + * chosing which module presents contexts.
> + */
> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
> + .lbs_task = sizeof(int),
> +};
>
> /* Boot-time LSM user choice */
> static __initdata const char *chosen_lsm_order;
> @@ -438,8 +446,10 @@ static int lsm_append(const char *new, char **result)
>
> /*
> * Current index to use while initializing the lsmblob secid list.
> + * Pointers to the LSM id structures for local use.
> */
> static int lsm_slot __lsm_ro_after_init;
> +static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES];
>
> /**
> * security_add_hooks - Add a modules hooks to the hook lists.
> @@ -459,6 +469,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> if (lsmid->slot == LSMBLOB_NEEDED) {
> if (lsm_slot >= LSMBLOB_ENTRIES)
> panic("%s Too many LSMs registered.\n", __func__);
> + lsm_slotlist[lsm_slot] = lsmid;
> lsmid->slot = lsm_slot++;
> init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm,
> lsmid->slot);
> @@ -588,6 +599,8 @@ int lsm_inode_alloc(struct inode *inode)
> */
> static int lsm_task_alloc(struct task_struct *task)
> {
> + int *display;
> +
> if (blob_sizes.lbs_task == 0) {
> task->security = NULL;
> return 0;
> @@ -596,6 +609,15 @@ static int lsm_task_alloc(struct task_struct *task)
> task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
> if (task->security == NULL)
> return -ENOMEM;
> +
> + /*
> + * The start of the task blob contains the "display" LSM slot number.
> + * Start with it set to the invalid slot number, indicating that the
> + * default first registered LSM be displayed.
> + */
> + display = task->security;
> + *display = LSMBLOB_INVALID;
> +
> return 0;
> }
>
> @@ -1551,14 +1573,26 @@ int security_file_open(struct file *file)
>
> int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
> {
> + int *odisplay = current->security;
> + int *ndisplay;
> int rc = lsm_task_alloc(task);
>
> - if (rc)
> + if (unlikely(rc))
> return rc;
> +
> rc = call_int_hook(task_alloc, 0, task, clone_flags);
> - if (unlikely(rc))
> + if (unlikely(rc)) {
> security_task_free(task);
> - return rc;
> + return rc;
> + }
> +
> + if (odisplay) {
> + ndisplay = task->security;
> + if (ndisplay)
> + *ndisplay = *odisplay;
> + }
> +
> + return 0;
> }
>
> void security_task_free(struct task_struct *task)
> @@ -1955,23 +1989,110 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> char **value)
> {
> struct security_hook_list *hp;
> + int display = lsm_task_display(current);
> + int slot = 0;
> +
> + if (!strcmp(name, "display")) {
> + /*
> + * lsm_slot will be 0 if there are no displaying modules.
> + */
> + if (lsm_slot == 0)
> + return -EINVAL;
> +
> + /*
> + * Only allow getting the current process' display.
> + * There are too few reasons to get another process'
> + * display and too many LSM policy issues.
> + */
> + if (current != p)
> + return -EINVAL;
> +
> + display = lsm_task_display(p);
> + if (display != LSMBLOB_INVALID)
> + slot = display;
> + *value = kstrdup(lsm_slotlist[slot]->lsm, GFP_KERNEL);
> + if (*value)
> + return strlen(*value);
> + return -ENOMEM;
> + }
>
> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> + if (lsm == NULL && display != LSMBLOB_INVALID &&
> + display != hp->lsmid->slot)
> + continue;
> return hp->hook.getprocattr(p, name, value);
> }
> return -EINVAL;
> }
>
> +/**
> + * security_setprocattr - Set process attributes via /proc
> + * @lsm: name of module involved, or NULL
> + * @name: name of the attribute
> + * @value: value to set the attribute to
> + * @size: size of the value
> + *
> + * Set the process attribute for the specified security module
> + * to the specified value. Note that this can only be used to set
> + * the process attributes for the current, or "self" process.
> + * The /proc code has already done this check.
> + *
> + * Returns 0 on success, an appropriate code otherwise.
> + */
> int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size)
> {
> struct security_hook_list *hp;
> + char *term;
> + char *cp;
> + int *display = current->security;
> + int rc = -EINVAL;
> + int slot = 0;
> +
> + if (!strcmp(name, "display")) {
> + /*
> + * Change the "display" value only if all the security
> + * modules that support setting a procattr allow it.
> + * It is assumed that all such security modules will be
> + * cooperative.
> + */
> + if (size == 0)
> + return -EINVAL;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
> + list) {
> + rc = hp->hook.setprocattr(name, value, size);
> + if (rc < 0)
> + return rc;
> + }
> +
> + rc = -EINVAL;
> +
> + term = kmemdup_nul(value, size, GFP_KERNEL);
> + if (term == NULL)
> + return -ENOMEM;
> +
> + cp = strsep(&term, " \n");
> +
> + for (slot = 0; slot < lsm_slot; slot++)
> + if (!strcmp(cp, lsm_slotlist[slot]->lsm)) {
> + *display = lsm_slotlist[slot]->slot;
> + rc = size;
> + break;
> + }
> +
> + kfree(cp);
This makes me slightly nervous; I see that it is correct currently but
worry about cp being changed at some point to no longer refer to the
start of the allocated buffer. I'd favor not mutating term (i.e. pass a
temporary to strsep) and freeing it.
> + return rc;
> + }
>
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> + if (lsm == NULL && *display != LSMBLOB_INVALID &&
> + *display != hp->lsmid->slot)
> + continue;
> return hp->hook.setprocattr(name, value, size);
> }
> return -EINVAL;
> @@ -1991,15 +2112,15 @@ EXPORT_SYMBOL(security_ismaclabel);
> int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen)
> {
> struct security_hook_list *hp;
> - int rc;
> + int display = lsm_task_display(current);
>
> hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> continue;
> - rc = hp->hook.secid_to_secctx(blob->secid[hp->lsmid->slot],
> - secdata, seclen);
> - if (rc != 0)
> - return rc;
> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> + return hp->hook.secid_to_secctx(
> + blob->secid[hp->lsmid->slot],
> + secdata, seclen);
> }
> return 0;
> }
> @@ -2009,16 +2130,15 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob)
> {
> struct security_hook_list *hp;
> - int rc;
> + int display = lsm_task_display(current);
>
> lsmblob_init(blob, 0);
> hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> continue;
> - rc = hp->hook.secctx_to_secid(secdata, seclen,
> - &blob->secid[hp->lsmid->slot]);
> - if (rc != 0)
> - return rc;
> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> + return hp->hook.secctx_to_secid(secdata, seclen,
> + &blob->secid[hp->lsmid->slot]);
> }
> return 0;
> }
> @@ -2026,7 +2146,15 @@ EXPORT_SYMBOL(security_secctx_to_secid);
>
> void security_release_secctx(char *secdata, u32 seclen)
> {
> - call_void_hook(release_secctx, secdata, seclen);
> + struct security_hook_list *hp;
> + int *display = current->security;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> + if (display == NULL || *display == LSMBLOB_INVALID ||
> + *display == hp->lsmid->slot) {
> + hp->hook.release_secctx(secdata, seclen);
> + return;
> + }
> }
I was wondering why you didn't use lsm_task_display() here and retain
the same pattern as the other hooks?
> EXPORT_SYMBOL(security_release_secctx);
>
> @@ -2151,8 +2279,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> int __user *optlen, unsigned len)
> {
> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> - optval, optlen, len);
> + int display = lsm_task_display(current);
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
> + list)
> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> + return hp->hook.socket_getpeersec_stream(sock, optval,
> + optlen, len);
> + return -ENOPROTOOPT;
> }
>
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 97f2ee6e4080..b8501ca3c8f3 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6323,6 +6323,17 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> /*
> * Basic control over ability to set these attributes at all.
> */
> +
> + /*
> + * For setting display, we only perform a permission check;
> + * the actual update to the display value is handled by the
> + * LSM framework.
> + */
> + if (!strcmp(name, "display"))
> + return avc_has_perm(&selinux_state,
> + mysid, mysid, SECCLASS_PROCESS2,
> + PROCESS2__SETDISPLAY, NULL);
> +
> if (!strcmp(name, "exec"))
> error = avc_has_perm(&selinux_state,
> mysid, mysid, SECCLASS_PROCESS,
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 7db24855e12d..323da8a38c43 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -52,7 +52,7 @@ struct security_class_mapping secclass_map[] = {
> "execmem", "execstack", "execheap", "setkeycreate",
> "setsockcreate", "getrlimit", NULL } },
> { "process2",
> - { "nnp_transition", "nosuid_transition", NULL } },
> + { "nnp_transition", "nosuid_transition", "setdisplay", NULL } },
> { "system",
> { "ipc_info", "syslog_read", "syslog_mod",
> "syslog_console", "module_request", "module_load", NULL } },
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 82cbb3eeec76..9737ead06b39 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3518,6 +3518,13 @@ static int smack_setprocattr(const char *name, void *value, size_t size)
> struct smack_known_list_elem *sklep;
> int rc;
>
> + /*
> + * Allow the /proc/.../attr/current and SO_PEERSEC "display"
> + * to be reset at will.
> + */
> + if (strcmp(name, "display") == 0)
> + return 0;
> +
> if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
> return -EPERM;
>
>
^ permalink raw reply
* Re: [PATCH v12 14/25] LSM: Ensure the correct LSM context releaser
From: Stephen Smalley @ 2019-12-18 15:53 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-15-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Add a new lsmcontext data structure to hold all the information
> about a "security context", including the string, its size and
> which LSM allocated the string. The allocation information is
> necessary because LSMs have different policies regarding the
> lifecycle of these strings. SELinux allocates and destroys
> them on each use, whereas Smack provides a pointer to an entry
> in a list that never goes away.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-integrity@vger.kernel.org
> cc: netdev@vger.kernel.org
> ---
> drivers/android/binder.c | 10 +++++--
> fs/ceph/xattr.c | 6 +++-
> fs/nfs/nfs4proc.c | 8 +++--
> fs/nfsd/nfs4xdr.c | 7 +++--
> include/linux/security.h | 39 +++++++++++++++++++++++--
> include/net/scm.h | 5 +++-
> kernel/audit.c | 14 ++++++---
> kernel/auditsc.c | 12 ++++++--
> net/ipv4/ip_sockglue.c | 4 ++-
> net/netfilter/nf_conntrack_netlink.c | 4 ++-
> net/netfilter/nf_conntrack_standalone.c | 4 ++-
> net/netfilter/nfnetlink_queue.c | 13 ++++++---
> net/netlabel/netlabel_unlabeled.c | 19 +++++++++---
> net/netlabel/netlabel_user.c | 4 ++-
> security/security.c | 18 ++++++++----
> security/smack/smack_lsm.c | 14 ++++++---
> 16 files changed, 141 insertions(+), 40 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d12b5e828b8d..597d9802b89b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -128,6 +128,41 @@ enum lockdown_reason {
> LOCKDOWN_CONFIDENTIALITY_MAX,
> };
>
> +/*
> + * A "security context" is the text representation of
> + * the information used by LSMs.
> + * This structure contains the string, its length, and which LSM
> + * it is useful for.
> + */
> +struct lsmcontext {
> + char *context; /* Provided by the module */
> + u32 len;
> + int slot; /* Identifies the module */
> +};
> +
> +/**
> + * lsmcontext_init - initialize an lsmcontext structure.
> + * @cp: Pointer to the context to initialize
> + * @context: Initial context, or NULL
> + * @size: Size of context, or 0
> + * @slot: Which LSM provided the context
> + *
> + * Fill in the lsmcontext from the provided information.
> + * This is a scaffolding function that will be removed when
> + * lsmcontext integration is complete.
> + */
> +static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
> + u32 size, int slot)
> +{
> + cp->slot = slot;
> + cp->context = context;
> +
> + if (context == NULL || size == 0)
> + cp->len = 0;
> + else
> + cp->len = strlen(context);
> +}
Why do you recompute the length instead of just storing size? Aside from
being less efficient, it may also be incorrect; SELinux-generated
contexts include the terminating NUL byte.
> diff --git a/security/security.c b/security/security.c
> index aaac748e4d83..6310ca7e84ed 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2144,17 +2144,23 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
> -void security_release_secctx(char *secdata, u32 seclen)
> +void security_release_secctx(struct lsmcontext *cp)
> {
> struct security_hook_list *hp;
> - int *display = current->security;
> + bool found = false;
>
> hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> - if (display == NULL || *display == LSMBLOB_INVALID ||
> - *display == hp->lsmid->slot) {
> - hp->hook.release_secctx(secdata, seclen);
> - return;
> + if (cp->slot == hp->lsmid->slot) {
> + hp->hook.release_secctx(cp->context, cp->len);
> + found = true;
> + break;
> }
> +
> + memset(cp, 0, sizeof(*cp));
> +
> + if (!found)
> + pr_warn("%s context \"%s\" from slot %d not released\n",
> + __func__, cp->context, cp->slot);
Not sure we need this warning but regardless, you cleared cp before the
pr_warn() so the output won't be very useful.
> }
> EXPORT_SYMBOL(security_release_secctx);
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 9737ead06b39..8e960f82bf3f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4482,11 +4482,16 @@ static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
> return 0;
> }
>
> -/*
> - * There used to be a smack_release_secctx hook
> - * that did nothing back when hooks were in a vector.
> - * Now that there's a list such a hook adds cost.
> +/**
> + * smack_release_secctx - do everything necessary to free a context
> + * @secdata: Unused
> + * @seclen: Unused
> + *
> + * Do nothing but hold a slot in the hooks list.
> */
> +static void smack_release_secctx(char *secdata, u32 seclen)
> +{
> +}
>
> static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> {
> @@ -4729,6 +4734,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ismaclabel, smack_ismaclabel),
> LSM_HOOK_INIT(secid_to_secctx, smack_secid_to_secctx),
> LSM_HOOK_INIT(secctx_to_secid, smack_secctx_to_secid),
> + LSM_HOOK_INIT(release_secctx, smack_release_secctx),
> LSM_HOOK_INIT(inode_notifysecctx, smack_inode_notifysecctx),
> LSM_HOOK_INIT(inode_setsecctx, smack_inode_setsecctx),
> LSM_HOOK_INIT(inode_getsecctx, smack_inode_getsecctx),
>
Is this just to avoid the warning above? If so, I'd just get rid of the
warning instead.
^ permalink raw reply
* Re: [PATCH v12 15/25] LSM: Use lsmcontext in security_secid_to_secctx
From: Stephen Smalley @ 2019-12-18 16:06 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-16-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext pointer to allow return of the LSM identifier
> along with the context and context length. This allows
> security_release_secctx() to know how to release the
> context. Callers have been modified to use or save the
> returned data from the new structure.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
Usual disclaimer about needing to make sure netdev and perhaps others
(audit, binder?) have acked these changes.
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> drivers/android/binder.c | 26 +++++++---------
> include/linux/security.h | 4 +--
> include/net/scm.h | 10 ++-----
> kernel/audit.c | 29 +++++++-----------
> kernel/auditsc.c | 31 +++++++------------
> net/ipv4/ip_sockglue.c | 7 ++---
> net/netfilter/nf_conntrack_netlink.c | 14 +++++----
> net/netfilter/nf_conntrack_standalone.c | 7 ++---
> net/netfilter/nfnetlink_queue.c | 5 +++-
> net/netlabel/netlabel_unlabeled.c | 40 ++++++++-----------------
> net/netlabel/netlabel_user.c | 7 ++---
> security/security.c | 10 +++++--
> 12 files changed, 74 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 1bca4d589e87..3c5eee35aae6 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2859,9 +2859,7 @@ static void binder_transaction(struct binder_proc *proc,
> binder_size_t last_fixup_min_off = 0;
> struct binder_context *context = proc->context;
> int t_debug_id = atomic_inc_return(&binder_last_id);
> - char *secctx = NULL;
> - u32 secctx_sz = 0;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext lsmctx = { };
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3109,14 +3107,14 @@ static void binder_transaction(struct binder_proc *proc,
> size_t added_size;
>
> security_task_getsecid(proc->tsk, &blob);
> - ret = security_secid_to_secctx(&blob, &secctx, &secctx_sz);
> + ret = security_secid_to_secctx(&blob, &lsmctx);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> return_error_line = __LINE__;
> goto err_get_secctx_failed;
> }
> - added_size = ALIGN(secctx_sz, sizeof(u64));
> + added_size = ALIGN(lsmctx.len, sizeof(u64));
> extra_buffers_size += added_size;
> if (extra_buffers_size < added_size) {
> /* integer overflow of extra_buffers_size */
> @@ -3143,24 +3141,22 @@ static void binder_transaction(struct binder_proc *proc,
> t->buffer = NULL;
> goto err_binder_alloc_buf_failed;
> }
> - if (secctx) {
> + if (lsmctx.context) {
> int err;
> size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
> ALIGN(tr->offsets_size, sizeof(void *)) +
> ALIGN(extra_buffers_size, sizeof(void *)) -
> - ALIGN(secctx_sz, sizeof(u64));
> + ALIGN(lsmctx.len, sizeof(u64));
>
> t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
> err = binder_alloc_copy_to_buffer(&target_proc->alloc,
> t->buffer, buf_offset,
> - secctx, secctx_sz);
> + lsmctx.context, lsmctx.len);
> if (err) {
> t->security_ctx = 0;
> WARN_ON(1);
> }
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - secctx = NULL;
> + security_release_secctx(&lsmctx);
> }
> t->buffer->debug_id = t->debug_id;
> t->buffer->transaction = t;
> @@ -3216,7 +3212,7 @@ static void binder_transaction(struct binder_proc *proc,
> off_end_offset = off_start_offset + tr->offsets_size;
> sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));
> sg_buf_end_offset = sg_buf_offset + extra_buffers_size -
> - ALIGN(secctx_sz, sizeof(u64));
> + ALIGN(lsmctx.len, sizeof(u64));
> off_min = 0;
> for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
> buffer_offset += sizeof(binder_size_t)) {
> @@ -3492,10 +3488,8 @@ static void binder_transaction(struct binder_proc *proc,
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> err_bad_extra_size:
> - if (secctx) {
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmctx.context)
> + security_release_secctx(&lsmctx);
> err_get_secctx_failed:
> kfree(tcomplete);
> binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 597d9802b89b..00421941f683 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -530,7 +530,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen);
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob);
> void security_release_secctx(struct lsmcontext *cp);
> @@ -1335,7 +1335,7 @@ static inline int security_ismaclabel(const char *name)
> }
>
> static inline int security_secid_to_secctx(struct lsmblob *blob,
> - char **secdata, u32 *seclen)
> + struct lsmcontext *cp)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 30ba801c91bd..4a6ad8caf423 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -93,18 +93,14 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
> static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> {
> struct lsmcontext context;
> - char *secdata;
> - u32 seclen;
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - err = security_secid_to_secctx(&scm->lsmblob, &secdata,
> - &seclen);
> + err = security_secid_to_secctx(&scm->lsmblob, &context);
>
> if (!err) {
> - put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> - /*scaffolding*/
> - lsmcontext_init(&context, secdata, seclen, 0);
> + put_cmsg(msg, SOL_SOCKET, SCM_SECURITY,
> + context.len, context.context);
> security_release_secctx(&context);
> }
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3305c4af43a8..224c7b4a1bc0 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1178,9 +1178,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_buffer *ab;
> u16 msg_type = nlh->nlmsg_type;
> struct audit_sig_info *sig_data;
> - char *ctx = NULL;
> u32 len;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context = { };
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1418,25 +1417,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> case AUDIT_SIGNAL_INFO:
> len = 0;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> - &len);
> + err = security_secid_to_secctx(&audit_sig_lsm,
> + &context);
> if (err)
> return err;
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm)) {
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmblob_is_set(&audit_sig_lsm))
> + security_release_secctx(&context);
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - memcpy(sig_data->ctx, ctx, len);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + memcpy(sig_data->ctx, context.context, context.len);
> + security_release_secctx(&context);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> @@ -2061,26 +2057,23 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>
> int audit_log_task_context(struct audit_buffer *ab)
> {
> - char *ctx = NULL;
> - unsigned len;
> int error;
> struct lsmblob blob;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context;
>
> security_task_getsecid(current, &blob);
> if (!lsmblob_is_set(&blob))
> return 0;
>
> - error = security_secid_to_secctx(&blob, &ctx, &len);
> + error = security_secid_to_secctx(&blob, &context);
> if (error) {
> if (error != -EINVAL)
> goto error_path;
> return 0;
> }
>
> - audit_log_format(ab, " subj=%s", ctx);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + audit_log_format(ab, " subj=%s", context.context);
> + security_release_secctx(&context);
> return 0;
>
> error_path:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8790e7aafa7d..6d273183dd87 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -962,9 +962,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> struct lsmblob *blob, char *comm)
> {
> struct audit_buffer *ab;
> - struct lsmcontext lsmcxt;
> - char *ctx = NULL;
> - u32 len;
> + struct lsmcontext lsmctx;
> int rc = 0;
>
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
> @@ -975,13 +973,12 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> from_kuid(&init_user_ns, auid),
> from_kuid(&init_user_ns, uid), sessionid);
> if (lsmblob_is_set(blob)) {
> - if (security_secid_to_secctx(blob, &ctx, &len)) {
> + if (security_secid_to_secctx(blob, &lsmctx)) {
> audit_log_format(ab, " obj=(none)");
> rc = 1;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0); /*scaffolding*/
> - security_release_secctx(&lsmcxt);
> + audit_log_format(ab, " obj=%s", lsmctx.context);
> + security_release_secctx(&lsmctx);
> }
> }
> audit_log_format(ab, " ocomm=");
> @@ -1194,7 +1191,6 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>
> static void show_special(struct audit_context *context, int *call_panic)
> {
> - struct lsmcontext lsmcxt;
> struct audit_buffer *ab;
> int i;
>
> @@ -1218,17 +1214,15 @@ static void show_special(struct audit_context *context, int *call_panic)
> from_kgid(&init_user_ns, context->ipc.gid),
> context->ipc.mode);
> if (osid) {
> - char *ctx = NULL;
> - u32 len;
> + struct lsmcontext lsmcxt;
> struct lsmblob blob;
>
> lsmblob_init(&blob, osid);
> - if (security_secid_to_secctx(&blob, &ctx, &len)) {
> + if (security_secid_to_secctx(&blob, &lsmcxt)) {
> audit_log_format(ab, " osid=%u", osid);
> *call_panic = 1;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0);
> + audit_log_format(ab, " obj=%s", lsmcxt.context);
> security_release_secctx(&lsmcxt);
> }
> }
> @@ -1372,20 +1366,17 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> MAJOR(n->rdev),
> MINOR(n->rdev));
> if (n->osid != 0) {
> - char *ctx = NULL;
> - u32 len;
> struct lsmblob blob;
> - struct lsmcontext lsmcxt;
> + struct lsmcontext lsmctx;
>
> lsmblob_init(&blob, n->osid);
> - if (security_secid_to_secctx(&blob, &ctx, &len)) {
> + if (security_secid_to_secctx(&blob, &lsmctx)) {
> audit_log_format(ab, " osid=%u", n->osid);
> if (call_panic)
> *call_panic = 2;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0); /* scaffolding */
> - security_release_secctx(&lsmcxt);
> + audit_log_format(ab, " obj=%s", lsmctx.context);
> + security_release_secctx(&lsmctx);
> }
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 96d56a30ecca..27af7a6b8780 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -132,20 +132,17 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> {
> struct lsmcontext context;
> struct lsmblob lb;
> - char *secdata;
> - u32 seclen;
> int err;
>
> err = security_socket_getpeersec_dgram(NULL, skb, &lb);
> if (err)
> return;
>
> - err = security_secid_to_secctx(&lb, &secdata, &seclen);
> + err = security_secid_to_secctx(&lb, &context);
> if (err)
> return;
>
> - put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
> - lsmcontext_init(&context, secdata, seclen, 0); /* scaffolding */
> + put_cmsg(msg, SOL_IP, SCM_SECURITY, context.len, context.context);
> security_release_secctx(&context);
> }
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 2f233f40c926..255bcb886a2f 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -329,13 +329,12 @@ static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct)
> static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> {
> struct nlattr *nest_secctx;
> - int len, ret;
> - char *secctx;
> + int ret;
> struct lsmblob blob;
> struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, &secctx, &len);
> + ret = security_secid_to_secctx(&blob, &context);
> if (ret)
> return 0;
>
> @@ -344,13 +343,12 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> if (!nest_secctx)
> goto nla_put_failure;
>
> - if (nla_put_string(skb, CTA_SECCTX_NAME, secctx))
> + if (nla_put_string(skb, CTA_SECCTX_NAME, context.context))
> goto nla_put_failure;
> nla_nest_end(skb, nest_secctx);
>
> ret = 0;
> nla_put_failure:
> - lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> security_release_secctx(&context);
> return ret;
> }
> @@ -648,12 +646,16 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
> #ifdef CONFIG_NF_CONNTRACK_SECMARK
> int len, ret;
> struct lsmblob blob;
> + struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, NULL, &len);
> + ret = security_secid_to_secctx(&blob, &context);
> if (ret)
> return 0;
>
> + len = context.len;
> + security_release_secctx(&context);
> +
> return nla_total_size(0) /* CTA_SECCTX */
> + nla_total_size(sizeof(char) * len); /* CTA_SECCTX_NAME */
> #else
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 8601fcb99f7a..8969754d7fe9 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -173,19 +173,16 @@ static void ct_seq_stop(struct seq_file *s, void *v)
> static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> {
> int ret;
> - u32 len;
> - char *secctx;
> struct lsmblob blob;
> struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, &secctx, &len);
> + ret = security_secid_to_secctx(&blob, &context);
> if (ret)
> return;
>
> - seq_printf(s, "secctx=%s ", secctx);
> + seq_printf(s, "secctx=%s ", context.context);
>
> - lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> security_release_secctx(&context);
> }
> #else
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index cc3ef03ee198..2d6668fd026c 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -306,6 +306,7 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> struct lsmblob blob;
> + struct lsmcontext context = { };
>
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
> @@ -314,10 +315,12 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
>
> if (skb->secmark) {
> lsmblob_init(&blob, skb->secmark);
> - security_secid_to_secctx(&blob, secdata, &seclen);
> + security_secid_to_secctx(&blob, &context);
> + *secdata = context.context;
> }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> + seclen = context.len;
> #endif
> return seclen;
> }
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 288c005b44c7..c03fe9a4f7b9 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -374,8 +374,6 @@ int netlbl_unlhsh_add(struct net *net,
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> struct lsmcontext context;
> - char *secctx = NULL;
> - u32 secctx_len;
> struct lsmblob blob;
>
> if (addr_len != sizeof(struct in_addr) &&
> @@ -440,12 +438,9 @@ int netlbl_unlhsh_add(struct net *net,
> rcu_read_unlock();
> if (audit_buf != NULL) {
> lsmblob_init(&blob, secid);
> - if (security_secid_to_secctx(&blob,
> - &secctx,
> - &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + if (security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
> @@ -478,8 +473,6 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> @@ -503,11 +496,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> if (entry != NULL)
> lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob,
> - &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> @@ -546,8 +537,6 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> @@ -570,10 +559,9 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> if (entry != NULL)
> lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob,
> - &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> @@ -1091,8 +1079,6 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> struct lsmcontext context;
> void *data;
> u32 secid;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> @@ -1149,15 +1135,13 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> }
>
> lsmblob_init(&blob, secid);
> - ret_val = security_secid_to_secctx(&blob, &secctx, &secctx_len);
> + ret_val = security_secid_to_secctx(&blob, &context);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> NLBL_UNLABEL_A_SECCTX,
> - secctx_len,
> - secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + context.len,
> + context.context);
> security_release_secctx(&context);
> if (ret_val != 0)
> goto list_cb_failure;
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index ef139d8ae7cd..951ba0639d20 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -85,8 +85,6 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> {
> struct audit_buffer *audit_buf;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> if (audit_enabled == AUDIT_OFF)
> @@ -102,9 +100,8 @@ struct audit_buffer *netlbl_audit_start_common(int type,
>
> lsmblob_init(&blob, audit_info->secid);
> if (audit_info->secid != 0 &&
> - security_secid_to_secctx(&blob, &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " subj=%s", secctx);
> - lsmcontext_init(&context, secctx, secctx_len, 0);/*scaffolding*/
> + security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " subj=%s", context.context);
> security_release_secctx(&context);
> }
>
> diff --git a/security/security.c b/security/security.c
> index 6310ca7e84ed..4ba1a6ed36e0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2109,18 +2109,22 @@ int security_ismaclabel(const char *name)
> }
> EXPORT_SYMBOL(security_ismaclabel);
>
> -int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen)
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp)
> {
> struct security_hook_list *hp;
> int display = lsm_task_display(current);
>
> + memset(cp, 0, sizeof(*cp));
> +
> hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> continue;
> - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) {
> + cp->slot = hp->lsmid->slot;
> return hp->hook.secid_to_secctx(
> blob->secid[hp->lsmid->slot],
> - secdata, seclen);
> + &cp->context, &cp->len);
> + }
> }
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH v12 16/25] LSM: Use lsmcontext in security_dentry_init_security
From: Stephen Smalley @ 2019-12-18 16:16 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-17-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Change the security_dentry_init_security() interface to
> fill an lsmcontext structure instead of a void * data area
> and a length. The lone caller of this interface is NFS4,
> which may make copies of the data using its own mechanisms.
> A rework of the nfs4 code to use the lsmcontext properly
> is a significant project, so the coward's way out is taken,
> and the lsmcontext data from security_dentry_init_security()
> is copied, then released directly.
>
> This interface does not use the "display". There is currently
> not case where that is useful or reasonable.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/nfs/nfs4proc.c | 26 ++++++++++++++++----------
> include/linux/security.h | 7 +++----
> security/security.c | 29 +++++++++++++++++++++++++----
> 3 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index a30e36654c57..78d63f7f0088 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -112,6 +112,7 @@ static inline struct nfs4_label *
> nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> struct iattr *sattr, struct nfs4_label *label)
> {
> + struct lsmcontext context;
> int err;
>
> if (label == NULL)
> @@ -121,21 +122,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> return NULL;
>
> err = security_dentry_init_security(dentry, sattr->ia_mode,
> - &dentry->d_name, (void **)&label->label, &label->len);
> - if (err == 0)
> - return label;
> + &dentry->d_name, &context);
> +
> + if (err)
> + return NULL;
> +
> + label->label = kmemdup(context.context, context.len, GFP_KERNEL);
This seems unfortunate; it introduces an extra allocation/copy of the
context. I'd prefer to avoid it. Also wondering if GFP_KERNEL is
always safe here.
> + if (label->label == NULL)
> + label = NULL;
> + else
> + label->len = context.len;
> +
> + security_release_secctx(&context);
> +
> + return label;
>
> - return NULL;
> }
> static inline void
> nfs4_label_release_security(struct nfs4_label *label)
> {
> - struct lsmcontext scaff; /* scaffolding */
> -
> - if (label) {
> - lsmcontext_init(&scaff, label->label, label->len, 0);
> - security_release_secctx(&scaff);
> - }
> + kfree(label->label);
> }
> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
> {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 00421941f683..a5eba06a9382 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -398,8 +398,8 @@ int security_add_mnt_opt(const char *option, const char *val,
> int len, void **mnt_opts);
> int security_move_mount(const struct path *from_path, const struct path *to_path);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen);
> + const struct qstr *name,
> + struct lsmcontext *ctx);
> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> @@ -790,8 +790,7 @@ static inline void security_inode_free(struct inode *inode)
> static inline int security_dentry_init_security(struct dentry *dentry,
> int mode,
> const struct qstr *name,
> - void **ctx,
> - u32 *ctxlen)
> + struct lsmcontext *ctx)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/security.c b/security/security.c
> index 4ba1a6ed36e0..8aa107b57af9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1011,12 +1011,33 @@ void security_inode_free(struct inode *inode)
> inode_free_by_rcu);
> }
>
> +/*
> + * security_dentry_init_security - initial context for a dentry
> + * @dentry: directory entry
> + * @mode: access mode
> + * @name: path name
> + * @context: resulting security context
> + *
> + * Use at most one security module to get the initial
> + * security context. Do not use the "display".
> + *
> + * Returns -EOPNOTSUPP if not supplied by any module or the module result.
> + */
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen)
> + const struct qstr *name,
> + struct lsmcontext *cp)
> {
> - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> - name, ctx, ctxlen);
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security,
> + list) {
> + cp->slot = hp->lsmid->slot;
> + return hp->hook.dentry_init_security(dentry, mode, name,
> + (void **)&cp->context,
> + &cp->len);
> + }
> +
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_dentry_init_security);
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox