* [PATCH nf-next v3 0/2] fixes for recent nf_compact hooks @ 2016-09-28 13:12 Aaron Conole 2016-09-28 13:12 ` [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference Aaron Conole 2016-09-28 13:12 ` [PATCH nf-next v3 2/2] nf_set_hooks_head: accommodate different kconfig Aaron Conole 0 siblings, 2 replies; 7+ messages in thread From: Aaron Conole @ 2016-09-28 13:12 UTC (permalink / raw) To: netfilter-devel, netdev; +Cc: Florian Westphal, Pablo Neira Ayuso Two possible error conditions were caught during an extended testing session, and by a build robot. These patches fix the two issues (a missing handler when config is changed, and a potential NULL dereference). Aaron Conole (2): netfilter: Fix potential null pointer dereference nf_set_hooks_head: acommodate different kconfig net/netfilter/core.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) -- 2.5.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 13:12 [PATCH nf-next v3 0/2] fixes for recent nf_compact hooks Aaron Conole @ 2016-09-28 13:12 ` Aaron Conole 2016-09-28 13:35 ` Eric Dumazet 2016-09-28 13:12 ` [PATCH nf-next v3 2/2] nf_set_hooks_head: accommodate different kconfig Aaron Conole 1 sibling, 1 reply; 7+ messages in thread From: Aaron Conole @ 2016-09-28 13:12 UTC (permalink / raw) To: netfilter-devel, netdev; +Cc: Florian Westphal, Pablo Neira Ayuso It's possible for nf_hook_entry_head to return NULL. If two nf_unregister_net_hook calls happen simultaneously with a single hook entry in the list, both will enter the nf_hook_mutex critical section. The first will successfully delete the head, but the second will see this NULL pointer and attempt to dereference. This fix ensures that no null pointer dereference could occur when such a condition happens. Signed-off-by: Aaron Conole <aconole@bytheb.org> --- net/netfilter/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/netfilter/core.c b/net/netfilter/core.c index 360c63d..e58e420 100644 --- a/net/netfilter/core.c +++ b/net/netfilter/core.c @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg) mutex_lock(&nf_hook_mutex); hooks_entry = nf_hook_entry_head(net, reg); - if (hooks_entry->orig_ops == reg) { + if (hooks_entry && hooks_entry->orig_ops == reg) { nf_set_hooks_head(net, reg, nf_entry_dereference(hooks_entry->next)); goto unlock; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 13:12 ` [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference Aaron Conole @ 2016-09-28 13:35 ` Eric Dumazet 2016-09-28 14:56 ` Aaron Conole 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2016-09-28 13:35 UTC (permalink / raw) To: Aaron Conole; +Cc: netfilter-devel, netdev, Florian Westphal, Pablo Neira Ayuso On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote: > It's possible for nf_hook_entry_head to return NULL. If two > nf_unregister_net_hook calls happen simultaneously with a single hook > entry in the list, both will enter the nf_hook_mutex critical section. > The first will successfully delete the head, but the second will see > this NULL pointer and attempt to dereference. > > This fix ensures that no null pointer dereference could occur when such > a condition happens. > > Signed-off-by: Aaron Conole <aconole@bytheb.org> > --- > net/netfilter/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/netfilter/core.c b/net/netfilter/core.c > index 360c63d..e58e420 100644 > --- a/net/netfilter/core.c > +++ b/net/netfilter/core.c > @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg) > > mutex_lock(&nf_hook_mutex); > hooks_entry = nf_hook_entry_head(net, reg); > - if (hooks_entry->orig_ops == reg) { > + if (hooks_entry && hooks_entry->orig_ops == reg) { > nf_set_hooks_head(net, reg, > nf_entry_dereference(hooks_entry->next)); > goto unlock; When was the bug added exactly ? For all bug fixes, you need to add a Fixes: tag. Like : Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") So that 100 different people in stable teams do not have to do the archeology themselves ... Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 13:35 ` Eric Dumazet @ 2016-09-28 14:56 ` Aaron Conole 2016-09-28 15:30 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Aaron Conole @ 2016-09-28 14:56 UTC (permalink / raw) To: Eric Dumazet; +Cc: netfilter-devel, netdev, Florian Westphal, Pablo Neira Ayuso Eric Dumazet <eric.dumazet@gmail.com> writes: > On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote: >> It's possible for nf_hook_entry_head to return NULL. If two >> nf_unregister_net_hook calls happen simultaneously with a single hook >> entry in the list, both will enter the nf_hook_mutex critical section. >> The first will successfully delete the head, but the second will see >> this NULL pointer and attempt to dereference. >> >> This fix ensures that no null pointer dereference could occur when such >> a condition happens. >> >> Signed-off-by: Aaron Conole <aconole@bytheb.org> >> --- >> net/netfilter/core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/netfilter/core.c b/net/netfilter/core.c >> index 360c63d..e58e420 100644 >> --- a/net/netfilter/core.c >> +++ b/net/netfilter/core.c >> @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg) >> >> mutex_lock(&nf_hook_mutex); >> hooks_entry = nf_hook_entry_head(net, reg); >> - if (hooks_entry->orig_ops == reg) { >> + if (hooks_entry && hooks_entry->orig_ops == reg) { >> nf_set_hooks_head(net, reg, >> nf_entry_dereference(hooks_entry->next)); >> goto unlock; > > When was the bug added exactly ? Sunday, on the nf-next tree. > For all bug fixes, you need to add a Fixes: tag. > > Like : > > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") I would but it's in nf-next tree, and I'm not sure how pulls go. If they are done via patch imports, then the sha sums will be wrong and the commit message will be misleading. If the sums are preserved, then I can resubmit with this information. > So that 100 different people in stable teams do not have to do the > archeology themselves ... > > Thanks. Thanks for the review, Eric. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 14:56 ` Aaron Conole @ 2016-09-28 15:30 ` Eric Dumazet 2016-09-28 15:38 ` Aaron Conole 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2016-09-28 15:30 UTC (permalink / raw) To: Aaron Conole; +Cc: netfilter-devel, netdev, Florian Westphal, Pablo Neira Ayuso On Wed, 2016-09-28 at 10:56 -0400, Aaron Conole wrote: > Eric Dumazet <eric.dumazet@gmail.com> writes: > > > On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote: > >> It's possible for nf_hook_entry_head to return NULL. If two > >> nf_unregister_net_hook calls happen simultaneously with a single hook > >> entry in the list, both will enter the nf_hook_mutex critical section. > >> The first will successfully delete the head, but the second will see > >> this NULL pointer and attempt to dereference. > >> > >> This fix ensures that no null pointer dereference could occur when such > >> a condition happens. > >> > >> Signed-off-by: Aaron Conole <aconole@bytheb.org> > >> --- > >> net/netfilter/core.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/net/netfilter/core.c b/net/netfilter/core.c > >> index 360c63d..e58e420 100644 > >> --- a/net/netfilter/core.c > >> +++ b/net/netfilter/core.c > >> @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg) > >> > >> mutex_lock(&nf_hook_mutex); > >> hooks_entry = nf_hook_entry_head(net, reg); > >> - if (hooks_entry->orig_ops == reg) { > >> + if (hooks_entry && hooks_entry->orig_ops == reg) { > >> nf_set_hooks_head(net, reg, > >> nf_entry_dereference(hooks_entry->next)); > >> goto unlock; > > > > When was the bug added exactly ? > > Sunday, on the nf-next tree. > > > For all bug fixes, you need to add a Fixes: tag. > > > > Like : > > > > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") > > I would but it's in nf-next tree, and I'm not sure how pulls go. If > they are done via patch imports, then the sha sums will be wrong and the > commit message will be misleading. If the sums are preserved, then I > can resubmit with this information. > I gave the (12 digits) sha-1 as present in David Miller net-next tree. https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=e3b37f11e6e4e6b6f02cc762f182ce233d2c1c9d This wont change, because David never rebases his tree under normal operations. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 15:30 ` Eric Dumazet @ 2016-09-28 15:38 ` Aaron Conole 0 siblings, 0 replies; 7+ messages in thread From: Aaron Conole @ 2016-09-28 15:38 UTC (permalink / raw) To: Eric Dumazet; +Cc: netfilter-devel, netdev, Florian Westphal, Pablo Neira Ayuso Eric Dumazet <eric.dumazet@gmail.com> writes: > On Wed, 2016-09-28 at 10:56 -0400, Aaron Conole wrote: >> Eric Dumazet <eric.dumazet@gmail.com> writes: >> >> > On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote: >> >> It's possible for nf_hook_entry_head to return NULL. If two >> >> nf_unregister_net_hook calls happen simultaneously with a single hook >> >> entry in the list, both will enter the nf_hook_mutex critical section. >> >> The first will successfully delete the head, but the second will see >> >> this NULL pointer and attempt to dereference. >> >> >> >> This fix ensures that no null pointer dereference could occur when such >> >> a condition happens. >> >> >> >> Signed-off-by: Aaron Conole <aconole@bytheb.org> >> >> --- >> >> net/netfilter/core.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/net/netfilter/core.c b/net/netfilter/core.c >> >> index 360c63d..e58e420 100644 >> >> --- a/net/netfilter/core.c >> >> +++ b/net/netfilter/core.c >> >> @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg) >> >> >> >> mutex_lock(&nf_hook_mutex); >> >> hooks_entry = nf_hook_entry_head(net, reg); >> >> - if (hooks_entry->orig_ops == reg) { >> >> + if (hooks_entry && hooks_entry->orig_ops == reg) { >> >> nf_set_hooks_head(net, reg, >> >> nf_entry_dereference(hooks_entry->next)); >> >> goto unlock; >> > >> > When was the bug added exactly ? >> >> Sunday, on the nf-next tree. >> >> > For all bug fixes, you need to add a Fixes: tag. >> > >> > Like : >> > >> > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") >> >> I would but it's in nf-next tree, and I'm not sure how pulls go. If >> they are done via patch imports, then the sha sums will be wrong and the >> commit message will be misleading. If the sums are preserved, then I >> can resubmit with this information. >> > > I gave the (12 digits) sha-1 as present in David Miller net-next tree. > > https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=e3b37f11e6e4e6b6f02cc762f182ce233d2c1c9d > > > This wont change, because David never rebases his tree under normal > operations. > > Thanks. Thank you very much, Eric. I've reposted. -Aaron ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH nf-next v3 2/2] nf_set_hooks_head: accommodate different kconfig 2016-09-28 13:12 [PATCH nf-next v3 0/2] fixes for recent nf_compact hooks Aaron Conole 2016-09-28 13:12 ` [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference Aaron Conole @ 2016-09-28 13:12 ` Aaron Conole 1 sibling, 0 replies; 7+ messages in thread From: Aaron Conole @ 2016-09-28 13:12 UTC (permalink / raw) To: netfilter-devel, netdev; +Cc: Florian Westphal, Pablo Neira Ayuso When CONFIG_NETFILTER_INGRESS is unset (or no), we need to handle the request for registration properly by dropping the hook. This releases the entry during the set. Signed-off-by: Aaron Conole <aconole@bytheb.org> --- net/netfilter/core.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/net/netfilter/core.c b/net/netfilter/core.c index e58e420..61e8a9d 100644 --- a/net/netfilter/core.c +++ b/net/netfilter/core.c @@ -90,10 +90,12 @@ static void nf_set_hooks_head(struct net *net, const struct nf_hook_ops *reg, { switch (reg->pf) { case NFPROTO_NETDEV: +#ifdef CONFIG_NETFILTER_INGRESS /* We already checked in nf_register_net_hook() that this is * used from ingress. */ rcu_assign_pointer(reg->dev->nf_hooks_ingress, entry); +#endif break; default: rcu_assign_pointer(net->nf.hooks[reg->pf][reg->hooknum], @@ -107,10 +109,15 @@ int nf_register_net_hook(struct net *net, const struct nf_hook_ops *reg) struct nf_hook_entry *hooks_entry; struct nf_hook_entry *entry; - if (reg->pf == NFPROTO_NETDEV && - (reg->hooknum != NF_NETDEV_INGRESS || - !reg->dev || dev_net(reg->dev) != net)) - return -EINVAL; + if (reg->pf == NFPROTO_NETDEV) { +#ifndef CONFIG_NETFILTER_INGRESS + if (reg->hooknum == NF_NETDEV_INGRESS) + return -EOPNOTSUPP; +#endif + if (reg->hooknum != NF_NETDEV_INGRESS || + !reg->dev || dev_net(reg->dev) != net) + return -EINVAL; + } entry = kmalloc(sizeof(*entry), GFP_KERNEL); if (!entry) -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-28 15:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-28 13:12 [PATCH nf-next v3 0/2] fixes for recent nf_compact hooks Aaron Conole 2016-09-28 13:12 ` [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference Aaron Conole 2016-09-28 13:35 ` Eric Dumazet 2016-09-28 14:56 ` Aaron Conole 2016-09-28 15:30 ` Eric Dumazet 2016-09-28 15:38 ` Aaron Conole 2016-09-28 13:12 ` [PATCH nf-next v3 2/2] nf_set_hooks_head: accommodate different kconfig Aaron Conole
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).