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

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

[trix]

From: Tom Rix <trix@redhat.com>

repo: linux-next
tag: next-20200611

Change from v1
Convert goto's to returns
Remove extra 'the' in the commit log
Add note on commit this is fixing in the commit log


Tom Rix (1):
  selinux: fix another double free

 security/selinux/ss/conditional.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

-- 
2.18.1

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

[trix]

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(-)

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

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

[Paul Moore]

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

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

[Tom Rix]

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.

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

[Paul Moore]

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

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

[Ondrej Mosnacek]

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.

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

[Ondrej Mosnacek]

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 [...]".

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

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

[Paul Moore]

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.

-- 
paul moore
www.paul-moore.com