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

From: Paul Moore <paul@paul-moore.com>
To: Tom Rix <trix@redhat.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>,
	Eric Paris <eparis@parisplace.org>,
	Ondrej Mosnacek <omosnace@redhat.com>,
	weiyongjun1@huawei.com, selinux@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] selinux: fix another double free
Date: Thu, 11 Jun 2020 19:27:12 -0400	[thread overview]
Message-ID: <CAHC9VhSPygyQAiWuaAWViuraCP6A9KTqmQeEt2_Rxta_WFsu5Q@mail.gmail.com> (raw)
In-Reply-To: <d8cee44a-316f-d1be-9893-428598ada2d8@redhat.com>

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.

-- 
paul moore
www.paul-moore.com