| Message ID | 20210816175558.617474-3-georgia.garcia@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,B,v3,1/1] apparmor: Fix memory leak of profile proxy | expand |
On 16.08.21 19:55, Georgia Garcia wrote: > From: John Johansen <john.johansen@canonical.com> > > BugLink: https://bugs.launchpad.net/bugs/1939915 > > When the proxy isn't replaced and the profile is removed, the proxy > is being leaked resulting in a kmemleak check message of > > unreferenced object 0xffff888077a3a490 (size 16): > comm "apparmor_parser", pid 128041, jiffies 4322684109 (age 1097.028s) > hex dump (first 16 bytes): > 03 00 00 00 00 00 00 00 b0 92 fd 4b 81 88 ff ff ...........K.... > backtrace: > [<0000000084d5daf2>] aa_alloc_proxy+0x58/0xe0 > [<00000000ecc0e21a>] aa_alloc_profile+0x159/0x1a0 > [<000000004cc9ce15>] unpack_profile+0x275/0x1c40 > [<000000007332b3ca>] aa_unpack+0x1e7/0x7e0 > [<00000000e25e31bd>] aa_replace_profiles+0x18a/0x1d10 > [<00000000350d9415>] policy_update+0x237/0x650 > [<000000003fbf934e>] profile_load+0x122/0x160 > [<0000000047f7b781>] vfs_write+0x139/0x290 > [<000000008ad12358>] ksys_write+0xcd/0x170 > [<000000001a9daa7b>] do_syscall_64+0x70/0x310 > [<00000000b9efb0cf>] entry_SYSCALL_64_after_hwframe+0x49/0xb3 > > Make sure to cleanup the profile's embedded label which will result > on the proxy being properly freed. > > Fixes: 637f688dc3dc ("apparmor: switch from profiles to using labels on contexts") > Signed-off-by: John Johansen <john.johansen@canonical.com> > (backported from commit 3622ad25d4d68fcbdef3bc084b5916873e785344) > Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> > --- > security/apparmor/include/label.h | 1 + > security/apparmor/label.c | 19 +++++++------------ > security/apparmor/policy.c | 1 + > 3 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h > index af22dcbbcb8a..8ef94d6ffa09 100644 > --- a/security/apparmor/include/label.h > +++ b/security/apparmor/include/label.h > @@ -279,6 +279,7 @@ void aa_labelset_destroy(struct aa_labelset *ls); > void aa_labelset_init(struct aa_labelset *ls); > void __aa_labelset_update_subtree(struct aa_ns *ns); > > +void aa_label_destroy(struct aa_label *label); > void aa_label_free(struct aa_label *label); > void aa_label_kref(struct kref *kref); > bool aa_label_init(struct aa_label *label, int size); > diff --git a/security/apparmor/label.c b/security/apparmor/label.c > index 9171cd8ec032..b5978f310ced 100644 > --- a/security/apparmor/label.c > +++ b/security/apparmor/label.c > @@ -313,10 +313,8 @@ int aa_vec_unique(struct aa_profile **vec, int n, int flags) > } > > > -static void label_destroy(struct aa_label *label) > +void aa_label_destroy(struct aa_label *label) > { > - struct aa_label *tmp; > - > AA_BUG(!label); > > if (!label_isprofile(label)) { > @@ -332,16 +330,13 @@ static void label_destroy(struct aa_label *label) > } > } > > - if (rcu_dereference_protected(label->proxy->label, true) == label) > - rcu_assign_pointer(label->proxy->label, NULL); > - > + if (label->proxy) { > + if (rcu_dereference_protected(label->proxy->label, true) == label) > + rcu_assign_pointer(label->proxy->label, NULL); > + aa_put_proxy(label->proxy); > + } > aa_free_secid(label->secid); > > - tmp = rcu_dereference_protected(label->proxy->label, true); > - if (tmp == label) > - rcu_assign_pointer(label->proxy->label, NULL); > - Instead of silently dropping these lines we should rather pick additionally commit c84b80cd41e0 Author: Mateusz Nosek <mateusznosek0@gmail.com> Date: Tue Mar 3 19:30:23 2020 +0100 security/apparmor/label.c: Clean code by removing redundant instructions which does only that. There is a chance that this potentially allows to cherry pick the actual fix for Bionic as well. So [SRU B/F] Fix memory leak... +- [SRU B 1/2] security/apparmor/label.c: Clean code... +- [SRU B 2/1, F 1/1] apparmor: Fix memory... > > label->proxy = (struct aa_proxy *) PROXY_POISON + 1; > } > > @@ -350,7 +345,7 @@ void aa_label_free(struct aa_label *label) > if (!label) > return; > > - label_destroy(label); > + aa_label_destroy(label); > kfree(label); > } > > diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c > index a92c167c9249..1a7aa0ad5d0b 100644 > --- a/security/apparmor/policy.c > +++ b/security/apparmor/policy.c > @@ -240,6 +240,7 @@ void aa_free_profile(struct aa_profile *profile) > > kzfree(profile->hash); > aa_put_loaddata(profile->rawdata); > + aa_label_destroy(&profile->label); > > kzfree(profile); > } >
diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h index af22dcbbcb8a..8ef94d6ffa09 100644 --- a/security/apparmor/include/label.h +++ b/security/apparmor/include/label.h @@ -279,6 +279,7 @@ void aa_labelset_destroy(struct aa_labelset *ls); void aa_labelset_init(struct aa_labelset *ls); void __aa_labelset_update_subtree(struct aa_ns *ns); +void aa_label_destroy(struct aa_label *label); void aa_label_free(struct aa_label *label); void aa_label_kref(struct kref *kref); bool aa_label_init(struct aa_label *label, int size); diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 9171cd8ec032..b5978f310ced 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -313,10 +313,8 @@ int aa_vec_unique(struct aa_profile **vec, int n, int flags) } -static void label_destroy(struct aa_label *label) +void aa_label_destroy(struct aa_label *label) { - struct aa_label *tmp; - AA_BUG(!label); if (!label_isprofile(label)) { @@ -332,16 +330,13 @@ static void label_destroy(struct aa_label *label) } } - if (rcu_dereference_protected(label->proxy->label, true) == label) - rcu_assign_pointer(label->proxy->label, NULL); - + if (label->proxy) { + if (rcu_dereference_protected(label->proxy->label, true) == label) + rcu_assign_pointer(label->proxy->label, NULL); + aa_put_proxy(label->proxy); + } aa_free_secid(label->secid); - tmp = rcu_dereference_protected(label->proxy->label, true); - if (tmp == label) - rcu_assign_pointer(label->proxy->label, NULL); - - aa_put_proxy(label->proxy); label->proxy = (struct aa_proxy *) PROXY_POISON + 1; } @@ -350,7 +345,7 @@ void aa_label_free(struct aa_label *label) if (!label) return; - label_destroy(label); + aa_label_destroy(label); kfree(label); } diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index a92c167c9249..1a7aa0ad5d0b 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -240,6 +240,7 @@ void aa_free_profile(struct aa_profile *profile) kzfree(profile->hash); aa_put_loaddata(profile->rawdata); + aa_label_destroy(&profile->label); kzfree(profile); }