[PATCH 0/3] Nfnetlink and ipset fixes

[PATCH 0/3] Nfnetlink and ipset fixes

[Tomasz Bursztyka]

Hi,

The 2 first patches fixes the same issue, but in 2 different place.

Patch 1:

So here is how the issue can be reprocuced:
Create a netlink message, targeting ipset subsystem. And send a command on id 0 (IPSET_CMD_NONE):
This will lead to a crash on line 188 of nfnetlink.c since nc->call is NULL (so you get a NULL dereference).

Or Patch 2:

Same issue.
It fixes ipset to handle the IPSET_CMD_NONE, return -EOPNOTSUPP.

Patch 3:

While doing patch 1 I figured out rcu_read_unlock() is not called in case nla_parse() error, so it fixes it.

Br,

Tomasz Bursztyka (3):
  nfnetlink: Check callbacks before using those in nfnetlink_rcv_msg
  ipset: Handle properly an IPSET_CMD_NONE
  nfnetlink: Unlock a previously locked rcu_read in nfnetlink_rcv_msg

 net/netfilter/ipset/ip_set_core.c |   12 ++++++++++++
 net/netfilter/nfnetlink.c         |    8 ++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

-- 
1.7.8.6

[PATCH 1/3] nfnetlink: Check callbacks before using those in nfnetlink_rcv_msg

[Tomasz Bursztyka]

nfnetlink_rcv_msg() might call a NULL callback which will cause NULL pointer
dereference.

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 net/netfilter/nfnetlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 3e797d1..4acdd76 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -184,9 +184,11 @@ replay:
 					lockdep_is_held(&nfnl_mutex)) != ss ||
 			    nfnetlink_find_client(type, ss) != nc)
 				err = -EAGAIN;
-			else
+			else if (nc->call)
 				err = nc->call(net->nfnl, skb, nlh,
 						   (const struct nlattr **)cda);
+			else
+				err = -EINVAL;
 			nfnl_unlock();
 		}
 		if (err == -EAGAIN)
-- 
1.7.8.6

Re: [PATCH 1/3] nfnetlink: Check callbacks before using those in nfnetlink_rcv_msg

[Pablo Neira Ayuso]

On Thu, Jun 28, 2012 at 03:57:47PM +0300, Tomasz Bursztyka wrote:
> nfnetlink_rcv_msg() might call a NULL callback which will cause NULL pointer
> dereference.
> 
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  net/netfilter/nfnetlink.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> index 3e797d1..4acdd76 100644
> --- a/net/netfilter/nfnetlink.c
> +++ b/net/netfilter/nfnetlink.c
> @@ -184,9 +184,11 @@ replay:
>  					lockdep_is_held(&nfnl_mutex)) != ss ||
>  			    nfnetlink_find_client(type, ss) != nc)
>  				err = -EAGAIN;
> -			else
> +			else if (nc->call)
>  				err = nc->call(net->nfnl, skb, nlh,
>  						   (const struct nlattr **)cda);
> +			else
> +				err = -EINVAL;

Hm, I think this is redundant. We got the ipset problem fixed with
2/3. I think we can add some BUG_ON instead as all nc->call needs to
be defined. Otherwise, spot a bug message so it is fixed.

Re: [PATCH 1/3] nfnetlink: Check callbacks before using those in nfnetlink_rcv_msg

[Jan Engelhardt]

On Friday 2012-06-29 13:15, Pablo Neira Ayuso wrote:
>On Thu, Jun 28, 2012 at 03:57:47PM +0300, Tomasz Bursztyka wrote:
>> nfnetlink_rcv_msg() might call a NULL callback which will cause NULL pointer
>> dereference.
>> 
>> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
>> ---
>> @@ -184,9 +184,11 @@ replay:
>>  					lockdep_is_held(&nfnl_mutex)) != ss ||
>>  			    nfnetlink_find_client(type, ss) != nc)
>>  				err = -EAGAIN;
>> -			else
>> +			else if (nc->call)
>>  				err = nc->call(net->nfnl, skb, nlh,
>>  						   (const struct nlattr **)cda);
>> +			else
>> +				err = -EINVAL;
>
>Hm, I think this is redundant. We got the ipset problem fixed with
>2/3. I think we can add some BUG_ON instead as all nc->call needs to
>be defined. Otherwise, spot a bug message so it is fixed.

Seriously, we have run into NULL dereferences at this place so often
that it should not be discarded as a PEBKAC issue. Yes, it is the
programmer's responsibility to fill in .call, but error handling is
anytime better than driving one's machine into the pit stop due to a
NULL deref or BUG_ON.

Re: [PATCH 1/3] nfnetlink: Check callbacks before using those in nfnetlink_rcv_msg

[Florian Westphal]

Jan Engelhardt <jengelh@inai.de> wrote:
> On Friday 2012-06-29 13:15, Pablo Neira Ayuso wrote:
> >On Thu, Jun 28, 2012 at 03:57:47PM +0300, Tomasz Bursztyka wrote:
> >> ---
> >> @@ -184,9 +184,11 @@ replay:
> >>  					lockdep_is_held(&nfnl_mutex)) != ss ||
> >>  			    nfnetlink_find_client(type, ss) != nc)
> >>  				err = -EAGAIN;
> >> -			else
> >> +			else if (nc->call)
> >>  				err = nc->call(net->nfnl, skb, nlh,
> >>  						   (const struct nlattr **)cda);
> >> +			else
> >> +				err = -EINVAL;
> >
> >Hm, I think this is redundant. We got the ipset problem fixed with
> >2/3. I think we can add some BUG_ON instead as all nc->call needs to
> >be defined. Otherwise, spot a bug message so it is fixed.
> 
> Seriously, we have run into NULL dereferences at this place so often
> that it should not be discarded as a PEBKAC issue. Yes, it is the
> programmer's responsibility to fill in .call, but error handling is
> anytime better than driving one's machine into the pit stop due to a
> NULL deref or BUG_ON.

Agreed.  I think this should be WARN_ON_ONCE + ret EINVAL.
We also do this with various places that e.g. fill netlink
messages so that adding new attributes without bumping allocation
sizes is reported without crashes.  Since we've started to move
the performance-critical nfnetlink stuff over to ->call_rcu
the extra NULL test should not harm fastpath either.

Re: [PATCH 1/3] nfnetlink: Check callbacks before using those in nfnetlink_rcv_msg

[Pablo Neira Ayuso]

On Sat, Jun 30, 2012 at 10:54:25AM +0200, Florian Westphal wrote:
> Jan Engelhardt <jengelh@inai.de> wrote:
> > On Friday 2012-06-29 13:15, Pablo Neira Ayuso wrote:
> > >On Thu, Jun 28, 2012 at 03:57:47PM +0300, Tomasz Bursztyka wrote:
> > >> ---
> > >> @@ -184,9 +184,11 @@ replay:
> > >>  					lockdep_is_held(&nfnl_mutex)) != ss ||
> > >>  			    nfnetlink_find_client(type, ss) != nc)
> > >>  				err = -EAGAIN;
> > >> -			else
> > >> +			else if (nc->call)
> > >>  				err = nc->call(net->nfnl, skb, nlh,
> > >>  						   (const struct nlattr **)cda);
> > >> +			else
> > >> +				err = -EINVAL;
> > >
> > >Hm, I think this is redundant. We got the ipset problem fixed with
> > >2/3. I think we can add some BUG_ON instead as all nc->call needs to
> > >be defined. Otherwise, spot a bug message so it is fixed.
> > 
> > Seriously, we have run into NULL dereferences at this place so often
> > that it should not be discarded as a PEBKAC issue. Yes, it is the
> > programmer's responsibility to fill in .call, but error handling is
> > anytime better than driving one's machine into the pit stop due to a
> > NULL deref or BUG_ON.
> 
> Agreed.  I think this should be WARN_ON_ONCE + ret EINVAL.
> We also do this with various places that e.g. fill netlink
> messages so that adding new attributes without bumping allocation
> sizes is reported without crashes.  Since we've started to move
> the performance-critical nfnetlink stuff over to ->call_rcu
> the extra NULL test should not harm fastpath either.

OK, fair enough. I've enqueued this to nf-next.

[PATCH 2/3] ipset: Handle properly an IPSET_CMD_NONE

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 net/netfilter/ipset/ip_set_core.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 819c342..9730882 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -640,6 +640,14 @@ find_free_id(const char *name, ip_set_id_t *index, struct ip_set **set)
 }
 
 static int
+ip_set_none(struct sock *ctnl, struct sk_buff *skb,
+	    const struct nlmsghdr *nlh,
+	    const struct nlattr * const attr[])
+{
+	return -EOPNOTSUPP;
+}
+
+static int
 ip_set_create(struct sock *ctnl, struct sk_buff *skb,
 	      const struct nlmsghdr *nlh,
 	      const struct nlattr * const attr[])
@@ -1539,6 +1547,10 @@ nlmsg_failure:
 }
 
 static const struct nfnl_callback ip_set_netlink_subsys_cb[IPSET_MSG_MAX] = {
+	[IPSET_CMD_NONE]	= {
+		.call		= ip_set_none,
+		.attr_count	= IPSET_ATTR_CMD_MAX,
+	},
 	[IPSET_CMD_CREATE]	= {
 		.call		= ip_set_create,
 		.attr_count	= IPSET_ATTR_CMD_MAX,
-- 
1.7.8.6

Re: [PATCH 2/3] ipset: Handle properly an IPSET_CMD_NONE

[Jozsef Kadlecsik]

On Thu, 28 Jun 2012, Tomasz Bursztyka wrote:

> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

Never occured to me to send IPSET_CMD_NONE :-).

Best regards,
Jozsef
> ---
>  net/netfilter/ipset/ip_set_core.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index 819c342..9730882 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -640,6 +640,14 @@ find_free_id(const char *name, ip_set_id_t *index, struct ip_set **set)
>  }
>  
>  static int
> +ip_set_none(struct sock *ctnl, struct sk_buff *skb,
> +	    const struct nlmsghdr *nlh,
> +	    const struct nlattr * const attr[])
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int
>  ip_set_create(struct sock *ctnl, struct sk_buff *skb,
>  	      const struct nlmsghdr *nlh,
>  	      const struct nlattr * const attr[])
> @@ -1539,6 +1547,10 @@ nlmsg_failure:
>  }
>  
>  static const struct nfnl_callback ip_set_netlink_subsys_cb[IPSET_MSG_MAX] = {
> +	[IPSET_CMD_NONE]	= {
> +		.call		= ip_set_none,
> +		.attr_count	= IPSET_ATTR_CMD_MAX,
> +	},
>  	[IPSET_CMD_CREATE]	= {
>  		.call		= ip_set_create,
>  		.attr_count	= IPSET_ATTR_CMD_MAX,
> -- 
> 1.7.8.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH 2/3] ipset: Handle properly an IPSET_CMD_NONE

[Pablo Neira Ayuso]

On Thu, Jun 28, 2012 at 04:40:30PM +0200, Jozsef Kadlecsik wrote:
> On Thu, 28 Jun 2012, Tomasz Bursztyka wrote:
> 
> > Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> 
> Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> 
> Never occured to me to send IPSET_CMD_NONE :-).

Applied, thanks.

[PATCH 3/3] nfnetlink: Unlock a previously locked rcu_read in nfnetlink_rcv_msg

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 net/netfilter/nfnetlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 4acdd76..a6366ae 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -169,8 +169,10 @@ replay:
 
 		err = nla_parse(cda, ss->cb[cb_id].attr_count,
 				attr, attrlen, ss->cb[cb_id].policy);
-		if (err < 0)
+		if (err < 0) {
+			rcu_read_unlock();
 			return err;
+		}
 
 		if (nc->call_rcu) {
 			err = nc->call_rcu(net->nfnl, skb, nlh,
-- 
1.7.8.6

Re: [PATCH 3/3] nfnetlink: Unlock a previously locked rcu_read in nfnetlink_rcv_msg

[Eric Dumazet]

On Thu, 2012-06-28 at 15:57 +0300, Tomasz Bursztyka wrote:
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  net/netfilter/nfnetlink.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> index 4acdd76..a6366ae 100644
> --- a/net/netfilter/nfnetlink.c
> +++ b/net/netfilter/nfnetlink.c
> @@ -169,8 +169,10 @@ replay:
>  
>  		err = nla_parse(cda, ss->cb[cb_id].attr_count,
>  				attr, attrlen, ss->cb[cb_id].policy);
> -		if (err < 0)
> +		if (err < 0) {
> +			rcu_read_unlock();
>  			return err;
> +		}
>  
>  		if (nc->call_rcu) {
>  			err = nc->call_rcu(net->nfnl, skb, nlh,

Nice catch, thanks.

Please someone add in changelog to ease stable team work :

Bug added in commit 6b75e3e8d664a9a (netfilter: nfnetlink: add RCU in
nfnetlink_rcv_msg())

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 3/3] nfnetlink: Unlock a previously locked rcu_read in nfnetlink_rcv_msg

[Pablo Neira Ayuso]

On Thu, Jun 28, 2012 at 03:30:10PM +0200, Eric Dumazet wrote:
> On Thu, 2012-06-28 at 15:57 +0300, Tomasz Bursztyka wrote:
> > Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> > ---
> >  net/netfilter/nfnetlink.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> > index 4acdd76..a6366ae 100644
> > --- a/net/netfilter/nfnetlink.c
> > +++ b/net/netfilter/nfnetlink.c
> > @@ -169,8 +169,10 @@ replay:
> >  
> >  		err = nla_parse(cda, ss->cb[cb_id].attr_count,
> >  				attr, attrlen, ss->cb[cb_id].policy);
> > -		if (err < 0)
> > +		if (err < 0) {
> > +			rcu_read_unlock();
> >  			return err;
> > +		}
> >  
> >  		if (nc->call_rcu) {
> >  			err = nc->call_rcu(net->nfnl, skb, nlh,
> 
> Nice catch, thanks.
> 
> Please someone add in changelog to ease stable team work :
> 
> Bug added in commit 6b75e3e8d664a9a (netfilter: nfnetlink: add RCU in
> nfnetlink_rcv_msg())
> 
> Acked-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.