Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4ad67bac-bae1-dd3f-bdcb-a2f5bfdfa2b6@canonical.com>
Date:   Mon, 24 Oct 2022 22:50:33 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH] apparmor: Fix memleak issue in unpack_profile()
Content-Language: en-US
To:     Xiu Jianfeng <xiujianfeng@huawei.com>, paul@paul-moore.com,
        serge@hallyn.com
Cc:     apparmor@lists.ubuntu.com, linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20221021093602.102839-1-xiujianfeng@huawei.com>
From:   John Johansen <john.johansen@canonical.com>
Organization: Canonical
In-Reply-To: <20221021093602.102839-1-xiujianfeng@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/21/22 02:36, Xiu Jianfeng wrote:
> Before aa_alloc_profile(), it has allocated string for @*ns_name if @tmpns
> is not NULL, so directly return -ENOMEM if aa_alloc_profile() failed will
> cause a memleak issue, and even if aa_alloc_profile() succeed, in the
> @fail_profile tag of aa_unpack(), it need to free @ns_name as well, this
> patch fixes them.
> 
> Fixes: 736ec752d95e ("AppArmor: policy routines for loading and unpacking policy")
> Fixes: 04dc715e24d0 ("apparmor: audit policy ns specified in policy load")
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>

yep thanks I have pulled this into my tree

Acked-by: John Johansen <john.johansen@canonical.com>

> ---
>   security/apparmor/policy_unpack.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 2e028d540c6b..1bf8cfb8700a 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -858,8 +858,11 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
>   	}
>   
>   	profile = aa_alloc_profile(name, NULL, GFP_KERNEL);
> -	if (!profile)
> -		return ERR_PTR(-ENOMEM);
> +	if (!profile) {
> +		info = "out of memory";
> +		error = -ENOMEM;
> +		goto fail;
> +	}
>   	rules = list_first_entry(&profile->rules, typeof(*rules), list);
>   
>   	/* profile renaming is optional */
> @@ -1090,6 +1093,10 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
>   	if (error == 0)
>   		/* default error covers most cases */
>   		error = -EPROTO;
> +	if (*ns_name) {
> +		kfree(*ns_name);
> +		*ns_name = NULL;
> +	}
>   	if (profile)
>   		name = NULL;
>   	else if (!name)
> @@ -1392,6 +1399,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
>   {
>   	struct aa_load_ent *tmp, *ent;
>   	struct aa_profile *profile = NULL;
> +	char *ns_name = NULL;
>   	int error;
>   	struct aa_ext e = {
>   		.start = udata->data,
> @@ -1401,7 +1409,6 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
>   
>   	*ns = NULL;
>   	while (e.pos < e.end) {
> -		char *ns_name = NULL;
>   		void *start;
>   		error = verify_header(&e, e.pos == e.start, ns);
>   		if (error)
> @@ -1432,6 +1439,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
>   
>   		ent->new = profile;
>   		ent->ns_name = ns_name;
> +		ns_name = NULL;
>   		list_add_tail(&ent->list, lh);
>   	}
>   	udata->abi = e.version & K_ABI_MASK;
> @@ -1452,6 +1460,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
>   	return 0;
>   
>   fail_profile:
> +	kfree(ns_name);
>   	aa_put_profile(profile);
>   
>   fail: