| Message ID | 20200615204548.9230-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] selinux: fix another double free | expand |
On Mon, Jun 15, 2020 at 10:46 PM <trix@redhat.com> wrote: > From: Tom Rix <trix@redhat.com> > > 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: 60abd3181db2 ("selinux: convert cond_list to array") > > Signed-off-by: Tom Rix <trix@redhat.com> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> Thanks!
On Mon, Jun 15, 2020 at 4:45 PM <trix@redhat.com> wrote: > From: Tom Rix <trix@redhat.com> > > 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: 60abd3181db2 ("selinux: convert cond_list to array") > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > v3: simplify returns > > security/selinux/ss/conditional.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) Merged into selinux/stable-5.8 with a better subject line. Thanks for catching this and submitting the fix. Assuming everything goes well I'll send this up to Linus later this week. It might be nice to do a follow-up patch for selinux/next which folds cond_node_destroy() into cond_list_destroy() as there is no longer a need for that code to be in a separate function.
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index da94a1b4bfda..450bc02f4cd2 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -392,27 +392,19 @@ 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; - } + if (!expr_node_isvalid(p, expr)) + return -EINVAL; } rc = cond_read_av_list(p, fp, &node->true_list, NULL); if (rc) - goto err; - 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; + return rc; + return cond_read_av_list(p, fp, &node->false_list, &node->true_list); } int cond_read_list(struct policydb *p, void *fp)