[PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[Jiri Slaby]

Stanse found that one error path in netpoll_setup dereferences npinfo
even though it is NULL. Avoid that by adding new label and go to that
instead.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Daniel Borkmann <danborkmann@googlemail.com>
Cc: David S. Miller <davem@davemloft.net>
---
 net/core/netpoll.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 7aa6972..d4ec38f 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np)
 		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
 		if (!npinfo) {
 			err = -ENOMEM;
-			goto release;
+			goto put;
 		}
 
 		npinfo->rx_flags = 0;
@@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np)
 
 		kfree(npinfo);
 	}
-
+put:
 	dev_put(ndev);
 	return err;
 }
-- 
1.7.0.1

Re: [PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[Laurent Chavey]

Acked-by: chavey@google.com

On Tue, Mar 16, 2010 at 8:29 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> Stanse found that one error path in netpoll_setup dereferences npinfo
> even though it is NULL. Avoid that by adding new label and go to that
> instead.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Daniel Borkmann <danborkmann@googlemail.com>
> Cc: David S. Miller <davem@davemloft.net>
> ---
>  net/core/netpoll.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 7aa6972..d4ec38f 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np)
>                npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>                if (!npinfo) {
>                        err = -ENOMEM;
> -                       goto release;
> +                       goto put;
>                }
>
>                npinfo->rx_flags = 0;
> @@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np)
>
>                kfree(npinfo);
>        }
> -
> +put:
>        dev_put(ndev);
>        return err;
>  }
> --
> 1.7.0.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[Matt Mackall]

On Tue, 2010-03-16 at 16:29 +0100, Jiri Slaby wrote:
> Stanse found that one error path in netpoll_setup dereferences npinfo
> even though it is NULL. Avoid that by adding new label and go to that
> instead.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Daniel Borkmann <danborkmann@googlemail.com>
> Cc: David S. Miller <davem@davemloft.net>
> ---
>  net/core/netpoll.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 7aa6972..d4ec38f 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np)
>  		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>  		if (!npinfo) {
>  			err = -ENOMEM;
> -			goto release;
> +			goto put;
>  		}
>  
>  		npinfo->rx_flags = 0;
> @@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np)
>  
>  		kfree(npinfo);
>  	}
> -
> +put:
>  	dev_put(ndev);
>  	return err;
>  }

I don't get it. The source of the branch tests for !ndev->npinfo and the
original destination of the branch also tests for !ndev->npinfo. I don't
see how it gets dereferenced.

This looks like it just patches over a false positive in your tool
(which isn't correlating the validity of npinfo with ndev->npinfo)
without actually improving the code. However, it seems that we can drop
the second check at release if we add your new exit point.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[Jiri Slaby]

On 03/16/2010 06:12 PM, Matt Mackall wrote:
> I don't get it. The source of the branch tests for !ndev->npinfo and the
> original destination of the branch also tests for !ndev->npinfo. I don't
> see how it gets dereferenced.

Let's look at more of the context:
         if (!ndev->npinfo) {
                 npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
                 if (!npinfo) {         // npinfo is NULL
                         err = -ENOMEM;
                         goto release;
                 }
...
release:                           // npinfo is still NULL
         if (!ndev->npinfo) {       // condition is the same (holds)
              // dereference below: vvvvvvvvvvvvvvv
                 spin_lock_irqsave(&npinfo->rx_lock, flags);
                 list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) {
                         npe->dev = NULL;
                 }
                 spin_unlock_irqrestore(&npinfo->rx_lock, flags);

                 kfree(npinfo);
         }

thanks,
-- 
js

Re: [PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[Matt Mackall]

On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote:
> On 03/16/2010 06:12 PM, Matt Mackall wrote:
> > I don't get it. The source of the branch tests for !ndev->npinfo and the
> > original destination of the branch also tests for !ndev->npinfo. I don't
> > see how it gets dereferenced.
> 
> Let's look at more of the context:
>          if (!ndev->npinfo) {
>                  npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>                  if (!npinfo) {         // npinfo is NULL
>                          err = -ENOMEM;
>                          goto release;
>                  }
> ...
> release:                           // npinfo is still NULL
>          if (!ndev->npinfo) {       // condition is the same (holds)
>               // dereference below: vvvvvvvvvvvvvvv
>                  spin_lock_irqsave(&npinfo->rx_lock, flags);
>                  list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) {
>                          npe->dev = NULL;
>                  }
>                  spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> 
>                  kfree(npinfo);
>          }

Ok, you're correct, I read the second test backwards.

Acked-by: Matt Mackall <mpm@selenic.com>

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[David Miller]

From: Matt Mackall <mpm@selenic.com>
Date: Tue, 16 Mar 2010 12:56:00 -0500

> On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote:
>> On 03/16/2010 06:12 PM, Matt Mackall wrote:
>> > I don't get it. The source of the branch tests for !ndev->npinfo and the
>> > original destination of the branch also tests for !ndev->npinfo. I don't
>> > see how it gets dereferenced.
>> 
>> Let's look at more of the context:
>>          if (!ndev->npinfo) {
>>                  npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>>                  if (!npinfo) {         // npinfo is NULL
>>                          err = -ENOMEM;
>>                          goto release;
>>                  }
>> ...
>> release:                           // npinfo is still NULL
>>          if (!ndev->npinfo) {       // condition is the same (holds)
>>               // dereference below: vvvvvvvvvvvvvvv
>>                  spin_lock_irqsave(&npinfo->rx_lock, flags);
>>                  list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) {
>>                          npe->dev = NULL;
>>                  }
>>                  spin_unlock_irqrestore(&npinfo->rx_lock, flags);
>> 
>>                  kfree(npinfo);
>>          }
> 
> Ok, you're correct, I read the second test backwards.
> 
> Acked-by: Matt Mackall <mpm@selenic.com>

Applied, thanks everyone.

Re: [PATCH 1/1] NET: netpoll, fix potential NULL ptr dereference

[Daniel Borkmann]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

Matt Mackall wrote:
> On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote:
>> On 03/16/2010 06:12 PM, Matt Mackall wrote:
>>> I don't get it. The source of the branch tests for !ndev->npinfo and the
>>> original destination of the branch also tests for !ndev->npinfo. I don't
>>> see how it gets dereferenced.
>> Let's look at more of the context:
>>          if (!ndev->npinfo) {
>>                  npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>>                  if (!npinfo) {         // npinfo is NULL
>>                          err = -ENOMEM;
>>                          goto release;
>>                  }
>> ...
>> release:                           // npinfo is still NULL
>>          if (!ndev->npinfo) {       // condition is the same (holds)
>>               // dereference below: vvvvvvvvvvvvvvv
>>                  spin_lock_irqsave(&npinfo->rx_lock, flags);
>>                  list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) {
>>                          npe->dev = NULL;
>>                  }
>>                  spin_unlock_irqrestore(&npinfo->rx_lock, flags);
>>
>>                  kfree(npinfo);
>>          }
> 
> Ok, you're correct, I read the second test backwards.
> 
> Acked-by: Matt Mackall <mpm@selenic.com>
> 

Thanks for fixing this and sorry for not being responsive, obviously it
sucks when you have a broken leg and German hospitals do not really have
Internet access ... ;)

Thanks,
Daniel


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]