| Message ID | 20200623003236.830149-2-tyhicks@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support | expand |
On 6/22/2020 5:32 PM, Tyler Hicks wrote: > Ask the LSM to free its audit rule rather than directly calling kfree(). > Both AppArmor and SELinux do additional work in their audit_rule_free() > hooks. Fix memory leaks by allowing the LSMs to perform necessary work. > > Fixes: b16942455193 ("ima: use the lsm policy update notifier") > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > Cc: Janne Karhunen <janne.karhunen@gmail.com> > --- > security/integrity/ima/ima.h | 6 ++++++ > security/integrity/ima/ima_policy.c | 2 +- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index df93ac258e01..de05d7f1d3ec 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -404,6 +404,7 @@ static inline void ima_free_modsig(struct modsig *modsig) > #ifdef CONFIG_IMA_LSM_RULES > > #define security_filter_rule_init security_audit_rule_init > +#define security_filter_rule_free security_audit_rule_free > #define security_filter_rule_match security_audit_rule_match In context this seems perfectly reasonable. If, however, you're working with the LSM infrastructure this set of #defines is maddening. The existing ones have been driving my nuts for the past few years, so I'd like to discourage adding another. Since the security_filter_rule functions are IMA specific they shouldn't be prefixed security_. I know that it seems to be code churn/bikesheading, but we please change these: static inline int ima_filter_rule_init(.....) { return security_audit_rule_init(.....); } and so forth. I understand if you don't want to make the change. I have plenty of other things driving me crazy just now, so this doesn't seem likely to push me over the edge. > > #else > @@ -414,6 +415,11 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr, > return -EINVAL; > } > > +static inline void security_filter_rule_free(void *lsmrule) > +{ > + return -EINVAL; > +} > + > static inline int security_filter_rule_match(u32 secid, u32 field, u32 op, > void *lsmrule) > { > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index e493063a3c34..236a731492d1 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry) > int i; > > for (i = 0; i < MAX_LSM_RULES; i++) { > - kfree(entry->lsm[i].rule); > + security_filter_rule_free(entry->lsm[i].rule); > kfree(entry->lsm[i].args_p); > } > kfree(entry);
On 2020-06-22 17:55:59, Casey Schaufler wrote: > On 6/22/2020 5:32 PM, Tyler Hicks wrote: > > Ask the LSM to free its audit rule rather than directly calling kfree(). > > Both AppArmor and SELinux do additional work in their audit_rule_free() > > hooks. Fix memory leaks by allowing the LSMs to perform necessary work. > > > > Fixes: b16942455193 ("ima: use the lsm policy update notifier") > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > Cc: Janne Karhunen <janne.karhunen@gmail.com> > > --- > > security/integrity/ima/ima.h | 6 ++++++ > > security/integrity/ima/ima_policy.c | 2 +- > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > > index df93ac258e01..de05d7f1d3ec 100644 > > --- a/security/integrity/ima/ima.h > > +++ b/security/integrity/ima/ima.h > > @@ -404,6 +404,7 @@ static inline void ima_free_modsig(struct modsig *modsig) > > #ifdef CONFIG_IMA_LSM_RULES > > > > #define security_filter_rule_init security_audit_rule_init > > +#define security_filter_rule_free security_audit_rule_free > > #define security_filter_rule_match security_audit_rule_match > > In context this seems perfectly reasonable. If, however, you're > working with the LSM infrastructure this set of #defines is maddening. > The existing ones have been driving my nuts for the past few years, > so I'd like to discourage adding another. Since the security_filter_rule > functions are IMA specific they shouldn't be prefixed security_. I know > that it seems to be code churn/bikesheading, but we please change these: > > static inline int ima_filter_rule_init(.....) > { > return security_audit_rule_init(.....); > } > > and so forth. I understand if you don't want to make the change. > I have plenty of other things driving me crazy just now, so this > doesn't seem likely to push me over the edge. I'd be happy to take a stab at that as a follow-up or a 13/12 patch. I'd like to leave this one as-is for stable kernel reasons since it is straightforward and simple. Tyler > > > > > #else > > @@ -414,6 +415,11 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr, > > return -EINVAL; > > } > > > > +static inline void security_filter_rule_free(void *lsmrule) > > +{ > > + return -EINVAL; > > +} > > + > > static inline int security_filter_rule_match(u32 secid, u32 field, u32 op, > > void *lsmrule) > > { > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > index e493063a3c34..236a731492d1 100644 > > --- a/security/integrity/ima/ima_policy.c > > +++ b/security/integrity/ima/ima_policy.c > > @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry) > > int i; > > > > for (i = 0; i < MAX_LSM_RULES; i++) { > > - kfree(entry->lsm[i].rule); > > + security_filter_rule_free(entry->lsm[i].rule); > > kfree(entry->lsm[i].args_p); > > } > > kfree(entry);
On 2020-06-22 19:32:25, Tyler Hicks wrote: > Ask the LSM to free its audit rule rather than directly calling kfree(). > Both AppArmor and SELinux do additional work in their audit_rule_free() > hooks. Fix memory leaks by allowing the LSMs to perform necessary work. > > Fixes: b16942455193 ("ima: use the lsm policy update notifier") > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > Cc: Janne Karhunen <janne.karhunen@gmail.com> > --- > security/integrity/ima/ima.h | 6 ++++++ > security/integrity/ima/ima_policy.c | 2 +- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index df93ac258e01..de05d7f1d3ec 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -404,6 +404,7 @@ static inline void ima_free_modsig(struct modsig *modsig) > #ifdef CONFIG_IMA_LSM_RULES > > #define security_filter_rule_init security_audit_rule_init > +#define security_filter_rule_free security_audit_rule_free > #define security_filter_rule_match security_audit_rule_match > > #else > @@ -414,6 +415,11 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr, > return -EINVAL; > } > > +static inline void security_filter_rule_free(void *lsmrule) > +{ > + return -EINVAL; Bah, I introduced a build warning here when CONFIG_IMA_LSM_RULES is disabled. This function should return nothing. I'll wait for additional feedback before spinning a v2. Tyler > +} > + > static inline int security_filter_rule_match(u32 secid, u32 field, u32 op, > void *lsmrule) > { > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index e493063a3c34..236a731492d1 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry) > int i; > > for (i = 0; i < MAX_LSM_RULES; i++) { > - kfree(entry->lsm[i].rule); > + security_filter_rule_free(entry->lsm[i].rule); > kfree(entry->lsm[i].args_p); > } > kfree(entry); > -- > 2.25.1
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote: > Ask the LSM to free its audit rule rather than directly calling kfree(). > Both AppArmor and SELinux do additional work in their audit_rule_free() > hooks. Fix memory leaks by allowing the LSMs to perform necessary work. > > Fixes: b16942455193 ("ima: use the lsm policy update notifier") > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > Cc: Janne Karhunen <janne.karhunen@gmail.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index df93ac258e01..de05d7f1d3ec 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -404,6 +404,7 @@ static inline void ima_free_modsig(struct modsig *modsig) #ifdef CONFIG_IMA_LSM_RULES #define security_filter_rule_init security_audit_rule_init +#define security_filter_rule_free security_audit_rule_free #define security_filter_rule_match security_audit_rule_match #else @@ -414,6 +415,11 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr, return -EINVAL; } +static inline void security_filter_rule_free(void *lsmrule) +{ + return -EINVAL; +} + static inline int security_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule) { diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index e493063a3c34..236a731492d1 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry) int i; for (i = 0; i < MAX_LSM_RULES; i++) { - kfree(entry->lsm[i].rule); + security_filter_rule_free(entry->lsm[i].rule); kfree(entry->lsm[i].args_p); } kfree(entry);
Ask the LSM to free its audit rule rather than directly calling kfree(). Both AppArmor and SELinux do additional work in their audit_rule_free() hooks. Fix memory leaks by allowing the LSMs to perform necessary work. Fixes: b16942455193 ("ima: use the lsm policy update notifier") Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> Cc: Janne Karhunen <janne.karhunen@gmail.com> --- security/integrity/ima/ima.h | 6 ++++++ security/integrity/ima/ima_policy.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)