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

login
register
mail settings
Submitter Tomasz Bursztyka
Date June 28, 2012, 12:57 p.m.
Message ID <1340888269-4926-2-git-send-email-tomasz.bursztyka@linux.intel.com>
Download mbox | patch
Permalink /patch/167872/
State Accepted
Headers show

Comments

Tomasz Bursztyka - June 28, 2012, 12:57 p.m.
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(-)
Pablo Neira - June 29, 2012, 11:15 a.m.
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.
--
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
Jan Engelhardt - June 29, 2012, 7:03 p.m.
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.
--
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
Florian Westphal - June 30, 2012, 8:54 a.m.
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.
--
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
Pablo Neira - July 4, 2012, 8:54 p.m.
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.
--
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

Patch

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)