On Thu, Jun 11, 2020 at 4:48 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 > earlier nodes. > > Because cond_read_node no longer does any error handling, the goto's > the error case are redundant. Instead just return the error code. > > Fixes a problem was introduced by commit > > selinux: convert cond_list to array > > Signed-off-by: Tom Rix <trix@xxxxxxxxxx> > --- > security/selinux/ss/conditional.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) Hi Tom, Thanks for the patch! A few more notes, in no particular order: * There is no need to send a cover letter for just a single patch. Typically cover letters are reserved for large patchsets that require some additional explanation and/or instructions beyond the individual commit descriptions. * Thank you for including a changelog with your patch updates, but it would be helpful if you included them in the patch by using a "---" delimiter in the commit description after your signoff but before the diffstat. Here is a recent example: -> https://lore.kernel.org/selinux/20200611135303.19538-3-cgzones@xxxxxxxxxxxxxx * When referencing a patch which you are "fixing", the proper syntax is 'Fixes: <12char_commitID> ("<subject_line")'. Look at commit 46619b44e431 in Linus' tree to see an example. If you have any questions, let us know. > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c > index da94a1b4bfda..d0d6668709f0 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -392,26 +392,21 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp) > > rc = next_entry(buf, fp, sizeof(u32) * 2); > if (rc) > - goto err; > + return rc; > > expr->expr_type = le32_to_cpu(buf[0]); > expr->bool = le32_to_cpu(buf[1]); > > if (!expr_node_isvalid(p, expr)) { > rc = -EINVAL; > - goto err; > + return rc; > } > } > > rc = cond_read_av_list(p, fp, &node->true_list, NULL); > if (rc) > - goto err; > + return rc; > rc = cond_read_av_list(p, fp, &node->false_list, &node->true_list); > - if (rc) > - goto err; > - return 0; > -err: > - cond_node_destroy(node); > return rc; > } > > -- > 2.18.1 -- paul moore www.paul-moore.com
