From: Gou Hao gouhao@uniontech.com
patch1: is memory leak of audit rule patch2~3: is memory leak about 'fsname' field of struct ima_rule_entry
Tyler Hicks (3): ima: Have the LSM free its audit rule ima: Free the entire rule when deleting a list of rules ima: Free the entire rule if it fails to parse
security/integrity/ima/ima.h | 5 +++++ security/integrity/ima/ima_policy.c | 24 ++++++++++++++++++------ 2 files changed, 23 insertions(+), 6 deletions(-)
From: Tyler Hicks tyhicks@linux.microsoft.com
commit 9ff8a616dfab96a4fa0ddd36190907dc68886d9b upstream.
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 Signed-off-by: Mimi Zohar zohar@linux.ibm.com Cc: stable@vger.kernel.org # 4.19+ Signed-off-by: Gou Hao gouhao@uniontech.com --- security/integrity/ima/ima.h | 5 +++++ security/integrity/ima/ima_policy.c | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index d12b07eb3a58..e2916b115b93 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -298,6 +298,7 @@ static inline int ima_read_xattr(struct dentry *dentry, #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 @@ -308,6 +309,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, struct audit_context *actx) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 2d5a3daa02f9..733efc06d3c1 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1044,8 +1044,10 @@ void ima_delete_rules(void)
temp_ima_appraise = 0; list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) { - for (i = 0; i < MAX_LSM_RULES; i++) + for (i = 0; i < MAX_LSM_RULES; i++) { + security_filter_rule_free(entry->lsm[i].rule); kfree(entry->lsm[i].args_p); + }
list_del(&entry->list); kfree(entry);
From: Tyler Hicks tyhicks@linux.microsoft.com
commit 465aee77aae857b5fcde56ee192b33dc369fba04 upstream.
Create a function, ima_free_rule(), to free all memory associated with an ima_rule_entry. Use the new function to fix memory leaks of allocated ima_rule_entry members, such as .fsname and .keyrings, when deleting a list of rules.
Make the existing ima_lsm_free_rule() function specific to the LSM audit rule array of an ima_rule_entry and require that callers make an additional call to kfree to free the ima_rule_entry itself.
This fixes a memory leak seen when loading by a valid rule that contains an additional piece of allocated memory, such as an fsname, followed by an invalid rule that triggers a policy load failure:
# echo -e "dont_measure fsname=securityfs\nbad syntax" > \ /sys/kernel/security/ima/policy -bash: echo: write error: Invalid argument # echo scan > /sys/kernel/debug/kmemleak # cat /sys/kernel/debug/kmemleak unreferenced object 0xffff9bab67ca12c0 (size 16): comm "bash", pid 684, jiffies 4295212803 (age 252.344s) hex dump (first 16 bytes): 73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk. backtrace: [<00000000adc80b1b>] kstrdup+0x2e/0x60 [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020 [<00000000444825ac>] ima_write_policy+0xab/0x1d0 [<000000002b7f0d6c>] vfs_write+0xde/0x1d0 [<0000000096feedcf>] ksys_write+0x68/0xe0 [<0000000052b544a2>] do_syscall_64+0x56/0xa0 [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name") Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy") Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com Signed-off-by: Mimi Zohar zohar@linux.ibm.com Cc: stable@vger.kernel.org # 4.19+ Signed-off-by: Gou Hao gouhao@uniontech.com --- security/integrity/ima/ima_policy.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 733efc06d3c1..8a55bdfad404 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -241,6 +241,21 @@ static int __init default_appraise_policy_setup(char *str) } __setup("ima_appraise_tcb", default_appraise_policy_setup);
+static void ima_free_rule(struct ima_rule_entry *entry) +{ + int i; + + if (!entry) + return; + + kfree(entry->fsname); + for (i = 0; i < MAX_LSM_RULES; i++) { + security_filter_rule_free(entry->lsm[i].rule); + kfree(entry->lsm[i].args_p); + } + kfree(entry); +} + /* * The LSM policy can be reloaded, leaving the IMA LSM based rules referring * to the old, stale LSM policy. Update the IMA LSM based rules to reflect @@ -1040,17 +1055,11 @@ ssize_t ima_parse_add_rule(char *rule) void ima_delete_rules(void) { struct ima_rule_entry *entry, *tmp; - int i;
temp_ima_appraise = 0; list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) { - for (i = 0; i < MAX_LSM_RULES; i++) { - security_filter_rule_free(entry->lsm[i].rule); - kfree(entry->lsm[i].args_p); - } - list_del(&entry->list); - kfree(entry); + ima_free_rule(entry); } }
From: Tyler Hicks tyhicks@linux.microsoft.com
commit 2bdd737c5687d6dec30e205953146ede8a87dbdd upstream.
Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry members, such as .fsname and .keyrings, when an error is encountered during rule parsing.
Set the args_p pointer to NULL after freeing it in the error path of ima_lsm_rule_init() so that it isn't freed twice.
This fixes a memory leak seen when loading an rule that contains an additional piece of allocated memory, such as an fsname, followed by an invalid conditional:
# echo "measure fsname=tmpfs bad=cond" > /sys/kernel/security/ima/policy -bash: echo: write error: Invalid argument # echo scan > /sys/kernel/debug/kmemleak # cat /sys/kernel/debug/kmemleak unreferenced object 0xffff98e7e4ece6c0 (size 8): comm "bash", pid 672, jiffies 4294791843 (age 21.855s) hex dump (first 8 bytes): 74 6d 70 66 73 00 6b a5 tmpfs.k. backtrace: [<00000000abab7413>] kstrdup+0x2e/0x60 [<00000000f11ede32>] ima_parse_add_rule+0x7d4/0x1020 [<00000000f883dd7a>] ima_write_policy+0xab/0x1d0 [<00000000b17cf753>] vfs_write+0xde/0x1d0 [<00000000b8ddfdea>] ksys_write+0x68/0xe0 [<00000000b8e21e87>] do_syscall_64+0x56/0xa0 [<0000000089ea7b98>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name") Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy") Signed-off-by: Tyler Hicks tyhicks@linux.microsoft.com Signed-off-by: Mimi Zohar zohar@linux.ibm.com Cc: stable@vger.kernel.org # 4.19+ Signed-off-by: Gou Hao gouhao@uniontech.com --- security/integrity/ima/ima_policy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 8a55bdfad404..b2dadff3626b 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -662,6 +662,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry, &entry->lsm[lsm_rule].rule); if (!entry->lsm[lsm_rule].rule) { kfree(entry->lsm[lsm_rule].args_p); + entry->lsm[lsm_rule].args_p = NULL; return -EINVAL; }
@@ -1034,7 +1035,7 @@ ssize_t ima_parse_add_rule(char *rule)
result = ima_parse_rule(p, entry); if (result) { - kfree(entry); + ima_free_rule(entry); integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, op, "invalid-policy", result, audit_info);
On Fri, Sep 30, 2022 at 03:49:34PM +0800, gouhao@uniontech.com wrote:
From: Gou Hao gouhao@uniontech.com
patch1: is memory leak of audit rule patch2~3: is memory leak about 'fsname' field of struct ima_rule_entry
Tyler Hicks (3): ima: Have the LSM free its audit rule ima: Free the entire rule when deleting a list of rules ima: Free the entire rule if it fails to parse
security/integrity/ima/ima.h | 5 +++++ security/integrity/ima/ima_policy.c | 24 ++++++++++++++++++------ 2 files changed, 23 insertions(+), 6 deletions(-)
-- 2.20.1
Now queued up, thanks.
greg k-h
On 2022-10-02 17:35:24, Greg KH wrote:
On Fri, Sep 30, 2022 at 03:49:34PM +0800, gouhao@uniontech.com wrote:
From: Gou Hao gouhao@uniontech.com
patch1: is memory leak of audit rule patch2~3: is memory leak about 'fsname' field of struct ima_rule_entry
Tyler Hicks (3): ima: Have the LSM free its audit rule ima: Free the entire rule when deleting a list of rules ima: Free the entire rule if it fails to parse
security/integrity/ima/ima.h | 5 +++++ security/integrity/ima/ima_policy.c | 24 ++++++++++++++++++------ 2 files changed, 23 insertions(+), 6 deletions(-)
-- 2.20.1
Now queued up, thanks.
I know these patches have been already applied and were even released a couple days ago but I wanted to say that I reviewed these backports, since they were a little tricky, and they all look good. Thanks for doing this, Gou!
Tyler
greg k-h
linux-stable-mirror@lists.linaro.org
-
gouhao＠uniontech.com -
Greg KH -
Tyler Hicks