[PATCH v2] apparmor: fix potential label refcnt leak in aa_change_profile

[PATCH v2] apparmor: fix potential label refcnt leak in aa_change_profile

[Xiyu Yang]

aa_change_profile() invokes aa_get_current_label(), which returns
a reference of the current task's label.

According to the comment of aa_get_current_label(), the returned
reference must be put with aa_put_label().
However, when the original object pointed by "label" becomes
unreachable because aa_change_profile() returns or a new object
is assigned to "label", reference count increased by
aa_get_current_label() is not decreased, causing a refcnt leak.

Fix this by calling aa_put_label() before aa_change_profile() return
and dropping unnecessary aa_get_current_label().

Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
---
Changes in v2:
- Remove unnecessary aa_get_current_label() because it gets called
  earlier in the fn
---
 security/apparmor/domain.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 6ceb74e0f789..a84ef030fbd7 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -1328,6 +1328,7 @@ int aa_change_profile(const char *fqname, int flags)
 		ctx->nnp = aa_get_label(label);
 
 	if (!fqname || !*fqname) {
+		aa_put_label(label);
 		AA_DEBUG("no profile name");
 		return -EINVAL;
 	}
@@ -1346,8 +1347,6 @@ int aa_change_profile(const char *fqname, int flags)
 			op = OP_CHANGE_PROFILE;
 	}
 
-	label = aa_get_current_label();
-
 	if (*fqname == '&') {
 		stack = true;
 		/* don't have label_parse() do stacking */
-- 
2.7.4

Re: [PATCH v2] apparmor: fix potential label refcnt leak in aa_change_profile

[Markus Elfring]

> According to the comment of aa_get_current_label(), …

I suggest to make this wording clearer.
Would you like to refer to any software documentation here?


> However, when the original object pointed by "label" becomes
> unreachable because aa_change_profile() returns or a new object
> is assigned to "label", reference count increased by
> aa_get_current_label() is not decreased, causing a refcnt leak.

How do you think about to reduce abbreviations in the commit message?

Would you like to add the tag “Fixes” to the change description?

Regards,
Markus

Re: [PATCH v2] apparmor: fix potential label refcnt leak in aa_change_profile

[John Johansen]

On 4/15/20 4:27 AM, Markus Elfring wrote:
>> According to the comment of aa_get_current_label(), …
> 
> I suggest to make this wording clearer.
> Would you like to refer to any software documentation here?
> 
> 
>> However, when the original object pointed by "label" becomes
>> unreachable because aa_change_profile() returns or a new object
>> is assigned to "label", reference count increased by
>> aa_get_current_label() is not decreased, causing a refcnt leak.
> 
> How do you think about to reduce abbreviations in the commit message?
> 
> Would you like to add the tag “Fixes” to the change description?
> 
Fixes tags are always nice to have filled out, but some times its
hard to determine or the patch submitter doesn't know how or ...
If the fixes tags aren't there I will add them before I push them up.
In this case its

Fixes: 9fcf78cca198 ("apparmor: update domain transitions that are subsets of confinement at nnp")

> Regards,
> Markus
>