Re: [PATCH] net: ipv4: current group_info should be put after using.

Re: [PATCH] net: ipv4: current group_info should be put after using.

[David Miller]

From: "Wang, Xiaoming" <xiaoming.wang@intel.com>
Date: Fri, 11 Apr 2014 12:10:38 -0400

> +EXIT:

Do not use all CAPS names for local labels.

Make the label, in lower case, say something about
what state needs to be released at that point.

Something like "out_release_group:"

[PATCH] net: ipv4: current group_info should be put after using.

[Wang, Xiaoming]

There is a memory leak in ping when call ping in dumpstate.
Current group_info had been got in ping_init_sock and group_info->usage
increased. But the usage hasn't decreased.
This will make group_info never freed and cause memory leak.

unreferenced object 0xcd0e8840 (size 192):
  comm "dumpstate", pid 7583, jiffies 78360 (age 91.810s)
  hex dump (first 32 bytes):
    02 00 00 00 06 00 00 00 01 00 00 00 ef 03 00 00  ................
    f1 03 00 00 f7 03 00 00 04 04 00 00 bb 0b 00 00  ................
  backtrace:
    [<c1a6bbfc>] kmemleak_alloc+0x3c/0xa0
    [<c1320457>] __kmalloc+0xe7/0x1d0
    [<c1267c04>] groups_alloc+0x34/0xb0
    [<c1267e5c>] SyS_setgroups+0x3c/0xf0
    [<c1a864a8>] syscall_call+0x7/0xb
    [<ffffffff>] 0xffffffff

Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
---
 net/ipv4/ping.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index f4b19e5..2af7b1f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -255,23 +255,28 @@ int ping_init_sock(struct sock *sk)
 	struct group_info *group_info = get_current_groups();
 	int i, j, count = group_info->ngroups;
 	kgid_t low, high;
+	int ret = 0;
 
 	inet_get_ping_group_range_net(net, &low, &high);
 	if (gid_lte(low, group) && gid_lte(group, high))
-		return 0;
+		goto EXIT;
 
 	for (i = 0; i < group_info->nblocks; i++) {
 		int cp_count = min_t(int, NGROUPS_PER_BLOCK, count);
 		for (j = 0; j < cp_count; j++) {
 			kgid_t gid = group_info->blocks[i][j];
 			if (gid_lte(low, gid) && gid_lte(gid, high))
-				return 0;
+				goto EXIT;
 		}
 
 		count -= cp_count;
 	}
 
-	return -EACCES;
+	ret = -EACCES;
+
+EXIT:
+	put_group_info(group_info);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ping_init_sock);
 
-- 
1.7.1

Re: [PATCH] net: ipv4: current group_info should be put after using.

[Mateusz Guzik]

On Sun, Apr 13, 2014 at 10:54:07PM -0400, David Miller wrote:
> From: "Wang, Xiaoming" <xiaoming.wang@intel.com>
> Date: Mon, 14 Apr 2014 12:30:45 -0400
> 
> > Plug a group_info refcount leak in ping_init.
> > group_info is only needed during initialization and 
> > the code failed to release the reference on exit.
> > While here move grabbing the reference to a place 
> > where it is actually needed.
> > 
> > Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> > Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> > Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
> 
> Applied and queued up for -stable, thanks.

I see this only made its way to 3.12 kernel, what is holding up inclusion
in other kernels? (or maybe this question should be directed at someone
else?)

The bug was introduced with:
commit c319b4d76b9e583a5d88d6bf190e079c4e43213d
Author: Vasiliy Kulikov <segoon@openwall.com>
Date:   Fri May 13 10:01:00 2011 +0000
    net: ipv4: add IPPROTO_ICMP socket kind

starting with 3.0.

Thanks,
-- 
Mateusz Guzik

[PATCH] net: ipv4: current group_info should be put after using.

[Wang, Xiaoming]

Plug a group_info refcount leak in ping_init.
group_info is only needed during initialization and 
the code failed to release the reference on exit.
While here move grabbing the reference to a place 
where it is actually needed.

Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
---
 net/ipv4/ping.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index f4b19e5..8210964 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -252,26 +252,33 @@ int ping_init_sock(struct sock *sk)
 {
 	struct net *net = sock_net(sk);
 	kgid_t group = current_egid();
-	struct group_info *group_info = get_current_groups();
-	int i, j, count = group_info->ngroups;
+	struct group_info *group_info;
+	int i, j, count;
 	kgid_t low, high;
+	int ret = 0;
 
 	inet_get_ping_group_range_net(net, &low, &high);
 	if (gid_lte(low, group) && gid_lte(group, high))
 		return 0;
 
+	group_info = get_current_groups();
+	count = group_info->ngroups;
 	for (i = 0; i < group_info->nblocks; i++) {
 		int cp_count = min_t(int, NGROUPS_PER_BLOCK, count);
 		for (j = 0; j < cp_count; j++) {
 			kgid_t gid = group_info->blocks[i][j];
 			if (gid_lte(low, gid) && gid_lte(gid, high))
-				return 0;
+				goto out_release_group;
 		}
 
 		count -= cp_count;
 	}
 
-	return -EACCES;
+	ret = -EACCES;
+
+out_release_group:
+	put_group_info(group_info);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ping_init_sock);
 
-- 
1.7.1

Re: [PATCH] net: ipv4: current group_info should be put after using.

[David Miller]

From: "Wang, Xiaoming" <xiaoming.wang@intel.com>
Date: Mon, 14 Apr 2014 12:30:45 -0400

> Plug a group_info refcount leak in ping_init.
> group_info is only needed during initialization and 
> the code failed to release the reference on exit.
> While here move grabbing the reference to a place 
> where it is actually needed.
> 
> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>

Applied and queued up for -stable, thanks.

Re: [PATCH] net: ipv4: current group_info should be put after using.

[David Miller]

From: Mateusz Guzik <mguzik@redhat.com>
Date: Fri, 11 Apr 2014 15:50:27 +0200

> Please cc: me if you resend the patch.

Never resubmit a patch as a reply to a thread discussion a previous
version of the patch if you want it to be considered for inclusion.

Always make a new, fresh, mailing list posting properly formated
for a patch submission and with out any unrelated material in the
email message.

Thanks.

[PATCH] net: ipv4: current group_info should be put after using.

[Wang, Xiaoming]

This is a typical refcount leak exploitable by unprivileged users.
Current group_info had been got in ping_init_sock and
group_info->usage increased. But the usage hasn't decreased
anywhere in ping. This will make this group_info never freed.

Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
---
 net/ipv4/ping.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index f4b19e5..8210964 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -252,26 +252,33 @@ int ping_init_sock(struct sock *sk)
 {
 	struct net *net = sock_net(sk);
 	kgid_t group = current_egid();
-	struct group_info *group_info = get_current_groups();
-	int i, j, count = group_info->ngroups;
+	struct group_info *group_info;
+	int i, j, count;
 	kgid_t low, high;
+	int ret = 0;
 
 	inet_get_ping_group_range_net(net, &low, &high);
 	if (gid_lte(low, group) && gid_lte(group, high))
 		return 0;
 
+	group_info = get_current_groups();
+	count = group_info->ngroups;
 	for (i = 0; i < group_info->nblocks; i++) {
 		int cp_count = min_t(int, NGROUPS_PER_BLOCK, count);
 		for (j = 0; j < cp_count; j++) {
 			kgid_t gid = group_info->blocks[i][j];
 			if (gid_lte(low, gid) && gid_lte(gid, high))
-				return 0;
+				goto out_release_group;
 		}
 
 		count -= cp_count;
 	}
 
-	return -EACCES;
+	ret = -EACCES;
+
+out_release_group:
+	put_group_info(group_info);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ping_init_sock);
 
-- 
1.7.1

[PATCH] net: ipv4: current group_info should be put after using.

[Wang, Xiaoming]

There is a memory leak in ping. Current group_info had been got in 
ping_init_sock and group_info->usage increased. 
But the usage hasn't decreased anywhere in ping.
This will make this group_info never freed and cause memory leak.

unreferenced object 0xcd0e8840 (size 192):
  comm "dumpstate", pid 7583, jiffies 78360 (age 91.810s)
  hex dump (first 32 bytes):
    02 00 00 00 06 00 00 00 01 00 00 00 ef 03 00 00  ................
    f1 03 00 00 f7 03 00 00 04 04 00 00 bb 0b 00 00  ................
  backtrace:
    [<c1a6bbfc>] kmemleak_alloc+0x3c/0xa0
    [<c1320457>] __kmalloc+0xe7/0x1d0
    [<c1267c04>] groups_alloc+0x34/0xb0
    [<c1267e5c>] SyS_setgroups+0x3c/0xf0
    [<c1a864a8>] syscall_call+0x7/0xb
    [<ffffffff>] 0xffffffff

Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
---
 net/ipv4/ping.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index f4b19e5..2af7b1f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -255,23 +255,28 @@ int ping_init_sock(struct sock *sk)
 	struct group_info *group_info = get_current_groups();
 	int i, j, count = group_info->ngroups;
 	kgid_t low, high;
+	int ret = 0;
 
 	inet_get_ping_group_range_net(net, &low, &high);
 	if (gid_lte(low, group) && gid_lte(group, high))
-		return 0;
+		goto out_release_group;
 
 	for (i = 0; i < group_info->nblocks; i++) {
 		int cp_count = min_t(int, NGROUPS_PER_BLOCK, count);
 		for (j = 0; j < cp_count; j++) {
 			kgid_t gid = group_info->blocks[i][j];
 			if (gid_lte(low, gid) && gid_lte(gid, high))
-				return 0;
+				goto out_release_group;
 		}
 
 		count -= cp_count;
 	}
 
-	return -EACCES;
+	ret = -EACCES;
+
+out_release_group:
+	put_group_info(group_info);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ping_init_sock);
 
-- 
1.7.1

Re: [PATCH] net: ipv4: current group_info should be put after using.

[Mateusz Guzik]

On Fri, Apr 11, 2014 at 10:53:21PM -0400, Wang, Xiaoming wrote:
> This is a typical refcount leak exploitable by unprivileged users.
> Current group_info had been got in ping_init_sock and
> group_info->usage increased. But the usage hasn't decreased
> anywhere in ping. This will make this group_info never freed.
> 

The patch is fine, however I had a brainfart with my last sentence about
commit message, sorry for that.

group_info *can be freed* by malicious user while still being pointed to
by something, that's the biggest problem with refcount leaks, therefore
this message needs some reworking.

I think that discussion about various consequences of refcount leak in
commit message is not necessary.

how about:

Plug a group_info refcount leak in ping_init.

group_info is only needed during initialization and the code failed to
release the reference on exit.

While here move grabbing the reference to a place where it is actually
needed.

====

Please cc: me if you resend the patch.

Thanks,
-- 
Mateusz Guzik

Re: [PATCH] net: ipv4: current group_info should be put after using.

[Mateusz Guzik]

On Fri, Apr 11, 2014 at 10:35:33AM +0200, Mateusz Guzik wrote:
> On Fri, Apr 11, 2014 at 01:37:08PM -0400, Wang, Xiaoming wrote:
> > There is a memory leak in ping. Current group_info had been got in 
> > ping_init_sock and group_info->usage increased. 
> > But the usage hasn't decreased anywhere in ping.
> > This will make this group_info never freed and cause memory leak.
> > 
> 
> Memory leak is only one of possible side-effects, thus I believe commit
> message should be adjusted.
> 
> This is a typical refcount leak exploitable by unprivileged users, so
> side effects can range from nothing through memory leaks and crashes to
> possibly privilege escalation.
> 
> That said losing ' and cause memory leak' from your commit message
> would be fine in my opinion.
> 

Huh, not sure why I wrote that last sentence, it does not make much
sense. Sorry.

There is a pending CVE request for this bug:
http://seclists.org/oss-sec/2014/q2/97

The bug was introduced with:
commit c319b4d76b9e583a5d88d6bf190e079c4e43213d
Author: Vasiliy Kulikov <segoon@openwall.com>
Date:   Fri May 13 10:01:00 2011 +0000

    net: ipv4: add IPPROTO_ICMP socket kind

starting with 3.0 kernel.


> See also a nit below.
> 
> > ---
> >  net/ipv4/ping.c |   11 ++++++++---
> >  1 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> > index f4b19e5..2af7b1f 100644
> > --- a/net/ipv4/ping.c
> > +++ b/net/ipv4/ping.c
> > @@ -255,23 +255,28 @@ int ping_init_sock(struct sock *sk)
> >  	struct group_info *group_info = get_current_groups();
> >  	int i, j, count = group_info->ngroups;
> >  	kgid_t low, high;
> > +	int ret = 0;
> >  
> >  	inet_get_ping_group_range_net(net, &low, &high);
> >  	if (gid_lte(low, group) && gid_lte(group, high))
> > -		return 0;
> > +		goto out_release_group;
> >  
> 
> Since group_info is not even used here maybe it would be better to leave
> return 0 as it is and call get_current_groups before the loop?
> 
> >  	for (i = 0; i < group_info->nblocks; i++) {
> >  		int cp_count = min_t(int, NGROUPS_PER_BLOCK, count);
> >  		for (j = 0; j < cp_count; j++) {
> >  			kgid_t gid = group_info->blocks[i][j];
> >  			if (gid_lte(low, gid) && gid_lte(gid, high))
> > -				return 0;
> > +				goto out_release_group;
> >  		}
> >  
> >  		count -= cp_count;
> >  	}
> >  
> > -	return -EACCES;
> > +	ret = -EACCES;
> > +
> > +out_release_group:
> > +	put_group_info(group_info);
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(ping_init_sock);
> >  
> 
> -- 
> Mateusz Guzik

-- 
Mateusz Guzik

Re: [PATCH] net: ipv4: current group_info should be put after using.

[Mateusz Guzik]

On Fri, Apr 11, 2014 at 01:37:08PM -0400, Wang, Xiaoming wrote:
> There is a memory leak in ping. Current group_info had been got in 
> ping_init_sock and group_info->usage increased. 
> But the usage hasn't decreased anywhere in ping.
> This will make this group_info never freed and cause memory leak.
> 

Memory leak is only one of possible side-effects, thus I believe commit
message should be adjusted.

This is a typical refcount leak exploitable by unprivileged users, so
side effects can range from nothing through memory leaks and crashes to
possibly privilege escalation.

That said losing ' and cause memory leak' from your commit message
would be fine in my opinion.

See also a nit below.

> ---
>  net/ipv4/ping.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index f4b19e5..2af7b1f 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -255,23 +255,28 @@ int ping_init_sock(struct sock *sk)
>  	struct group_info *group_info = get_current_groups();
>  	int i, j, count = group_info->ngroups;
>  	kgid_t low, high;
> +	int ret = 0;
>  
>  	inet_get_ping_group_range_net(net, &low, &high);
>  	if (gid_lte(low, group) && gid_lte(group, high))
> -		return 0;
> +		goto out_release_group;
>  

Since group_info is not even used here maybe it would be better to leave
return 0 as it is and call get_current_groups before the loop?

>  	for (i = 0; i < group_info->nblocks; i++) {
>  		int cp_count = min_t(int, NGROUPS_PER_BLOCK, count);
>  		for (j = 0; j < cp_count; j++) {
>  			kgid_t gid = group_info->blocks[i][j];
>  			if (gid_lte(low, gid) && gid_lte(gid, high))
> -				return 0;
> +				goto out_release_group;
>  		}
>  
>  		count -= cp_count;
>  	}
>  
> -	return -EACCES;
> +	ret = -EACCES;
> +
> +out_release_group:
> +	put_group_info(group_info);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(ping_init_sock);
>  

-- 
Mateusz Guzik