| Message ID | 20200611204746.6370-2-trix@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: fix another double free | expand |
On Thu, Jun 11, 2020 at 4:48 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 a problem was introduced by commit > > selinux: convert cond_list to array > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > 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@googlemail.com * 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
On 6/11/20 3:30 PM, Paul Moore wrote: > On Thu, Jun 11, 2020 at 4:48 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 a problem was introduced by commit >> >> selinux: convert cond_list to array >> >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> 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. I was doing this to carry the repo name and tag info. So how do folks know which repo and commit the change applies to ? > * 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@googlemail.com Ok got it. > > * 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. Ok > If you have any questions, let us know.
On Thu, Jun 11, 2020 at 6:41 PM Tom Rix <trix@redhat.com> wrote: > On 6/11/20 3:30 PM, Paul Moore wrote: > > On Thu, Jun 11, 2020 at 4:48 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 a problem was introduced by commit > >> > >> selinux: convert cond_list to array > >> > >> Signed-off-by: Tom Rix <trix@redhat.com> > >> --- > >> 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. > > I was doing this to carry the repo name and tag info. > > So how do folks know which repo and commit the change applies to ? We read your mind ;) Generally it's pretty obvious, and in the rare occasion when it isn't, we ask. Most of the time you can deduce the destination repo by the files changed and the mailing lists on the To/CC line. From there it is then just a matter of -next vs -stable and that is something that is usually sorted out based on the context of the patch, and if needed, a discussion on-list.
On Thu, Jun 11, 2020 at 10:48 PM <trix@redhat.com> wrote: [...] > 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; > } Sorry for more nitpicking... This can be further simplified to just "return -EINVAL;" and the braces can be removed. > } > > 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; Also here you can skip the rc assignment: return cond_read_av_list(p, fp, &node->false_list, &node->true_list); > } > > -- > 2.18.1 -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
On Fri, Jun 12, 2020 at 1:27 AM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Jun 11, 2020 at 6:41 PM Tom Rix <trix@redhat.com> wrote: > > On 6/11/20 3:30 PM, Paul Moore wrote: > > > On Thu, Jun 11, 2020 at 4:48 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 a problem was introduced by commit > > >> > > >> selinux: convert cond_list to array > > >> > > >> Signed-off-by: Tom Rix <trix@redhat.com> > > >> --- > > >> 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. > > > > I was doing this to carry the repo name and tag info. > > > > So how do folks know which repo and commit the change applies to ? > > We read your mind ;) > > Generally it's pretty obvious, and in the rare occasion when it isn't, > we ask. Most of the time you can deduce the destination repo by the > files changed and the mailing lists on the To/CC line. From there it > is then just a matter of -next vs -stable and that is something that > is usually sorted out based on the context of the patch, and if > needed, a discussion on-list. Yes, it is normally not necessary, but I wouldn't discourage people from providing the info if they want to / are used to do that. It can be really useful in some situations, especially in case of cross-subsystem changes that are sent to many mailing lists. But of course this information belongs either to the cover letter or in case of single patches to the "informational" section between "---" and "diff --git [...]".
On Fri, Jun 12, 2020 at 4:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Fri, Jun 12, 2020 at 1:27 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Jun 11, 2020 at 6:41 PM Tom Rix <trix@redhat.com> wrote: > > > On 6/11/20 3:30 PM, Paul Moore wrote: > > > > On Thu, Jun 11, 2020 at 4:48 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 a problem was introduced by commit > > > >> > > > >> selinux: convert cond_list to array > > > >> > > > >> Signed-off-by: Tom Rix <trix@redhat.com> > > > >> --- > > > >> 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. > > > > > > I was doing this to carry the repo name and tag info. > > > > > > So how do folks know which repo and commit the change applies to ? > > > > We read your mind ;) > > > > Generally it's pretty obvious, and in the rare occasion when it isn't, > > we ask. Most of the time you can deduce the destination repo by the > > files changed and the mailing lists on the To/CC line. From there it > > is then just a matter of -next vs -stable and that is something that > > is usually sorted out based on the context of the patch, and if > > needed, a discussion on-list. > > Yes, it is normally not necessary, but I wouldn't discourage people > from providing the info if they want to / are used to do that. I would discourage adding this info into the commit. It clutters up the commit description and is of little value once the patch has been merged. > It can be really useful in some situations, especially in case of > cross-subsystem changes that are sent to many mailing lists. It has been my experience that those situations are rare enough that in the case where there is some question it is easily resolved over email. It's not something worth worrying about. > But of > course this information belongs either to the cover letter or in case > of single patches to the "informational" section between "---" and > "diff --git [...]". Here is my perspective ... Cover letters for single patches are annoying, either the information should be included in the commit description itself or the information is really not that important anyway. I also tend to like seeing only changelog information in that "informational" section so that it stays relatively clean and uncluttered. This means there really is no *good* place to put the repo information for single patches, which is okay, because as I've said before this really isn't a problem in practice; or at least it hasn't been a significant problem in the roughly seven years I've been maintaining the SELinux repo. If this ever becomes a common issue I'm open to discussing this further, but for right now let's not clutter things up to solve a problem that really isn't a major issue.
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; }