[v39,01/42] integrity: disassociate ima_filter_rule from security_audit_rule

Commit Message

Casey Schaufler Dec. 15, 2023, 10:15 p.m. UTC

Create real functions for the ima_filter_rule interfaces.
These replace #defines that obscure the reuse of audit
interfaces. The new functions are put in security.c because
they use security module registered hooks that we don't
want exported.

Acked-by: Paul Moore <[e-mail]>
Reviewed-by: John Johansen <[e-mail]>
Signed-off-by: Casey Schaufler <[e-mail]>
To: Mimi Zohar <[e-mail]>
Cc: [e-mail]
---
 include/linux/security.h     | 24 ++++++++++++++++++++++++
 security/integrity/ima/ima.h | 26 --------------------------
 security/security.c          | 21 +++++++++++++++++++++
 3 files changed, 45 insertions(+), 26 deletions(-)

Comments

Roberto Sassu March 6, 2024, 9:54 a.m. UTC | #1

On Fri, 2023-12-15 at 14:15 -0800, Casey Schaufler wrote:
> Create real functions for the ima_filter_rule interfaces.
> These replace #defines that obscure the reuse of audit
> interfaces. The new functions are put in security.c because
> they use security module registered hooks that we don't
> want exported.

Beginner question: what makes IMA special, that the audit subsystem
does not need an AUDIT_LSM field to do the same that IMA would do?

In other words, why can't we add the lsm_id parameter to
security_audit_*() functions, so that IMA can just call those?

Thanks

Roberto

> Acked-by: Paul Moore <[e-mail]>
> Reviewed-by: John Johansen <[e-mail]>
> Signed-off-by: Casey Schaufler <[e-mail]>
> To: Mimi Zohar <[e-mail]>
> Cc: [e-mail]
> ---
>  include/linux/security.h     | 24 ++++++++++++++++++++++++
>  security/integrity/ima/ima.h | 26 --------------------------
>  security/security.c          | 21 +++++++++++++++++++++
>  3 files changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 750130a7b9dd..4790508818ee 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
>  #endif /* CONFIG_SECURITY */
>  #endif /* CONFIG_AUDIT */
>  
> +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> +void ima_filter_rule_free(void *lsmrule);
> +
> +#else
> +
> +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> +					   void **lsmrule)
> +{
> +	return 0;
> +}
> +
> +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> +					    void *lsmrule)
> +{
> +	return 0;
> +}
> +
> +static inline void ima_filter_rule_free(void *lsmrule)
> +{ }
> +
> +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> +
>  #ifdef CONFIG_SECURITYFS
>  
>  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..560d6104de72 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
>  }
>  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
>  
> -/* LSM based policy rules require audit */
> -#ifdef CONFIG_IMA_LSM_RULES
> -
> -#define ima_filter_rule_init security_audit_rule_init
> -#define ima_filter_rule_free security_audit_rule_free
> -#define ima_filter_rule_match security_audit_rule_match
> -
> -#else
> -
> -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> -				       void **lsmrule)
> -{
> -	return -EINVAL;
> -}
> -
> -static inline void ima_filter_rule_free(void *lsmrule)
> -{
> -}
> -
> -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> -					void *lsmrule)
> -{
> -	return -EINVAL;
> -}
> -#endif /* CONFIG_IMA_LSM_RULES */
> -
>  #ifdef	CONFIG_IMA_READ_POLICY
>  #define	POLICY_FILE_FLAGS	(S_IWUSR | S_IRUSR)
>  #else
> diff --git a/security/security.c b/security/security.c
> index d7b15ea67c3f..8e5379a76369 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>  }
>  #endif /* CONFIG_AUDIT */
>  
> +#ifdef CONFIG_IMA_LSM_RULES
> +/*
> + * The integrity subsystem uses the same hooks as
> + * the audit subsystem.
> + */
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> +{
> +	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> +}
> +
> +void ima_filter_rule_free(void *lsmrule)
> +{
> +	call_void_hook(audit_rule_free, lsmrule);
> +}
> +
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +{
> +	return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> +}
> +#endif /* CONFIG_IMA_LSM_RULES */
> +
>  #ifdef CONFIG_BPF_SYSCALL
>  /**
>   * security_bpf() - Check if the bpf syscall operation is allowed

Casey Schaufler March 6, 2024, 4:56 p.m. UTC | #2

On 3/6/2024 1:54 AM, Roberto Sassu wrote:
> On Fri, 2023-12-15 at 14:15 -0800, Casey Schaufler wrote:
>> Create real functions for the ima_filter_rule interfaces.
>> These replace #defines that obscure the reuse of audit
>> interfaces. The new functions are put in security.c because
>> they use security module registered hooks that we don't
>> want exported.
> Beginner question: what makes IMA special, that the audit subsystem
> does not need an AUDIT_LSM field to do the same that IMA would do?
>
> In other words, why can't we add the lsm_id parameter to
> security_audit_*() functions, so that IMA can just call those?

I have never liked the reuse of the audit filter functions, especially
the way that they're hidden behind #define. The assumption that the
two facilities (audit and IMA) are going to use them the same way, and
that they will never diverge in their requirements, has always seemed
questionable.

In most cases audit needs an lsmblob rather than a secid+lsm_id pair,
as there is information required about all the LSMs, not just the one
actively involved.

>
> Thanks
>
> Roberto
>
>> Acked-by: Paul Moore <[e-mail]>
>> Reviewed-by: John Johansen <[e-mail]>
>> Signed-off-by: Casey Schaufler <[e-mail]>
>> To: Mimi Zohar <[e-mail]>
>> Cc: [e-mail]
>> ---
>>  include/linux/security.h     | 24 ++++++++++++++++++++++++
>>  security/integrity/ima/ima.h | 26 --------------------------
>>  security/security.c          | 21 +++++++++++++++++++++
>>  3 files changed, 45 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 750130a7b9dd..4790508818ee 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
>>  #endif /* CONFIG_SECURITY */
>>  #endif /* CONFIG_AUDIT */
>>  
>> +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
>> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
>> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
>> +void ima_filter_rule_free(void *lsmrule);
>> +
>> +#else
>> +
>> +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
>> +					   void **lsmrule)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
>> +					    void *lsmrule)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void ima_filter_rule_free(void *lsmrule)
>> +{ }
>> +
>> +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
>> +
>>  #ifdef CONFIG_SECURITYFS
>>  
>>  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c29db699c996..560d6104de72 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
>>  }
>>  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
>>  
>> -/* LSM based policy rules require audit */
>> -#ifdef CONFIG_IMA_LSM_RULES
>> -
>> -#define ima_filter_rule_init security_audit_rule_init
>> -#define ima_filter_rule_free security_audit_rule_free
>> -#define ima_filter_rule_match security_audit_rule_match
>> -
>> -#else
>> -
>> -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
>> -				       void **lsmrule)
>> -{
>> -	return -EINVAL;
>> -}
>> -
>> -static inline void ima_filter_rule_free(void *lsmrule)
>> -{
>> -}
>> -
>> -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
>> -					void *lsmrule)
>> -{
>> -	return -EINVAL;
>> -}
>> -#endif /* CONFIG_IMA_LSM_RULES */
>> -
>>  #ifdef	CONFIG_IMA_READ_POLICY
>>  #define	POLICY_FILE_FLAGS	(S_IWUSR | S_IRUSR)
>>  #else
>> diff --git a/security/security.c b/security/security.c
>> index d7b15ea67c3f..8e5379a76369 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>  }
>>  #endif /* CONFIG_AUDIT */
>>  
>> +#ifdef CONFIG_IMA_LSM_RULES
>> +/*
>> + * The integrity subsystem uses the same hooks as
>> + * the audit subsystem.
>> + */
>> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>> +{
>> +	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
>> +}
>> +
>> +void ima_filter_rule_free(void *lsmrule)
>> +{
>> +	call_void_hook(audit_rule_free, lsmrule);
>> +}
>> +
>> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>> +{
>> +	return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>> +}
>> +#endif /* CONFIG_IMA_LSM_RULES */
>> +
>>  #ifdef CONFIG_BPF_SYSCALL
>>  /**
>>   * security_bpf() - Check if the bpf syscall operation is allowed

Roberto Sassu March 7, 2024, 7:56 a.m. UTC | #3

On Wed, 2024-03-06 at 08:56 -0800, Casey Schaufler wrote:
> On 3/6/2024 1:54 AM, Roberto Sassu wrote:
> > On Fri, 2023-12-15 at 14:15 -0800, Casey Schaufler wrote:
> > > Create real functions for the ima_filter_rule interfaces.
> > > These replace #defines that obscure the reuse of audit
> > > interfaces. The new functions are put in security.c because
> > > they use security module registered hooks that we don't
> > > want exported.
> > Beginner question: what makes IMA special, that the audit subsystem
> > does not need an AUDIT_LSM field to do the same that IMA would do?
> > 
> > In other words, why can't we add the lsm_id parameter to
> > security_audit_*() functions, so that IMA can just call those?
> 
> I have never liked the reuse of the audit filter functions, especially
> the way that they're hidden behind #define. The assumption that the
> two facilities (audit and IMA) are going to use them the same way, and
> that they will never diverge in their requirements, has always seemed
> questionable.
> 
> In most cases audit needs an lsmblob rather than a secid+lsm_id pair,
> as there is information required about all the LSMs, not just the one
> actively involved.

Fair enough.

Thanks

Roberto

> > 
> > Thanks
> > 
> > Roberto
> > 
> > > Acked-by: Paul Moore <[e-mail]>
> > > Reviewed-by: John Johansen <[e-mail]>
> > > Signed-off-by: Casey Schaufler <[e-mail]>
> > > To: Mimi Zohar <[e-mail]>
> > > Cc: [e-mail]
> > > ---
> > >  include/linux/security.h     | 24 ++++++++++++++++++++++++
> > >  security/integrity/ima/ima.h | 26 --------------------------
> > >  security/security.c          | 21 +++++++++++++++++++++
> > >  3 files changed, 45 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 750130a7b9dd..4790508818ee 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
> > >  #endif /* CONFIG_SECURITY */
> > >  #endif /* CONFIG_AUDIT */
> > >  
> > > +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> > > +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> > > +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> > > +void ima_filter_rule_free(void *lsmrule);
> > > +
> > > +#else
> > > +
> > > +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> > > +					   void **lsmrule)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> > > +					    void *lsmrule)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void ima_filter_rule_free(void *lsmrule)
> > > +{ }
> > > +
> > > +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> > > +
> > >  #ifdef CONFIG_SECURITYFS
> > >  
> > >  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
> > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > > index c29db699c996..560d6104de72 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
> > >  }
> > >  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
> > >  
> > > -/* LSM based policy rules require audit */
> > > -#ifdef CONFIG_IMA_LSM_RULES
> > > -
> > > -#define ima_filter_rule_init security_audit_rule_init
> > > -#define ima_filter_rule_free security_audit_rule_free
> > > -#define ima_filter_rule_match security_audit_rule_match
> > > -
> > > -#else
> > > -
> > > -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> > > -				       void **lsmrule)
> > > -{
> > > -	return -EINVAL;
> > > -}
> > > -
> > > -static inline void ima_filter_rule_free(void *lsmrule)
> > > -{
> > > -}
> > > -
> > > -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> > > -					void *lsmrule)
> > > -{
> > > -	return -EINVAL;
> > > -}
> > > -#endif /* CONFIG_IMA_LSM_RULES */
> > > -
> > >  #ifdef	CONFIG_IMA_READ_POLICY
> > >  #define	POLICY_FILE_FLAGS	(S_IWUSR | S_IRUSR)
> > >  #else
> > > diff --git a/security/security.c b/security/security.c
> > > index d7b15ea67c3f..8e5379a76369 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> > >  }
> > >  #endif /* CONFIG_AUDIT */
> > >  
> > > +#ifdef CONFIG_IMA_LSM_RULES
> > > +/*
> > > + * The integrity subsystem uses the same hooks as
> > > + * the audit subsystem.
> > > + */
> > > +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> > > +{
> > > +	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> > > +}
> > > +
> > > +void ima_filter_rule_free(void *lsmrule)
> > > +{
> > > +	call_void_hook(audit_rule_free, lsmrule);
> > > +}
> > > +
> > > +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> > > +{
> > > +	return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> > > +}
> > > +#endif /* CONFIG_IMA_LSM_RULES */
> > > +
> > >  #ifdef CONFIG_BPF_SYSCALL
> > >  /**
> > >   * security_bpf() - Check if the bpf syscall operation is allowed

Paul Moore June 21, 2024, 4:50 p.m. UTC | #4

On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <[e-mail]> wrote:
>
> Create real functions for the ima_filter_rule interfaces.
> These replace #defines that obscure the reuse of audit
> interfaces. The new functions are put in security.c because
> they use security module registered hooks that we don't
> want exported.
>
> Acked-by: Paul Moore <[e-mail]>
> Reviewed-by: John Johansen <[e-mail]>
> Signed-off-by: Casey Schaufler <[e-mail]>
> To: Mimi Zohar <[e-mail]>
> Cc: [e-mail]
> ---
>  include/linux/security.h     | 24 ++++++++++++++++++++++++
>  security/integrity/ima/ima.h | 26 --------------------------
>  security/security.c          | 21 +++++++++++++++++++++
>  3 files changed, 45 insertions(+), 26 deletions(-)

Mimi, Roberto, are you both okay if I merge this into the lsm/dev
branch?  The #define approach taken with the ima_filter_rule_XXX
macros likely contributed to the recent problem where the build
problem caused by the new gfp_t parameter was missed during review;
I'd like to get this into an upstream tree independent of the larger
stacking effort as I believe it has standalone value.

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 750130a7b9dd..4790508818ee 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
>  #endif /* CONFIG_SECURITY */
>  #endif /* CONFIG_AUDIT */
>
> +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> +void ima_filter_rule_free(void *lsmrule);
> +
> +#else
> +
> +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> +                                          void **lsmrule)
> +{
> +       return 0;
> +}
> +
> +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> +                                           void *lsmrule)
> +{
> +       return 0;
> +}
> +
> +static inline void ima_filter_rule_free(void *lsmrule)
> +{ }
> +
> +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> +
>  #ifdef CONFIG_SECURITYFS
>
>  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..560d6104de72 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
>  }
>  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
>
> -/* LSM based policy rules require audit */
> -#ifdef CONFIG_IMA_LSM_RULES
> -
> -#define ima_filter_rule_init security_audit_rule_init
> -#define ima_filter_rule_free security_audit_rule_free
> -#define ima_filter_rule_match security_audit_rule_match
> -
> -#else
> -
> -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> -                                      void **lsmrule)
> -{
> -       return -EINVAL;
> -}
> -
> -static inline void ima_filter_rule_free(void *lsmrule)
> -{
> -}
> -
> -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> -                                       void *lsmrule)
> -{
> -       return -EINVAL;
> -}
> -#endif /* CONFIG_IMA_LSM_RULES */
> -
>  #ifdef CONFIG_IMA_READ_POLICY
>  #define        POLICY_FILE_FLAGS       (S_IWUSR | S_IRUSR)
>  #else
> diff --git a/security/security.c b/security/security.c
> index d7b15ea67c3f..8e5379a76369 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>  }
>  #endif /* CONFIG_AUDIT */
>
> +#ifdef CONFIG_IMA_LSM_RULES
> +/*
> + * The integrity subsystem uses the same hooks as
> + * the audit subsystem.
> + */
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> +{
> +       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> +}
> +
> +void ima_filter_rule_free(void *lsmrule)
> +{
> +       call_void_hook(audit_rule_free, lsmrule);
> +}
> +
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +{
> +       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> +}
> +#endif /* CONFIG_IMA_LSM_RULES */
> +
>  #ifdef CONFIG_BPF_SYSCALL
>  /**
>   * security_bpf() - Check if the bpf syscall operation is allowed
> --
> 2.41.0
>

Paul Moore June 21, 2024, 7:07 p.m. UTC | #5

On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <[e-mail]> wrote:
> On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <[e-mail]> wrote:
> >
> > Create real functions for the ima_filter_rule interfaces.
> > These replace #defines that obscure the reuse of audit
> > interfaces. The new functions are put in security.c because
> > they use security module registered hooks that we don't
> > want exported.
> >
> > Acked-by: Paul Moore <[e-mail]>
> > Reviewed-by: John Johansen <[e-mail]>
> > Signed-off-by: Casey Schaufler <[e-mail]>
> > To: Mimi Zohar <[e-mail]>
> > Cc: [e-mail]
> > ---
> >  include/linux/security.h     | 24 ++++++++++++++++++++++++
> >  security/integrity/ima/ima.h | 26 --------------------------
> >  security/security.c          | 21 +++++++++++++++++++++
> >  3 files changed, 45 insertions(+), 26 deletions(-)
>
> Mimi, Roberto, are you both okay if I merge this into the lsm/dev
> branch?  The #define approach taken with the ima_filter_rule_XXX
> macros likely contributed to the recent problem where the build
> problem caused by the new gfp_t parameter was missed during review;
> I'd like to get this into an upstream tree independent of the larger
> stacking effort as I believe it has standalone value.

... and I just realized neither Mimi or Roberto were directly CC'd on
that last email, oops.  Fixed.

> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 750130a7b9dd..4790508818ee 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
> >  #endif /* CONFIG_SECURITY */
> >  #endif /* CONFIG_AUDIT */
> >
> > +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> > +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> > +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> > +void ima_filter_rule_free(void *lsmrule);
> > +
> > +#else
> > +
> > +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> > +                                          void **lsmrule)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> > +                                           void *lsmrule)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline void ima_filter_rule_free(void *lsmrule)
> > +{ }
> > +
> > +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> > +
> >  #ifdef CONFIG_SECURITYFS
> >
> >  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index c29db699c996..560d6104de72 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
> >  }
> >  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
> >
> > -/* LSM based policy rules require audit */
> > -#ifdef CONFIG_IMA_LSM_RULES
> > -
> > -#define ima_filter_rule_init security_audit_rule_init
> > -#define ima_filter_rule_free security_audit_rule_free
> > -#define ima_filter_rule_match security_audit_rule_match
> > -
> > -#else
> > -
> > -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> > -                                      void **lsmrule)
> > -{
> > -       return -EINVAL;
> > -}
> > -
> > -static inline void ima_filter_rule_free(void *lsmrule)
> > -{
> > -}
> > -
> > -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> > -                                       void *lsmrule)
> > -{
> > -       return -EINVAL;
> > -}
> > -#endif /* CONFIG_IMA_LSM_RULES */
> > -
> >  #ifdef CONFIG_IMA_READ_POLICY
> >  #define        POLICY_FILE_FLAGS       (S_IWUSR | S_IRUSR)
> >  #else
> > diff --git a/security/security.c b/security/security.c
> > index d7b15ea67c3f..8e5379a76369 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> >  }
> >  #endif /* CONFIG_AUDIT */
> >
> > +#ifdef CONFIG_IMA_LSM_RULES
> > +/*
> > + * The integrity subsystem uses the same hooks as
> > + * the audit subsystem.
> > + */
> > +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> > +{
> > +       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> > +}
> > +
> > +void ima_filter_rule_free(void *lsmrule)
> > +{
> > +       call_void_hook(audit_rule_free, lsmrule);
> > +}
> > +
> > +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> > +{
> > +       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> > +}
> > +#endif /* CONFIG_IMA_LSM_RULES */
> > +
> >  #ifdef CONFIG_BPF_SYSCALL
> >  /**
> >   * security_bpf() - Check if the bpf syscall operation is allowed
> > --
> > 2.41.0

Mimi Zohar June 21, 2024, 8:23 p.m. UTC | #6

On Fri, 2024-06-21 at 15:07 -0400, Paul Moore wrote:
> On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <[e-mail]> wrote:
> > On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <[e-mail]> wrote:
> > > Create real functions for the ima_filter_rule interfaces.
> > > These replace #defines that obscure the reuse of audit
> > > interfaces. The new functions are put in security.c because
> > > they use security module registered hooks that we don't
> > > want exported.
> > > 
> > > Acked-by: Paul Moore <[e-mail]>
> > > Reviewed-by: John Johansen <[e-mail]>
> > > Signed-off-by: Casey Schaufler <[e-mail]>
> > > To: Mimi Zohar <[e-mail]>
> > > Cc: [e-mail]
> > > ---
> > >  include/linux/security.h     | 24 ++++++++++++++++++++++++
> > >  security/integrity/ima/ima.h | 26 --------------------------
> > >  security/security.c          | 21 +++++++++++++++++++++
> > >  3 files changed, 45 insertions(+), 26 deletions(-)
> > 
> > Mimi, Roberto, are you both okay if I merge this into the lsm/dev
> > branch?  The #define approach taken with the ima_filter_rule_XXX
> > macros likely contributed to the recent problem where the build
> > problem caused by the new gfp_t parameter was missed during review;
> > I'd like to get this into an upstream tree independent of the larger
> > stacking effort as I believe it has standalone value.
> 
> ... and I just realized neither Mimi or Roberto were directly CC'd on
> that last email, oops.  Fixed.

Paul, I do see things posted on the linux-integrity mailing list pretty quickly.
Unfortunately, something came up midday and I'm just seeing this now.  As for
Roberto, it's probably a time zone issue.

The patch looks ok, but I haven't had a chance to apply or test it.  I'll look
at it over the weekend and get back to you.

Mimi

> 
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 750130a7b9dd..4790508818ee 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
> > >  #endif /* CONFIG_SECURITY */
> > >  #endif /* CONFIG_AUDIT */
> > > 
> > > +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> > > +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> > > +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> > > +void ima_filter_rule_free(void *lsmrule);
> > > +
> > > +#else
> > > +
> > > +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> > > +                                          void **lsmrule)
> > > +{
> > > +       return 0;
> > > +}
> > > +
> > > +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> > > +                                           void *lsmrule)
> > > +{
> > > +       return 0;
> > > +}
> > > +
> > > +static inline void ima_filter_rule_free(void *lsmrule)
> > > +{ }
> > > +
> > > +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> > > +
> > >  #ifdef CONFIG_SECURITYFS
> > > 
> > >  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
> > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > > index c29db699c996..560d6104de72 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
> > >  }
> > >  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
> > > 
> > > -/* LSM based policy rules require audit */
> > > -#ifdef CONFIG_IMA_LSM_RULES
> > > -
> > > -#define ima_filter_rule_init security_audit_rule_init
> > > -#define ima_filter_rule_free security_audit_rule_free
> > > -#define ima_filter_rule_match security_audit_rule_match
> > > -
> > > -#else
> > > -
> > > -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> > > -                                      void **lsmrule)
> > > -{
> > > -       return -EINVAL;
> > > -}
> > > -
> > > -static inline void ima_filter_rule_free(void *lsmrule)
> > > -{
> > > -}
> > > -
> > > -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> > > -                                       void *lsmrule)
> > > -{
> > > -       return -EINVAL;
> > > -}
> > > -#endif /* CONFIG_IMA_LSM_RULES */
> > > -
> > >  #ifdef CONFIG_IMA_READ_POLICY
> > >  #define        POLICY_FILE_FLAGS       (S_IWUSR | S_IRUSR)
> > >  #else
> > > diff --git a/security/security.c b/security/security.c
> > > index d7b15ea67c3f..8e5379a76369 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> > >  }
> > >  #endif /* CONFIG_AUDIT */
> > > 
> > > +#ifdef CONFIG_IMA_LSM_RULES
> > > +/*
> > > + * The integrity subsystem uses the same hooks as
> > > + * the audit subsystem.
> > > + */
> > > +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> > > +{
> > > +       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> > > +}
> > > +
> > > +void ima_filter_rule_free(void *lsmrule)
> > > +{
> > > +       call_void_hook(audit_rule_free, lsmrule);
> > > +}
> > > +
> > > +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> > > +{
> > > +       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> > > +}
> > > +#endif /* CONFIG_IMA_LSM_RULES */
> > > +
> > >  #ifdef CONFIG_BPF_SYSCALL
> > >  /**
> > >   * security_bpf() - Check if the bpf syscall operation is allowed
> > > --
> > > 2.41.0

Roberto Sassu June 21, 2024, 8:34 p.m. UTC | #7

On 6/21/2024 10:23 PM, Mimi Zohar wrote:
> On Fri, 2024-06-21 at 15:07 -0400, Paul Moore wrote:
>> On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <[e-mail]> wrote:
>>> On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <[e-mail]> wrote:
>>>> Create real functions for the ima_filter_rule interfaces.
>>>> These replace #defines that obscure the reuse of audit
>>>> interfaces. The new functions are put in security.c because
>>>> they use security module registered hooks that we don't
>>>> want exported.
>>>>
>>>> Acked-by: Paul Moore <[e-mail]>
>>>> Reviewed-by: John Johansen <[e-mail]>
>>>> Signed-off-by: Casey Schaufler <[e-mail]>
>>>> To: Mimi Zohar <[e-mail]>
>>>> Cc: [e-mail]
>>>> ---
>>>>   include/linux/security.h     | 24 ++++++++++++++++++++++++
>>>>   security/integrity/ima/ima.h | 26 --------------------------
>>>>   security/security.c          | 21 +++++++++++++++++++++
>>>>   3 files changed, 45 insertions(+), 26 deletions(-)
>>>
>>> Mimi, Roberto, are you both okay if I merge this into the lsm/dev
>>> branch?  The #define approach taken with the ima_filter_rule_XXX
>>> macros likely contributed to the recent problem where the build
>>> problem caused by the new gfp_t parameter was missed during review;
>>> I'd like to get this into an upstream tree independent of the larger
>>> stacking effort as I believe it has standalone value.
>>
>> ... and I just realized neither Mimi or Roberto were directly CC'd on
>> that last email, oops.  Fixed.
> 
> Paul, I do see things posted on the linux-integrity mailing list pretty quickly.
> Unfortunately, something came up midday and I'm just seeing this now.  As for
> Roberto, it's probably a time zone issue.

Will review/check it first thing Monday morning.

Roberto

> The patch looks ok, but I haven't had a chance to apply or test it.  I'll look
> at it over the weekend and get back to you.
> 
> Mimi
> 
>>
>>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>>> index 750130a7b9dd..4790508818ee 100644
>>>> --- a/include/linux/security.h
>>>> +++ b/include/linux/security.h
>>>> @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
>>>>   #endif /* CONFIG_SECURITY */
>>>>   #endif /* CONFIG_AUDIT */
>>>>
>>>> +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
>>>> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
>>>> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
>>>> +void ima_filter_rule_free(void *lsmrule);
>>>> +
>>>> +#else
>>>> +
>>>> +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
>>>> +                                          void **lsmrule)
>>>> +{
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
>>>> +                                           void *lsmrule)
>>>> +{
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static inline void ima_filter_rule_free(void *lsmrule)
>>>> +{ }
>>>> +
>>>> +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
>>>> +
>>>>   #ifdef CONFIG_SECURITYFS
>>>>
>>>>   extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>> index c29db699c996..560d6104de72 100644
>>>> --- a/security/integrity/ima/ima.h
>>>> +++ b/security/integrity/ima/ima.h
>>>> @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
>>>>   }
>>>>   #endif /* CONFIG_IMA_APPRAISE_MODSIG */
>>>>
>>>> -/* LSM based policy rules require audit */
>>>> -#ifdef CONFIG_IMA_LSM_RULES
>>>> -
>>>> -#define ima_filter_rule_init security_audit_rule_init
>>>> -#define ima_filter_rule_free security_audit_rule_free
>>>> -#define ima_filter_rule_match security_audit_rule_match
>>>> -
>>>> -#else
>>>> -
>>>> -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
>>>> -                                      void **lsmrule)
>>>> -{
>>>> -       return -EINVAL;
>>>> -}
>>>> -
>>>> -static inline void ima_filter_rule_free(void *lsmrule)
>>>> -{
>>>> -}
>>>> -
>>>> -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
>>>> -                                       void *lsmrule)
>>>> -{
>>>> -       return -EINVAL;
>>>> -}
>>>> -#endif /* CONFIG_IMA_LSM_RULES */
>>>> -
>>>>   #ifdef CONFIG_IMA_READ_POLICY
>>>>   #define        POLICY_FILE_FLAGS       (S_IWUSR | S_IRUSR)
>>>>   #else
>>>> diff --git a/security/security.c b/security/security.c
>>>> index d7b15ea67c3f..8e5379a76369 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>>>   }
>>>>   #endif /* CONFIG_AUDIT */
>>>>
>>>> +#ifdef CONFIG_IMA_LSM_RULES
>>>> +/*
>>>> + * The integrity subsystem uses the same hooks as
>>>> + * the audit subsystem.
>>>> + */
>>>> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>>>> +{
>>>> +       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
>>>> +}
>>>> +
>>>> +void ima_filter_rule_free(void *lsmrule)
>>>> +{
>>>> +       call_void_hook(audit_rule_free, lsmrule);
>>>> +}
>>>> +
>>>> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>>> +{
>>>> +       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>>>> +}
>>>> +#endif /* CONFIG_IMA_LSM_RULES */
>>>> +
>>>>   #ifdef CONFIG_BPF_SYSCALL
>>>>   /**
>>>>    * security_bpf() - Check if the bpf syscall operation is allowed
>>>> --
>>>> 2.41.0
>

Paul Moore June 21, 2024, 9:18 p.m. UTC | #8

On Fri, Jun 21, 2024 at 4:27 PM Mimi Zohar <[e-mail]> wrote:
> On Fri, 2024-06-21 at 15:07 -0400, Paul Moore wrote:
> > On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <[e-mail]> wrote:
> > > On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <[e-mail]> wrote:
> > > > Create real functions for the ima_filter_rule interfaces.
> > > > These replace #defines that obscure the reuse of audit
> > > > interfaces. The new functions are put in security.c because
> > > > they use security module registered hooks that we don't
> > > > want exported.
> > > >
> > > > Acked-by: Paul Moore <[e-mail]>
> > > > Reviewed-by: John Johansen <[e-mail]>
> > > > Signed-off-by: Casey Schaufler <[e-mail]>
> > > > To: Mimi Zohar <[e-mail]>
> > > > Cc: [e-mail]
> > > > ---
> > > >  include/linux/security.h     | 24 ++++++++++++++++++++++++
> > > >  security/integrity/ima/ima.h | 26 --------------------------
> > > >  security/security.c          | 21 +++++++++++++++++++++
> > > >  3 files changed, 45 insertions(+), 26 deletions(-)
> > >
> > > Mimi, Roberto, are you both okay if I merge this into the lsm/dev
> > > branch?  The #define approach taken with the ima_filter_rule_XXX
> > > macros likely contributed to the recent problem where the build
> > > problem caused by the new gfp_t parameter was missed during review;
> > > I'd like to get this into an upstream tree independent of the larger
> > > stacking effort as I believe it has standalone value.
> >
> > ... and I just realized neither Mimi or Roberto were directly CC'd on
> > that last email, oops.  Fixed.
>
> Paul, I do see things posted on the linux-integrity mailing list pretty quickly.
> Unfortunately, something came up midday and I'm just seeing this now.  As for
> Roberto, it's probably a time zone issue.

Oh, no worries at all, please don't take my comment above to mean I
was expecting an immediate response!  I try to make sure that if I'm
addressing someone directly that they are explicitly included on the
To/CC line.  I was writing another email and it occurred to me that I
didn't check for that when emailing the two of you, and sure enough,
you guys weren't on the To/CC line ... I was just trying to fix my
screw-up :)

> The patch looks ok, but I haven't had a chance to apply or test it.  I'll look
> at it over the weekend and get back to you.

No rush, enjoy your weekend, the patch isn't going to run away :)

Paul Moore June 21, 2024, 9:19 p.m. UTC | #9

On Fri, Jun 21, 2024 at 4:34 PM Roberto Sassu
<[e-mail]> wrote:
> On 6/21/2024 10:23 PM, Mimi Zohar wrote:
> > On Fri, 2024-06-21 at 15:07 -0400, Paul Moore wrote:
> >> On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <[e-mail]> wrote:
> >>> On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <[e-mail]> wrote:
> >>>> Create real functions for the ima_filter_rule interfaces.
> >>>> These replace #defines that obscure the reuse of audit
> >>>> interfaces. The new functions are put in security.c because
> >>>> they use security module registered hooks that we don't
> >>>> want exported.
> >>>>
> >>>> Acked-by: Paul Moore <[e-mail]>
> >>>> Reviewed-by: John Johansen <[e-mail]>
> >>>> Signed-off-by: Casey Schaufler <[e-mail]>
> >>>> To: Mimi Zohar <[e-mail]>
> >>>> Cc: [e-mail]
> >>>> ---
> >>>>   include/linux/security.h     | 24 ++++++++++++++++++++++++
> >>>>   security/integrity/ima/ima.h | 26 --------------------------
> >>>>   security/security.c          | 21 +++++++++++++++++++++
> >>>>   3 files changed, 45 insertions(+), 26 deletions(-)
> >>>
> >>> Mimi, Roberto, are you both okay if I merge this into the lsm/dev
> >>> branch?  The #define approach taken with the ima_filter_rule_XXX
> >>> macros likely contributed to the recent problem where the build
> >>> problem caused by the new gfp_t parameter was missed during review;
> >>> I'd like to get this into an upstream tree independent of the larger
> >>> stacking effort as I believe it has standalone value.
> >>
> >> ... and I just realized neither Mimi or Roberto were directly CC'd on
> >> that last email, oops.  Fixed.
> >
> > Paul, I do see things posted on the linux-integrity mailing list pretty quickly.
> > Unfortunately, something came up midday and I'm just seeing this now.  As for
> > Roberto, it's probably a time zone issue.
>
> Will review/check it first thing Monday morning.

Thanks Roberto, no rush.

Roberto Sassu June 24, 2024, 8:45 a.m. UTC | #10

On Fri, 2024-06-21 at 17:19 -0400, Paul Moore wrote:
> On Fri, Jun 21, 2024 at 4:34 PM Roberto Sassu
> <[e-mail]> wrote:
> > On 6/21/2024 10:23 PM, Mimi Zohar wrote:
> > > On Fri, 2024-06-21 at 15:07 -0400, Paul Moore wrote:
> > > > On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <[e-mail]> wrote:
> > > > > On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <[e-mail]> wrote:
> > > > > > Create real functions for the ima_filter_rule interfaces.
> > > > > > These replace #defines that obscure the reuse of audit
> > > > > > interfaces. The new functions are put in security.c because
> > > > > > they use security module registered hooks that we don't
> > > > > > want exported.
> > > > > > 
> > > > > > Acked-by: Paul Moore <[e-mail]>
> > > > > > Reviewed-by: John Johansen <[e-mail]>
> > > > > > Signed-off-by: Casey Schaufler <[e-mail]>
> > > > > > To: Mimi Zohar <[e-mail]>
> > > > > > Cc: [e-mail]
> > > > > > ---
> > > > > >   include/linux/security.h     | 24 ++++++++++++++++++++++++
> > > > > >   security/integrity/ima/ima.h | 26 --------------------------
> > > > > >   security/security.c          | 21 +++++++++++++++++++++
> > > > > >   3 files changed, 45 insertions(+), 26 deletions(-)
> > > > > 
> > > > > Mimi, Roberto, are you both okay if I merge this into the lsm/dev
> > > > > branch?  The #define approach taken with the ima_filter_rule_XXX
> > > > > macros likely contributed to the recent problem where the build
> > > > > problem caused by the new gfp_t parameter was missed during review;
> > > > > I'd like to get this into an upstream tree independent of the larger
> > > > > stacking effort as I believe it has standalone value.
> > > > 
> > > > ... and I just realized neither Mimi or Roberto were directly CC'd on
> > > > that last email, oops.  Fixed.
> > > 
> > > Paul, I do see things posted on the linux-integrity mailing list pretty quickly.
> > > Unfortunately, something came up midday and I'm just seeing this now.  As for
> > > Roberto, it's probably a time zone issue.
> > 
> > Will review/check it first thing Monday morning.
> 
> Thanks Roberto, no rush.

Ok, so no problem from my side to upstream the patch.

My only comment would be that I would not call the new functions with
the ima_ prefix, being those in security.c, which is LSM agnostic, but
I would rather use a name that more resembles the differences, if any.

If not:

Acked-by: Roberto Sassu <[e-mail]>

Thanks

Roberto

Mimi Zohar June 24, 2024, 1:57 p.m. UTC | #11

On Mon, 2024-06-24 at 10:45 +0200, Roberto Sassu wrote:
> My only comment would be that I would not call the new functions with
> the ima_ prefix, being those in security.c, which is LSM agnostic, but
> I would rather use a name that more resembles the differences, if any.

Commit 4af4662fa4a9 ("integrity: IMA policy") originally referred to these hooks
as security_filter_rule_XXXX, but commit b8867eedcf76 ("ima: Rename internal
filter rule functions") renamed the function to ima_filter_rule_XXX) to avoid
security namespace polution.

If these were regular security hooks, the hooks would be named:
filter_rule_init, filter_rule_free, filter_rule_match with the matching
"security" prefix functions. Audit and IMA would then register the hooks.

I agree these functions should probably be renamed again, probably to
security_ima_filter_rule_XXXX.

Mimi

Mimi Zohar June 24, 2024, 1:58 p.m. UTC | #12

On Fri, 2023-12-15 at 14:15 -0800, Casey Schaufler wrote:
> Create real functions for the ima_filter_rule interfaces.
> These replace #defines that obscure the reuse of audit
> interfaces. The new functions are put in security.c because
> they use security module registered hooks that we don't
> want exported.
> 
> Acked-by: Paul Moore <[e-mail]>
> Reviewed-by: John Johansen <[e-mail]>
> Signed-off-by: Casey Schaufler <[e-mail]>
> To: Mimi Zohar <[e-mail]>
> Cc: [e-mail]
> ---
>  include/linux/security.h     | 24 ++++++++++++++++++++++++
>  security/integrity/ima/ima.h | 26 --------------------------
>  security/security.c          | 21 +++++++++++++++++++++
>  3 files changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 750130a7b9dd..4790508818ee 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2009,6 +2009,30 @@ static inline void security_audit_rule_free(void *lsmrule)
>  #endif /* CONFIG_SECURITY */
>  #endif /* CONFIG_AUDIT */
>  
> +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> +void ima_filter_rule_free(void *lsmrule);
> +
> +#else
> +
> +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> +					   void **lsmrule)
> +{
> +	return 0;
> +}
> +
> +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> +					    void *lsmrule)
> +{
> +	return 0;
> +}
> +

scripts/checkpatch.pl complains about the formatting of "void *lsmrule".


> +static inline void ima_filter_rule_free(void *lsmrule)
> +{ }
> +
> +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> +
>  #ifdef CONFIG_SECURITYFS
>  
>  extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..560d6104de72 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -420,32 +420,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
>  }
>  #endif /* CONFIG_IMA_APPRAISE_MODSIG */
>  
> -/* LSM based policy rules require audit */
> -#ifdef CONFIG_IMA_LSM_RULES
> -
> -#define ima_filter_rule_init security_audit_rule_init
> -#define ima_filter_rule_free security_audit_rule_free
> -#define ima_filter_rule_match security_audit_rule_match
> -
> -#else
> -
> -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> -				       void **lsmrule)
> -{
> -	return -EINVAL;
> -}
> -
> -static inline void ima_filter_rule_free(void *lsmrule)
> -{
> -}
> -
> -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> -					void *lsmrule)
> -{
> -	return -EINVAL;
> -}
> -#endif /* CONFIG_IMA_LSM_RULES */
> -
>  #ifdef	CONFIG_IMA_READ_POLICY
>  #define	POLICY_FILE_FLAGS	(S_IWUSR | S_IRUSR)
>  #else
> diff --git a/security/security.c b/security/security.c
> index d7b15ea67c3f..8e5379a76369 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -5350,6 +5350,27 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>  }
>  #endif /* CONFIG_AUDIT */
>  
> +#ifdef CONFIG_IMA_LSM_RULES
> +/*
> + * The integrity subsystem uses the same hooks as
> + * the audit subsystem.

- No reason for the comment to be split.
- Please note that these calls are limited to IMA, not the integrity subsystem
in general.

> + */
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> +{
> +	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> +}
> +
> +void ima_filter_rule_free(void *lsmrule)
> +{
> +	call_void_hook(audit_rule_free, lsmrule);
> +}
> +
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +{
> +	return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> +}
> +#endif /* CONFIG_IMA_LSM_RULES */

In terms of function naming, refer to thread with Roberto's response.

> +
>  #ifdef CONFIG_BPF_SYSCALL
>  /**
>   * security_bpf() - Check if the bpf syscall operation is allowed

Paul Moore June 24, 2024, 10:03 p.m. UTC | #13

On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <[e-mail]> wrote:
> On Mon, 2024-06-24 at 10:45 +0200, Roberto Sassu wrote:
> > My only comment would be that I would not call the new functions with
> > the ima_ prefix, being those in security.c, which is LSM agnostic, but
> > I would rather use a name that more resembles the differences, if any.
>
> Commit 4af4662fa4a9 ("integrity: IMA policy") originally referred to these hooks
> as security_filter_rule_XXXX, but commit b8867eedcf76 ("ima: Rename internal
> filter rule functions") renamed the function to ima_filter_rule_XXX) to avoid
> security namespace polution.
>
> If these were regular security hooks, the hooks would be named:
> filter_rule_init, filter_rule_free, filter_rule_match with the matching
> "security" prefix functions. Audit and IMA would then register the hooks.
>
> I agree these functions should probably be renamed again, probably to
> security_ima_filter_rule_XXXX.

It's funny, my mind saw that the patch was removing those preprocessor
macros and was so happy it must have shut off, because we already have
security_XXX functions for these :)

See security_audit_rule_init(), security_audit_rule_free(), and
security_audit_rule_match().

Casey, do you want to respin this patch to use the existing LSM
functions?  It looks like you should have Mimi's and Roberto's support
in this.  Please submit this as a standalone patch as it really is a
IMA/LSM cleanup.

Thanks all.

Casey Schaufler June 24, 2024, 10:19 p.m. UTC | #14

On 6/24/2024 3:03 PM, Paul Moore wrote:
> On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <[e-mail]> wrote:
>> On Mon, 2024-06-24 at 10:45 +0200, Roberto Sassu wrote:
>>> My only comment would be that I would not call the new functions with
>>> the ima_ prefix, being those in security.c, which is LSM agnostic, but
>>> I would rather use a name that more resembles the differences, if any.
>> Commit 4af4662fa4a9 ("integrity: IMA policy") originally referred to these hooks
>> as security_filter_rule_XXXX, but commit b8867eedcf76 ("ima: Rename internal
>> filter rule functions") renamed the function to ima_filter_rule_XXX) to avoid
>> security namespace polution.
>>
>> If these were regular security hooks, the hooks would be named:
>> filter_rule_init, filter_rule_free, filter_rule_match with the matching
>> "security" prefix functions. Audit and IMA would then register the hooks.
>>
>> I agree these functions should probably be renamed again, probably to
>> security_ima_filter_rule_XXXX.
> It's funny, my mind saw that the patch was removing those preprocessor
> macros and was so happy it must have shut off, because we already have
> security_XXX functions for these :)
>
> See security_audit_rule_init(), security_audit_rule_free(), and
> security_audit_rule_match().
>
> Casey, do you want to respin this patch to use the existing LSM
> functions?

If you want to use shared functions they shouldn't be security_audit_blah().
I like Mimi's suggestion. Rename security_audit_filter_rule_init() to
security_filter_rule_init() and use that in both places.

>   It looks like you should have Mimi's and Roberto's support
> in this.  Please submit this as a standalone patch as it really is a
> IMA/LSM cleanup.
>
> Thanks all.
>

Paul Moore June 24, 2024, 11:05 p.m. UTC | #15

On Mon, Jun 24, 2024 at 6:19 PM Casey Schaufler <[e-mail]> wrote:
> On 6/24/2024 3:03 PM, Paul Moore wrote:
> > On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <[e-mail]> wrote:
> >> On Mon, 2024-06-24 at 10:45 +0200, Roberto Sassu wrote:
> >>> My only comment would be that I would not call the new functions with
> >>> the ima_ prefix, being those in security.c, which is LSM agnostic, but
> >>> I would rather use a name that more resembles the differences, if any.
> >> Commit 4af4662fa4a9 ("integrity: IMA policy") originally referred to these hooks
> >> as security_filter_rule_XXXX, but commit b8867eedcf76 ("ima: Rename internal
> >> filter rule functions") renamed the function to ima_filter_rule_XXX) to avoid
> >> security namespace polution.
> >>
> >> If these were regular security hooks, the hooks would be named:
> >> filter_rule_init, filter_rule_free, filter_rule_match with the matching
> >> "security" prefix functions. Audit and IMA would then register the hooks.
> >>
> >> I agree these functions should probably be renamed again, probably to
> >> security_ima_filter_rule_XXXX.
> > It's funny, my mind saw that the patch was removing those preprocessor
> > macros and was so happy it must have shut off, because we already have
> > security_XXX functions for these :)
> >
> > See security_audit_rule_init(), security_audit_rule_free(), and
> > security_audit_rule_match().
> >
> > Casey, do you want to respin this patch to use the existing LSM
> > functions?
>
> If you want to use shared functions they shouldn't be security_audit_blah().
> I like Mimi's suggestion. Rename security_audit_filter_rule_init() to
> security_filter_rule_init() and use that in both places.

They are currently shared, the preprocessor macros just hide that fact
(which is not a good thing, IMO).  Renaming the existing LSM functions
to drop the "audit" in the name doesn't really solve the problem as
you still end up with "Audit_equal", etc. constants (which are awful
for multiple reasons, some having nothing to do with the LSM) in the
callers.

... and let me just get ahead of this, please do not do a macro-based
rename of "Audit_equal" to something else to "fix" that problem;
that's just as bad as what we have now.

Properly fixing this may be worthwhile, but I think it's an
unnecessary distraction at this point from improving that state of
multiple LSMs.  If you aren't comfortable submitting a patch that just
does a "/ima_filter_rule_match/security_audit_rule_match/" style
rename, or if Mimi and Roberto aren't supportive of that, you might as
well just drop this from the patchset.

--
paul-moore.com

Mimi Zohar June 24, 2024, 11:06 p.m. UTC | #16

On Mon, 2024-06-24 at 15:19 -0700, Casey Schaufler wrote:
> On 6/24/2024 3:03 PM, Paul Moore wrote:
> > On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <[e-mail]> wrote:
> > > On Mon, 2024-06-24 at 10:45 +0200, Roberto Sassu wrote:
> > > > My only comment would be that I would not call the new functions with
> > > > the ima_ prefix, being those in security.c, which is LSM agnostic, but
> > > > I would rather use a name that more resembles the differences, if any.
> > > Commit 4af4662fa4a9 ("integrity: IMA policy") originally referred to these hooks
> > > as security_filter_rule_XXXX, but commit b8867eedcf76 ("ima: Rename internal
> > > filter rule functions") renamed the function to ima_filter_rule_XXX) to avoid
> > > security namespace polution.
> > > 
> > > If these were regular security hooks, the hooks would be named:
> > > filter_rule_init, filter_rule_free, filter_rule_match with the matching
> > > "security" prefix functions. Audit and IMA would then register the hooks.
> > > 
> > > I agree these functions should probably be renamed again, probably to
> > > security_ima_filter_rule_XXXX.
> > It's funny, my mind saw that the patch was removing those preprocessor
> > macros and was so happy it must have shut off, because we already have
> > security_XXX functions for these :)
> > 
> > See security_audit_rule_init(), security_audit_rule_free(), and
> > security_audit_rule_match().
> > 
> > Casey, do you want to respin this patch to use the existing LSM
> > functions?
> 
> If you want to use shared functions they shouldn't be security_audit_blah().
> I like Mimi's suggestion. Rename security_audit_filter_rule_init() to
> security_filter_rule_init() and use that in both places.

The existing name is security_audit_rule_init().  Replacing '_audit_' with
'_filter_' would definitely resolve the naming issue.  Each of the LSMs would
need to be converted as well (e.g. smack_audit_rule_init,
selinux_audit_rule_init, aa_audit_rule_init).

> 
> >   It looks like you should have Mimi's and Roberto's support
> > in this.  Please submit this as a standalone patch as it really is a
> > IMA/LSM cleanup.
> > 
> > Thanks all.
> >

Casey Schaufler June 24, 2024, 11:16 p.m. UTC | #17

On 6/24/2024 4:05 PM, Paul Moore wrote:
> On Mon, Jun 24, 2024 at 6:19 PM Casey Schaufler <[e-mail]> wrote:
>> On 6/24/2024 3:03 PM, Paul Moore wrote:
>>> On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <[e-mail]> wrote:
>>>> On Mon, 2024-06-24 at 10:45 +0200, Roberto Sassu wrote:
>>>>> My only comment would be that I would not call the new functions with
>>>>> the ima_ prefix, being those in security.c, which is LSM agnostic, but
>>>>> I would rather use a name that more resembles the differences, if any.
>>>> Commit 4af4662fa4a9 ("integrity: IMA policy") originally referred to these hooks
>>>> as security_filter_rule_XXXX, but commit b8867eedcf76 ("ima: Rename internal
>>>> filter rule functions") renamed the function to ima_filter_rule_XXX) to avoid
>>>> security namespace polution.
>>>>
>>>> If these were regular security hooks, the hooks would be named:
>>>> filter_rule_init, filter_rule_free, filter_rule_match with the matching
>>>> "security" prefix functions. Audit and IMA would then register the hooks.
>>>>
>>>> I agree these functions should probably be renamed again, probably to
>>>> security_ima_filter_rule_XXXX.
>>> It's funny, my mind saw that the patch was removing those preprocessor
>>> macros and was so happy it must have shut off, because we already have
>>> security_XXX functions for these :)
>>>
>>> See security_audit_rule_init(), security_audit_rule_free(), and
>>> security_audit_rule_match().
>>>
>>> Casey, do you want to respin this patch to use the existing LSM
>>> functions?
>> If you want to use shared functions they shouldn't be security_audit_blah().
>> I like Mimi's suggestion. Rename security_audit_filter_rule_init() to
>> security_filter_rule_init() and use that in both places.
> They are currently shared, the preprocessor macros just hide that fact
> (which is not a good thing, IMO).  Renaming the existing LSM functions
> to drop the "audit" in the name doesn't really solve the problem as
> you still end up with "Audit_equal", etc. constants (which are awful
> for multiple reasons, some having nothing to do with the LSM) in the
> callers.
>
> .. and let me just get ahead of this, please do not do a macro-based
> rename of "Audit_equal" to something else to "fix" that problem;
> that's just as bad as what we have now.

Agreed.

> Properly fixing this may be worthwhile, but I think it's an
> unnecessary distraction at this point from improving that state of
> multiple LSMs.  If you aren't comfortable submitting a patch that just
> does a "/ima_filter_rule_match/security_audit_rule_match/" style
> rename, or if Mimi and Roberto aren't supportive of that, you might as
> well just drop this from the patchset.

There was a time (long ago by now) when the stacking patches really needed
the functions to be different. They don't now. I'd be perfectly happy with
dropping this patch from the set.

>
> --
> paul-moore.com
>

Patch

diff --git a/include/linux/security.h b/include/linux/security.h
index 750130a7b9dd..4790508818ee 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2009,6 +2009,30 @@  static inline void security_audit_rule_free(void *lsmrule)
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_AUDIT */
 
+#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
+int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
+int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
+void ima_filter_rule_free(void *lsmrule);
+
+#else
+
+static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
+					   void **lsmrule)
+{
+	return 0;
+}
+
+static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
+					    void *lsmrule)
+{
+	return 0;
+}
+
+static inline void ima_filter_rule_free(void *lsmrule)
+{ }
+
+#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
+
 #ifdef CONFIG_SECURITYFS
 
 extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c29db699c996..560d6104de72 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -420,32 +420,6 @@  static inline void ima_free_modsig(struct modsig *modsig)
 }
 #endif /* CONFIG_IMA_APPRAISE_MODSIG */
 
-/* LSM based policy rules require audit */
-#ifdef CONFIG_IMA_LSM_RULES
-
-#define ima_filter_rule_init security_audit_rule_init
-#define ima_filter_rule_free security_audit_rule_free
-#define ima_filter_rule_match security_audit_rule_match
-
-#else
-
-static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
-				       void **lsmrule)
-{
-	return -EINVAL;
-}
-
-static inline void ima_filter_rule_free(void *lsmrule)
-{
-}
-
-static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
-					void *lsmrule)
-{
-	return -EINVAL;
-}
-#endif /* CONFIG_IMA_LSM_RULES */
-
 #ifdef	CONFIG_IMA_READ_POLICY
 #define	POLICY_FILE_FLAGS	(S_IWUSR | S_IRUSR)
 #else
diff --git a/security/security.c b/security/security.c
index d7b15ea67c3f..8e5379a76369 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5350,6 +5350,27 @@  int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
 }
 #endif /* CONFIG_AUDIT */
 
+#ifdef CONFIG_IMA_LSM_RULES
+/*
+ * The integrity subsystem uses the same hooks as
+ * the audit subsystem.
+ */
+int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
+{
+	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
+}
+
+void ima_filter_rule_free(void *lsmrule)
+{
+	call_void_hook(audit_rule_free, lsmrule);
+}
+
+int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
+{
+	return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
+}
+#endif /* CONFIG_IMA_LSM_RULES */
+
 #ifdef CONFIG_BPF_SYSCALL
 /**
  * security_bpf() - Check if the bpf syscall operation is allowed