* [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline
@ 2022-09-30 7:49 gouhao
2022-09-30 7:49 ` [PATCH 1/3] ima: Have the LSM free its audit rule gouhao
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: gouhao @ 2022-09-30 7:49 UTC (permalink / raw)
To: stable; +Cc: gouhao, tyhicks, zohar, dmitry.kasatkin, jmorris, serge
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] ima: Have the LSM free its audit rule
2022-09-30 7:49 [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline gouhao
@ 2022-09-30 7:49 ` gouhao
2022-09-30 7:49 ` [PATCH 2/3] ima: Free the entire rule when deleting a list of rules gouhao
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: gouhao @ 2022-09-30 7:49 UTC (permalink / raw)
To: stable; +Cc: gouhao, tyhicks, zohar, dmitry.kasatkin, jmorris, serge
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);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] ima: Free the entire rule when deleting a list of rules
2022-09-30 7:49 [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline gouhao
2022-09-30 7:49 ` [PATCH 1/3] ima: Have the LSM free its audit rule gouhao
@ 2022-09-30 7:49 ` gouhao
2022-09-30 7:49 ` [PATCH 3/3] ima: Free the entire rule if it fails to parse gouhao
2022-10-02 15:35 ` [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline Greg KH
3 siblings, 0 replies; 6+ messages in thread
From: gouhao @ 2022-09-30 7:49 UTC (permalink / raw)
To: stable; +Cc: gouhao, tyhicks, zohar, dmitry.kasatkin, jmorris, serge
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);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] ima: Free the entire rule if it fails to parse
2022-09-30 7:49 [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline gouhao
2022-09-30 7:49 ` [PATCH 1/3] ima: Have the LSM free its audit rule gouhao
2022-09-30 7:49 ` [PATCH 2/3] ima: Free the entire rule when deleting a list of rules gouhao
@ 2022-09-30 7:49 ` gouhao
2022-10-02 15:35 ` [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline Greg KH
3 siblings, 0 replies; 6+ messages in thread
From: gouhao @ 2022-09-30 7:49 UTC (permalink / raw)
To: stable; +Cc: gouhao, tyhicks, zohar, dmitry.kasatkin, jmorris, serge
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);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline
2022-09-30 7:49 [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline gouhao
` (2 preceding siblings ...)
2022-09-30 7:49 ` [PATCH 3/3] ima: Free the entire rule if it fails to parse gouhao
@ 2022-10-02 15:35 ` Greg KH
2022-10-07 18:31 ` Tyler Hicks
3 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2022-10-02 15:35 UTC (permalink / raw)
To: gouhao; +Cc: stable, tyhicks, zohar, dmitry.kasatkin, jmorris, serge
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline
2022-10-02 15:35 ` [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline Greg KH
@ 2022-10-07 18:31 ` Tyler Hicks
0 siblings, 0 replies; 6+ messages in thread
From: Tyler Hicks @ 2022-10-07 18:31 UTC (permalink / raw)
To: Greg KH; +Cc: gouhao, stable, zohar, dmitry.kasatkin, jmorris, serge
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-07 18:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-30 7:49 [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline gouhao
2022-09-30 7:49 ` [PATCH 1/3] ima: Have the LSM free its audit rule gouhao
2022-09-30 7:49 ` [PATCH 2/3] ima: Free the entire rule when deleting a list of rules gouhao
2022-09-30 7:49 ` [PATCH 3/3] ima: Free the entire rule if it fails to parse gouhao
2022-10-02 15:35 ` [PATCH 0/3] Backporting some memory leak of ima policy to 4.19+ from mainline Greg KH
2022-10-07 18:31 ` Tyler Hicks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).