| Message ID | 20191017014619.26708-1-navid.emamdoost@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | apparmor: Fix use-after-free in aa_audit_rule_init | expand |
> … But after this release the the return statement > tries to access the label field of the rule which results in > use-after-free. Before releaseing the rule, copy errNo and return it > after releasing rule. Please avoid a duplicate word and a typo in this change description. … > +++ b/security/apparmor/audit.c … > @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr, > GFP_KERNEL, true, false); > if (IS_ERR(rule->label)) { > + err = rule->label; How do you think about to define the added local variable in this if branch directly? + int err = rule->label; > aa_audit_rule_free(rule); > - return PTR_ERR(rule->label); > + return PTR_ERR(err); > } > > *vrule = rule; Regards, Markus
On 10/20/19 7:16 AM, Markus Elfring wrote: >> … But after this release the the return statement >> tries to access the label field of the rule which results in >> use-after-free. Before releaseing the rule, copy errNo and return it >> after releasing rule. > Navid thanks for finding this, and Markus thanks for the review > Please avoid a duplicate word and a typo in this change description. > My preference would be a v2 version of the patch with the small clean-ups that Markus has pointed out. If I don't see a v2 this week I can pull this one in and do the revisions myself adding a little fix-up note. > > … >> +++ b/security/apparmor/audit.c > … >> @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) >> rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr, >> GFP_KERNEL, true, false); >> if (IS_ERR(rule->label)) { >> + err = rule->label; > > How do you think about to define the added local variable in this if branch directly? > > + int err = rule->label; > yes, since err isn't defined or in use else where this would be preferable >> aa_audit_rule_free(rule); >> - return PTR_ERR(rule->label); >> + return PTR_ERR(err); >> } >> >> *vrule = rule; > > > Regards, > Markus >
On Sun, Oct 20, 2019 at 1:51 PM John Johansen <john.johansen@canonical.com> wrote: > > On 10/20/19 7:16 AM, Markus Elfring wrote: > >> … But after this release the the return statement > >> tries to access the label field of the rule which results in > >> use-after-free. Before releaseing the rule, copy errNo and return it > >> after releasing rule. > > > Navid thanks for finding this, and Markus thanks for the review > > > Please avoid a duplicate word and a typo in this change description. > > My preference would be a v2 version of the patch with the small clean-ups > that Markus has pointed out. John and Markus, I updated and submitted v2. > > If I don't see a v2 this week I can pull this one in and do the revisions > myself adding a little fix-up note. > > > > > … > >> +++ b/security/apparmor/audit.c > > … > >> @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) > >> rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr, > >> GFP_KERNEL, true, false); > >> if (IS_ERR(rule->label)) { > >> + err = rule->label; > > > > How do you think about to define the added local variable in this if branch directly? > > > > + int err = rule->label; > > > > yes, since err isn't defined or in use else where this would be preferable > > >> aa_audit_rule_free(rule); > >> - return PTR_ERR(rule->label); > >> + return PTR_ERR(err); > >> } > >> > >> *vrule = rule; > > > > > > Regards, > > Markus > > >
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c index 5a98661a8b46..48c15fb0aafe 100644 --- a/security/apparmor/audit.c +++ b/security/apparmor/audit.c @@ -178,6 +178,7 @@ void aa_audit_rule_free(void *vrule) int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) { struct aa_audit_rule *rule; + int err; switch (field) { case AUDIT_SUBJ_ROLE: @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr, GFP_KERNEL, true, false); if (IS_ERR(rule->label)) { + err = rule->label; aa_audit_rule_free(rule); - return PTR_ERR(rule->label); + return PTR_ERR(err); } *vrule = rule;
In the implementation of aa_audit_rule_init(), when aa_label_parse() fails the allocated memory for rule is released using aa_audit_rule_free(). But after this release the the return statement tries to access the label field of the rule which results in use-after-free. Before releaseing the rule, copy errNo and return it after releasing rule. Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path") Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- security/apparmor/audit.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)