| Message ID | 1340888269-4926-2-git-send-email-tomasz.bursztyka@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
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
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)
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(-)