[v12,03/25] LSM: Use lsmblob in security_audit_rule_match
diff mbox series

Message ID 20191216223621.5127-4-casey@schaufler-ca.com
State New
Headers show
Series
  • [v12,01/25] LSM: Infrastructure management of the sock security
Related show

Commit Message

Casey Schaufler Dec. 16, 2019, 10:35 p.m. UTC
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(-)

Comments

Stephen Smalley Dec. 17, 2019, 5:34 p.m. UTC | #1
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?  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 */
>   
>
Casey Schaufler Dec. 17, 2019, 10:01 p.m. UTC | #2
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 */
>>  
>
Kees Cook Dec. 17, 2019, 11:47 p.m. UTC | #3
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 */
> >>  
> >
Casey Schaufler Dec. 18, 2019, 12:28 a.m. UTC | #4
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 */
>>>>
Stephen Smalley Dec. 18, 2019, 1:16 p.m. UTC | #5
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 */
>>>>>

Patch
diff mbox series

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;
+		rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
+					       field, op, lsmrule);
+		if (rc != 0)
+			return rc;
+	}
+	return 0;
 }
 #endif /* CONFIG_AUDIT */