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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHC9VhRWcRbQi0z90_BHMSBzaCW+muD4Oy_hwNfuxyVjLJsA+A@mail.gmail.com>
Date:   Fri, 12 Jun 2020 09:30:53 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Ondrej Mosnacek <omosnace@...hat.com>
Cc:     Tom Rix <trix@...hat.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>,
        Wei Yongjun <weiyongjun1@...wei.com>,
        SElinux list <selinux@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/1] selinux: fix another double free

On Fri, Jun 12, 2020 at 4:01 AM Ondrej Mosnacek <omosnace@...hat.com> wrote:
> On Fri, Jun 12, 2020 at 1:27 AM Paul Moore <paul@...l-moore.com> wrote:
> > On Thu, Jun 11, 2020 at 6:41 PM Tom Rix <trix@...hat.com> wrote:
> > > On 6/11/20 3:30 PM, Paul Moore wrote:
> > > > On Thu, Jun 11, 2020 at 4:48 PM <trix@...hat.com> wrote:
> > > >> From: Tom Rix <trix@...hat.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@...hat.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.

-- 
paul moore
www.paul-moore.com