| Message ID | 20231215221636.105680-2-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v39,01/42] integrity: disassociate ima_filter_rule from security_audit_rule | expand |
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 <paul@paul-moore.com> > Reviewed-by: John Johansen <john.johansen@canonical.com> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > To: Mimi Zohar <zohar@linux.ibm.com> > Cc: linux-integrity@vger.kernel.org > --- > 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
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 <paul@paul-moore.com> >> Reviewed-by: John Johansen <john.johansen@canonical.com> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> To: Mimi Zohar <zohar@linux.ibm.com> >> Cc: linux-integrity@vger.kernel.org >> --- >> 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
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 <paul@paul-moore.com> > > > Reviewed-by: John Johansen <john.johansen@canonical.com> > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > > To: Mimi Zohar <zohar@linux.ibm.com> > > > Cc: linux-integrity@vger.kernel.org > > > --- > > > 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
On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <casey@schaufler-ca.com> 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 <paul@paul-moore.com> > Reviewed-by: John Johansen <john.johansen@canonical.com> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > To: Mimi Zohar <zohar@linux.ibm.com> > Cc: linux-integrity@vger.kernel.org > --- > 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 >
On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <paul@paul-moore.com> wrote: > On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <casey@schaufler-ca.com> 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 <paul@paul-moore.com> > > Reviewed-by: John Johansen <john.johansen@canonical.com> > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > To: Mimi Zohar <zohar@linux.ibm.com> > > Cc: linux-integrity@vger.kernel.org > > --- > > 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
On Fri, 2024-06-21 at 15:07 -0400, Paul Moore wrote: > On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <casey@schaufler-ca.com> 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 <paul@paul-moore.com> > > > Reviewed-by: John Johansen <john.johansen@canonical.com> > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > > To: Mimi Zohar <zohar@linux.ibm.com> > > > Cc: linux-integrity@vger.kernel.org > > > --- > > > 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
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 <paul@paul-moore.com> wrote: >>> On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <casey@schaufler-ca.com> 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 <paul@paul-moore.com> >>>> Reviewed-by: John Johansen <john.johansen@canonical.com> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> To: Mimi Zohar <zohar@linux.ibm.com> >>>> Cc: linux-integrity@vger.kernel.org >>>> --- >>>> 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 >
On Fri, Jun 21, 2024 at 4:27 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > On Fri, 2024-06-21 at 15:07 -0400, Paul Moore wrote: > > On Fri, Jun 21, 2024 at 12:50 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <casey@schaufler-ca.com> 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 <paul@paul-moore.com> > > > > Reviewed-by: John Johansen <john.johansen@canonical.com> > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > > > To: Mimi Zohar <zohar@linux.ibm.com> > > > > Cc: linux-integrity@vger.kernel.org > > > > --- > > > > 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 :)
On Fri, Jun 21, 2024 at 4:34 PM Roberto Sassu <roberto.sassu@huaweicloud.com> 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 <paul@paul-moore.com> wrote: > >>> On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <casey@schaufler-ca.com> 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 <paul@paul-moore.com> > >>>> Reviewed-by: John Johansen <john.johansen@canonical.com> > >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >>>> To: Mimi Zohar <zohar@linux.ibm.com> > >>>> Cc: linux-integrity@vger.kernel.org > >>>> --- > >>>> 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.
On Fri, 2024-06-21 at 17:19 -0400, Paul Moore wrote: > On Fri, Jun 21, 2024 at 4:34 PM Roberto Sassu > <roberto.sassu@huaweicloud.com> 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 <paul@paul-moore.com> wrote: > > > > > On Fri, Dec 15, 2023 at 5:16 PM Casey Schaufler <casey@schaufler-ca.com> 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 <paul@paul-moore.com> > > > > > > Reviewed-by: John Johansen <john.johansen@canonical.com> > > > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > > > > > To: Mimi Zohar <zohar@linux.ibm.com> > > > > > > Cc: linux-integrity@vger.kernel.org > > > > > > --- > > > > > > 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 <roberto.sassu@huawei.com> Thanks Roberto
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
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 <paul@paul-moore.com> > Reviewed-by: John Johansen <john.johansen@canonical.com> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > To: Mimi Zohar <zohar@linux.ibm.com> > Cc: linux-integrity@vger.kernel.org > --- > 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
On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <zohar@linux.ibm.com> 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.
On 6/24/2024 3:03 PM, Paul Moore wrote: > On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <zohar@linux.ibm.com> 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. >
On Mon, Jun 24, 2024 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 6/24/2024 3:03 PM, Paul Moore wrote: > > On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <zohar@linux.ibm.com> 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
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 <zohar@linux.ibm.com> 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. > >
On 6/24/2024 4:05 PM, Paul Moore wrote: > On Mon, Jun 24, 2024 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 6/24/2024 3:03 PM, Paul Moore wrote: >>> On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <zohar@linux.ibm.com> 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 >
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