Re: [PATCH v2 1/1] selinux: fix another double free

[Ondrej Mosnacek]

On Fri, Jun 12, 2020 at 1:27 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Thu, Jun 11, 2020 at 6:41 PM Tom Rix <trix@xxxxxxxxxx> wrote:
> > On 6/11/20 3:30 PM, Paul Moore wrote:
> > > 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.
> >
> > 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 [...]".

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.