Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] genetlink: prevent memory leak in netlbl_unlabel_defconf
@ 2019-09-25 22:10 Navid Emamdoost
  2019-09-25 23:27 ` Paul Moore
  0 siblings, 1 reply; 5+ messages in thread
From: Navid Emamdoost @ 2019-09-25 22:10 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Paul Moore,
	David S. Miller, netdev, linux-security-module, linux-kernel

In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the
allocated entry should be released.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 net/netlabel/netlabel_unlabeled.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index d2e4ab8d1cb1..c63ec480ee4e 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -1541,8 +1541,10 @@ int __init netlbl_unlabel_defconf(void)
 	entry->family = AF_UNSPEC;
 	entry->def.type = NETLBL_NLTYPE_UNLABELED;
 	ret_val = netlbl_domhsh_add_default(entry, &audit_info);
-	if (ret_val != 0)
+	if (ret_val != 0) {
+		kfree(entry);
 		return ret_val;
+	}
 
 	netlbl_unlabel_acceptflg_set(1, &audit_info);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] genetlink: prevent memory leak in netlbl_unlabel_defconf
  2019-09-25 22:10 [PATCH] genetlink: prevent memory leak in netlbl_unlabel_defconf Navid Emamdoost
@ 2019-09-25 23:27 ` Paul Moore
  2019-09-27 13:15   ` Markus Elfring
  0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2019-09-25 23:27 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: emamd001, kjlu, smccaman, David S. Miller, netdev,
	linux-security-module, linux-kernel

On Wed, Sep 25, 2019 at 6:10 PM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
>
> In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the
> allocated entry should be released.
>
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
>  net/netlabel/netlabel_unlabeled.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

It is worth noting that netlbl_unlabel_defconf() is only called during
kernel boot by netlbl_init() and if netlbl_unlabel_defconf() returns
an error code the system panics, so this isn't currently a very
practical concern.

That said, netlbl_unlabel_defconf() *should* clean up here just on
principal if nothing else.

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index d2e4ab8d1cb1..c63ec480ee4e 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -1541,8 +1541,10 @@ int __init netlbl_unlabel_defconf(void)
>         entry->family = AF_UNSPEC;
>         entry->def.type = NETLBL_NLTYPE_UNLABELED;
>         ret_val = netlbl_domhsh_add_default(entry, &audit_info);
> -       if (ret_val != 0)
> +       if (ret_val != 0) {
> +               kfree(entry);
>                 return ret_val;
> +       }
>
>         netlbl_unlabel_acceptflg_set(1, &audit_info);
>
> --
> 2.17.1
>


-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: genetlink: prevent memory leak in netlbl_unlabel_defconf
  2019-09-25 23:27 ` Paul Moore
@ 2019-09-27 13:15   ` Markus Elfring
  2019-09-27 14:48     ` Paul Moore
  0 siblings, 1 reply; 5+ messages in thread
From: Markus Elfring @ 2019-09-27 13:15 UTC (permalink / raw)
  To: Paul Moore, Navid Emamdoost, linux-security-module, netdev
  Cc: Navid Emamdoost, Kangjie Lu, linux-kernel, kernel-janitors,
	Stephen A McCamant, David S. Miller

> > In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the
> > allocated entry should be released.
> That said, netlbl_unlabel_defconf() *should* clean up here just on
> principal if nothing else.

How do you think about to add the tag “Fixes” then?

Regards,
Markus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: genetlink: prevent memory leak in netlbl_unlabel_defconf
  2019-09-27 13:15   ` Markus Elfring
@ 2019-09-27 14:48     ` Paul Moore
  2019-09-27 16:14       ` David Miller
  0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2019-09-27 14:48 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Navid Emamdoost, linux-security-module, netdev, Navid Emamdoost,
	Kangjie Lu, linux-kernel, kernel-janitors, Stephen A McCamant,
	David S. Miller

On Fri, Sep 27, 2019 at 9:15 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > > In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the
> > > allocated entry should be released.
> …
> > That said, netlbl_unlabel_defconf() *should* clean up here just on
> > principal if nothing else.
>
> How do you think about to add the tag “Fixes” then?

From what I've seen the "Fixes" tag is typically used by people who
are backporting patches, e.g. the -stable folks, to help decide what
they need to backport.  As I mentioned in my previous email this
missing free doesn't actually manifest itself as a practical leak on
any of the existing kernels so there isn't a need to backport this
patch.  For that reason I would probably skip the "Fixes" metadata
here, but I don't feel strongly enough about it to object if others
want it.  FWIW, I play things very conservatively when talking about
backporting patches to stable kernels; if it doesn't fix a serious
user-visible bug it shouldn't be backported IMHO.

This patch is more of a conceptual fix than a practical fix.  Not that
there is anything wrong with this patch, I just think it isn't as
critical as most people would think from reading "memory leak" in the
subject line.  Yes, there is a memory leak, but the kernel panics soon
after so it's a bit moot.  Further, even if the panic was somehow
skipped (?) the memory leak only happens once during boot; the failed
initialization is undoubtedly going to be far more damaging to the
system than a few lost bytes of memory.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: genetlink: prevent memory leak in netlbl_unlabel_defconf
  2019-09-27 14:48     ` Paul Moore
@ 2019-09-27 16:14       ` David Miller
  0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2019-09-27 16:14 UTC (permalink / raw)
  To: paul
  Cc: Markus.Elfring, navid.emamdoost, linux-security-module, netdev,
	emamd001, kjlu, linux-kernel, kernel-janitors, smccaman

From: Paul Moore <paul@paul-moore.com>
Date: Fri, 27 Sep 2019 10:48:54 -0400

> From what I've seen the "Fixes" tag is typically used by people who
> are backporting patches, e.g. the -stable folks, to help decide what
> they need to backport.

Fixes: tags say what commit introduced the code being fixed, whether
it manifests in a real problem or not.

It has nothing directly to do with -stable and exists in it's own right
whether a change gets backported to -stable or not.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 22:10 [PATCH] genetlink: prevent memory leak in netlbl_unlabel_defconf Navid Emamdoost
2019-09-25 23:27 ` Paul Moore
2019-09-27 13:15   ` Markus Elfring
2019-09-27 14:48     ` Paul Moore
2019-09-27 16:14       ` David Miller

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git