* [PATCH nf-next v2 0/2] fixes for recent nf_compact hooks @ 2016-09-27 13:38 Aaron Conole 2016-09-27 13:38 ` [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference Aaron Conole 2016-09-27 13:38 ` [PATCH nf-next v2 2/2] nf_set_hooks_head: acommodate different kconfig Aaron Conole 0 siblings, 2 replies; 8+ messages in thread From: Aaron Conole @ 2016-09-27 13:38 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] 8+ messages in thread
* [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference 2016-09-27 13:38 [PATCH nf-next v2 0/2] fixes for recent nf_compact hooks Aaron Conole @ 2016-09-27 13:38 ` Aaron Conole 2016-09-28 1:23 ` Feng Gao 2016-09-27 13:38 ` [PATCH nf-next v2 2/2] nf_set_hooks_head: acommodate different kconfig Aaron Conole 1 sibling, 1 reply; 8+ messages in thread From: Aaron Conole @ 2016-09-27 13:38 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. This fix ensures that no null pointer dereference could occur when such a race 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] 8+ messages in thread
* Re: [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference 2016-09-27 13:38 ` [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference Aaron Conole @ 2016-09-28 1:23 ` Feng Gao 2016-09-28 3:08 ` Liping Zhang 0 siblings, 1 reply; 8+ messages in thread From: Feng Gao @ 2016-09-28 1:23 UTC (permalink / raw) To: Aaron Conole Cc: Netfilter Developer Mailing List, Linux Kernel Network Developers, Florian Westphal, Pablo Neira Ayuso Hi Aaraon, On Tue, Sep 27, 2016 at 9:38 PM, Aaron Conole <aconole@bytheb•org> 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 The critical region of nf_unregister_net_hook is protected by &nf_hook_mutex. When it would be called simultaneously? Regards Feng > entry in the list. This fix ensures that no null pointer dereference > could occur when such a race 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 > > -- > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 1:23 ` Feng Gao @ 2016-09-28 3:08 ` Liping Zhang 2016-09-28 3:13 ` Liping Zhang 0 siblings, 1 reply; 8+ messages in thread From: Liping Zhang @ 2016-09-28 3:08 UTC (permalink / raw) To: Feng Gao Cc: Aaron Conole, Netfilter Developer Mailing List, Linux Kernel Network Developers, Florian Westphal, Pablo Neira Ayuso Hi Feng, 2016-09-28 9:23 GMT+08:00 Feng Gao <gfree.wind@gmail•com>: > Hi Aaraon, > > On Tue, Sep 27, 2016 at 9:38 PM, Aaron Conole <aconole@bytheb•org> 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 > > The critical region of nf_unregister_net_hook is protected by &nf_hook_mutex. > When it would be called simultaneously? This is unrelated to race condition. Suppose that only the last nf_hook_entry exist, and two callers want to do un-register work. The first one will remove it successfully, after the end of the work, the second one will enter the critical section, but it will see the NULL pointer. Because the last nf_hook_entry was already removed by the first one. > > Regards > Feng > >> entry in the list. This fix ensures that no null pointer dereference >> could occur when such a race happens. >> >> Signed-off-by: Aaron Conole <aconole@bytheb•org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 3:08 ` Liping Zhang @ 2016-09-28 3:13 ` Liping Zhang 2016-09-28 7:56 ` Feng Gao 2016-09-28 13:30 ` Aaron Conole 0 siblings, 2 replies; 8+ messages in thread From: Liping Zhang @ 2016-09-28 3:13 UTC (permalink / raw) To: Feng Gao Cc: Aaron Conole, Netfilter Developer Mailing List, Linux Kernel Network Developers, Florian Westphal, Pablo Neira Ayuso 2016-09-28 11:08 GMT+08:00 Liping Zhang <zlpnobody@gmail•com>: > Hi Feng, > > 2016-09-28 9:23 GMT+08:00 Feng Gao <gfree.wind@gmail•com>: >> Hi Aaraon, >> >> On Tue, Sep 27, 2016 at 9:38 PM, Aaron Conole <aconole@bytheb•org> 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 >> >> The critical region of nf_unregister_net_hook is protected by &nf_hook_mutex. >> When it would be called simultaneously? > > This is unrelated to race condition. > > Suppose that only the last nf_hook_entry exist, and two callers want to do > un-register work. > > The first one will remove it successfully, after the end of the work, the > second one will enter the critical section, but it will see the NULL pointer. > Because the last nf_hook_entry was already removed by the first one. > >> >> Regards >> Feng >> >>> entry in the list. This fix ensures that no null pointer dereference >>> could occur when such a race happens. >>> >>> Signed-off-by: Aaron Conole <aconole@bytheb•org> I read the commit log again, I think the description here is a little confusing indeed. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 3:13 ` Liping Zhang @ 2016-09-28 7:56 ` Feng Gao 2016-09-28 13:30 ` Aaron Conole 1 sibling, 0 replies; 8+ messages in thread From: Feng Gao @ 2016-09-28 7:56 UTC (permalink / raw) To: Liping Zhang Cc: Aaron Conole, Netfilter Developer Mailing List, Linux Kernel Network Developers, Florian Westphal, Pablo Neira Ayuso Hi Liping, On Wed, Sep 28, 2016 at 11:13 AM, Liping Zhang <zlpnobody@gmail•com> wrote: > 2016-09-28 11:08 GMT+08:00 Liping Zhang <zlpnobody@gmail•com>: >> Hi Feng, >> >> 2016-09-28 9:23 GMT+08:00 Feng Gao <gfree.wind@gmail•com>: >>> Hi Aaraon, >>> >>> On Tue, Sep 27, 2016 at 9:38 PM, Aaron Conole <aconole@bytheb•org> 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 >>> >>> The critical region of nf_unregister_net_hook is protected by &nf_hook_mutex. >>> When it would be called simultaneously? >> >> This is unrelated to race condition. >> >> Suppose that only the last nf_hook_entry exist, and two callers want to do >> un-register work. >> >> The first one will remove it successfully, after the end of the work, the >> second one will enter the critical section, but it will see the NULL pointer. >> Because the last nf_hook_entry was already removed by the first one. >> >>> >>> Regards >>> Feng >>> >>>> entry in the list. This fix ensures that no null pointer dereference >>>> could occur when such a race happens. >>>> >>>> Signed-off-by: Aaron Conole <aconole@bytheb•org> > > I read the commit log again, I think the description here is a > little confusing indeed. Yes. I doesn't check if the list head always exists, just learn the patch from commit log. It confuses me indeed. Regards Feng ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference 2016-09-28 3:13 ` Liping Zhang 2016-09-28 7:56 ` Feng Gao @ 2016-09-28 13:30 ` Aaron Conole 1 sibling, 0 replies; 8+ messages in thread From: Aaron Conole @ 2016-09-28 13:30 UTC (permalink / raw) To: Liping Zhang Cc: Feng Gao, Netfilter Developer Mailing List, Linux Kernel Network Developers, Florian Westphal, Pablo Neira Ayuso Liping Zhang <zlpnobody@gmail•com> writes: > 2016-09-28 11:08 GMT+08:00 Liping Zhang <zlpnobody@gmail•com>: >> Hi Feng, >> >> 2016-09-28 9:23 GMT+08:00 Feng Gao <gfree.wind@gmail•com>: >>> Hi Aaraon, >>> >>> On Tue, Sep 27, 2016 at 9:38 PM, Aaron Conole <aconole@bytheb•org> 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 >>> >>> The critical region of nf_unregister_net_hook is protected by &nf_hook_mutex. >>> When it would be called simultaneously? >> >> This is unrelated to race condition. >> >> Suppose that only the last nf_hook_entry exist, and two callers want to do >> un-register work. >> >> The first one will remove it successfully, after the end of the work, the >> second one will enter the critical section, but it will see the NULL pointer. >> Because the last nf_hook_entry was already removed by the first one. >> >>> >>> Regards >>> Feng >>> >>>> entry in the list. This fix ensures that no null pointer dereference >>>> could occur when such a race happens. >>>> >>>> Signed-off-by: Aaron Conole <aconole@bytheb•org> > > I read the commit log again, I think the description here is a > little confusing indeed. Sorry about that. I've posted a new description which describes the situation better. I hope it helps. -Aaron ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH nf-next v2 2/2] nf_set_hooks_head: acommodate different kconfig 2016-09-27 13:38 [PATCH nf-next v2 0/2] fixes for recent nf_compact hooks Aaron Conole 2016-09-27 13:38 ` [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference Aaron Conole @ 2016-09-27 13:38 ` Aaron Conole 1 sibling, 0 replies; 8+ messages in thread From: Aaron Conole @ 2016-09-27 13:38 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] 8+ messages in thread
end of thread, other threads:[~2016-09-28 13:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-27 13:38 [PATCH nf-next v2 0/2] fixes for recent nf_compact hooks Aaron Conole 2016-09-27 13:38 ` [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference Aaron Conole 2016-09-28 1:23 ` Feng Gao 2016-09-28 3:08 ` Liping Zhang 2016-09-28 3:13 ` Liping Zhang 2016-09-28 7:56 ` Feng Gao 2016-09-28 13:30 ` Aaron Conole 2016-09-27 13:38 ` [PATCH nf-next v2 2/2] nf_set_hooks_head: acommodate 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