NACK/Cmnt: [SRU][B][PATCH v3 1/1] apparmor: Fix memory leak of profile proxy

[Stefan Bader]

On 16.08.21 19:55, Georgia Garcia wrote:
> From: John Johansen <john.johansen at 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 at canonical.com>
> (backported from commit 3622ad25d4d68fcbdef3bc084b5916873e785344)
> Signed-off-by: Georgia Garcia <georgia.garcia at 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 at 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);
>   }
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210819/af40fd09/attachment.sig>