[v3,01/12] ima: Have the LSM free its audit rule
diff mbox series

Message ID 20200709061911.954326-2-tyhicks@linux.microsoft.com
State New
Headers show
Series
  • ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
Related show

Commit Message

Tyler Hicks July 9, 2020, 6:19 a.m. UTC
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>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---

* v3
  - No change
* v2
  - Fixed build warning by dropping the 'return -EINVAL' from
    the stubbed out security_filter_rule_free() since it has a void
    return type
  - Added Mimi's Reviewed-by
  - Developed a follow-on patch to rename security_filter_rule_*()
    functions, to address Casey's request, but I'll submit it
    independently of this patch series since it is somewhat unrelated

 security/integrity/ima/ima.h        | 5 +++++
 security/integrity/ima/ima_policy.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Nayna July 17, 2020, 7:20 p.m. UTC | #1
On 7/9/20 2:19 AM, Tyler Hicks wrote:
> Ask the LSM to free its audit rule rather than directly calling kfree().

Is it to be called audit rule or filter rule ?  Likewise in subject line.

Thanks & Regards,

     - Nayna
Tyler Hicks July 17, 2020, 7:24 p.m. UTC | #2
On 2020-07-17 15:20:22, Nayna wrote:
> 
> On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > Ask the LSM to free its audit rule rather than directly calling kfree().
> 
> Is it to be called audit rule or filter rule ?  Likewise in subject line.

The security hooks call this "audit rule" but Mimi explained the
reasoning for IMA referring to this as an "audit filter" here:

 https://lore.kernel.org/lkml/1593466203.5085.62.camel@linux.ibm.com/

I would be fine with her renaming/rewording this patch, accordingly, in
next-integrity-testing.

Tyler

> 
> Thanks & Regards,
> 
>     - Nayna
Mimi Zohar July 19, 2020, 11:02 a.m. UTC | #3
On Fri, 2020-07-17 at 14:24 -0500, Tyler Hicks wrote:
> On 2020-07-17 15:20:22, Nayna wrote:
> > 
> > On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > > Ask the LSM to free its audit rule rather than directly calling kfree().
> > 
> > Is it to be called audit rule or filter rule ?  Likewise in subject line.
> gt
> The security hooks call this "audit rule" but Mimi explained the
> reasoning for IMA referring to this as an "audit filter" here:
> 
>  https://lore.kernel.org/lkml/1593466203.5085.62.camel@linux.ibm.com/
> 
> I would be fine with her renaming/rewording this patch, accordingly, in
> next-integrity-testing.

Both here and "ima: AppArmor satisfies the audit rule requirements",
the subject is AppArmor/LSM, which do refer to the rules as "audit"
rules.  In the "ima: Rename internal audit rule functions" case, the
rule rename is internal to IMA.  Here it makes sense to replace
"audit" with "filter".  Tyler, I've gone ahead and made the change.

Mimi

Patch
diff mbox series

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4515975cc540..59ec28f5c117 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -420,6 +420,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
@@ -430,6 +431,10 @@  static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
 	return -EINVAL;
 }
 
+static inline void security_filter_rule_free(void *lsmrule)
+{
+}
+
 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 66aa3e17a888..d7c268c2b0ce 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);