Hi Tom, On Thu, Jun 11, 2020 at 5:58 PM <trix@xxxxxxxxxx> wrote: > From: Tom Rix <trix@xxxxxxxxxx> > > Clang static analysis reports this double free error > > security/selinux/ss/conditional.c:139:2: warning: Attempt to free released memory [unix.Malloc] > kfree(node->expr.nodes); > ^~~~~~~~~~~~~~~~~~~~~~~ > > When cond_read_node fails, it calls cond_node_destroy which frees the > node but does not poison the entry in the node list. So when it > returns to its caller cond_read_list, cond_read_list deletes the > partial list. The latest entry in the list will be deleted twice. > > So instead of freeing the node in cond_read_node, let list freeing in > code_read_list handle the freeing the problem node along with all of the the There is an extra "the" before "freeing". > earlier nodes. > > Signed-off-by: Tom Rix <trix@xxxxxxxxxx> > --- > security/selinux/ss/conditional.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c > index da94a1b4bfda..ffb31af22f4f 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -411,7 +411,6 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp) > goto err; > return 0; > err: > - cond_node_destroy(node); > return rc; Since there is now just "return rc" in the error path, can you please replace all the gotos with plain return statements? And please also add a Fixes: tag pointing to the commit that introduced the bug (see Stephen's reply). Thanks, -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
