| Message ID | 20190821141505.2394-1-leonardo@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [v2,1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot | expand |
On Wed, Aug 21, 2019 at 11:15:06AM -0300, Leonardo Bras wrote: > If IPv6 is disabled on boot (ipv6.disable=1), but nft_fib_inet ends up > dealing with a IPv6 package, it causes a kernel panic in > fib6_node_lookup_1(), crashing in bad_page_fault. Q: How do you get to see IPv6 packets if IPv6 module is disable? > The panic is caused by trying to deference a very low address (0x38 > in ppc64le), due to ipv6.fib6_main_tbl = NULL. > BUG: Kernel NULL pointer dereference at 0x00000038 > > Fix this behavior by dropping IPv6 packages if !ipv6_mod_enabled(). I'd suggest: s/package/packet/ [...] > diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c > index 7ece86afd079..75acc417e2ff 100644 > --- a/net/ipv6/netfilter/nft_fib_ipv6.c > +++ b/net/ipv6/netfilter/nft_fib_ipv6.c > @@ -125,6 +125,11 @@ void nft_fib6_eval_type(const struct nft_expr *expr, struct nft_regs *regs, > u32 *dest = ®s->data[priv->dreg]; > struct ipv6hdr *iph, _iph; > > + if (!ipv6_mod_enabled()) { > + regs->verdict.code = NF_DROP; NFT_BREAK instead to stop evaluating this rule, this results in a mismatch, so you let the user decide what to do with packets that do not match your policy. The drop case at the bottom of the fib eval function never actually never happens.
On Tue, 2019-08-27 at 12:35 +0200, Pablo Neira Ayuso wrote: > On Wed, Aug 21, 2019 at 11:15:06AM -0300, Leonardo Bras wrote: > > If IPv6 is disabled on boot (ipv6.disable=1), but nft_fib_inet ends up > > dealing with a IPv6 package, it causes a kernel panic in > > fib6_node_lookup_1(), crashing in bad_page_fault. > > Q: How do you get to see IPv6 packets if IPv6 module is disable? I could reproduce this bug on a host ('ipv6.disable=1') starting a guest with a virtio-net interface with 'filterref' over a virtual bridge. It crashes the host during guest boot (just before login). By that I could understand that a guest IPv6 network traffic (viavirtio-net) may cause this kernel panic. > > > The panic is caused by trying to deference a very low address (0x38 > > in ppc64le), due to ipv6.fib6_main_tbl = NULL. > > BUG: Kernel NULL pointer dereference at 0x00000038 > > > > Fix this behavior by dropping IPv6 packages if !ipv6_mod_enabled(). > > I'd suggest: s/package/packet/ Sure, I will make sure to put it on v3. (Sorry, I am not very used to net subsystem.) > > [...] > > diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c > > index 7ece86afd079..75acc417e2ff 100644 > > --- a/net/ipv6/netfilter/nft_fib_ipv6.c > > +++ b/net/ipv6/netfilter/nft_fib_ipv6.c > > @@ -125,6 +125,11 @@ void nft_fib6_eval_type(const struct nft_expr *expr, struct nft_regs *regs, > > u32 *dest = ®s->data[priv->dreg]; > > struct ipv6hdr *iph, _iph; > > > > + if (!ipv6_mod_enabled()) { > > + regs->verdict.code = NF_DROP; > > NFT_BREAK instead to stop evaluating this rule, this results in a > mismatch, so you let the user decide what to do with packets that do > not match your policy. Ok, I will replace for v3. > > The drop case at the bottom of the fib eval function never actually > never happens. Which one do you mean?
On Tue, Aug 27, 2019 at 02:34:14PM -0300, Leonardo Bras wrote: > On Tue, 2019-08-27 at 12:35 +0200, Pablo Neira Ayuso wrote: [...] > > NFT_BREAK instead to stop evaluating this rule, this results in a > > mismatch, so you let the user decide what to do with packets that do > > not match your policy. > > Ok, I will replace for v3. Thanks. > > The drop case at the bottom of the fib eval function never actually > > never happens. > > Which one do you mean? Line 31 of net/netfilter/nft_fib_inet.c.
On Tue, 2019-08-27 at 20:51 +0200, Pablo Neira Ayuso wrote: > > > The drop case at the bottom of the fib eval function never actually > > > never happens. > > > > Which one do you mean? > > Line 31 of net/netfilter/nft_fib_inet.c. Oh, yeah, I was thinking about that when I wrote the patch. Thanks for explaining :) I will send the v3 in a few minutes. Best regards,
From: Leonardo Bras <leonardo@linux.ibm.com> Date: Tue, 27 Aug 2019 14:34:14 -0300 > I could reproduce this bug on a host ('ipv6.disable=1') starting a > guest with a virtio-net interface with 'filterref' over a virtual > bridge. It crashes the host during guest boot (just before login). > > By that I could understand that a guest IPv6 network traffic > (viavirtio-net) may cause this kernel panic. Really this is bad and I suspected bridging to be involved somehow. If ipv6 is disabled ipv6 traffic should not pass through the machine by any means whatsoever. Otherwise there is no point to the knob and we will keep having to add hack checks all over the tree instead of fixing the fundamental issue.
David Miller <davemdavemloft!net> wrote: > From: Leonardo Bras <leonardo@linux.ibm.com> > Date: Tue, 27 Aug 2019 14:34:14 -0300 > > > I could reproduce this bug on a host ('ipv6.disable=1') starting a > > guest with a virtio-net interface with 'filterref' over a virtual > > bridge. It crashes the host during guest boot (just before login). > > > > By that I could understand that a guest IPv6 network traffic > > (viavirtio-net) may cause this kernel panic. > > Really this is bad and I suspected bridging to be involved somehow. Thats a good point -- Leonardo, is the "net.bridge.bridge-nf-call-ip6tables" sysctl on? As much as i'd like to send a patch to remove br_netfilter, I fear we can't even stop passing ipv6 packets up to netfilter if ipv6.disable=1 is set because users might be using ip6tables for bridged traffic.
On Tue, Aug 27, 2019 at 11:58:36PM +0200, Florian Westphal wrote: > David Miller <davemdavemloft!net> wrote: > > From: Leonardo Bras <leonardo@linux.ibm.com> > > Date: Tue, 27 Aug 2019 14:34:14 -0300 > > > > > I could reproduce this bug on a host ('ipv6.disable=1') starting a > > > guest with a virtio-net interface with 'filterref' over a virtual > > > bridge. It crashes the host during guest boot (just before login). > > > > > > By that I could understand that a guest IPv6 network traffic > > > (viavirtio-net) may cause this kernel panic. > > > > Really this is bad and I suspected bridging to be involved somehow. > > Thats a good point -- Leonardo, is the > "net.bridge.bridge-nf-call-ip6tables" sysctl on? > > As much as i'd like to send a patch to remove br_netfilter, I fear > we can't even stop passing ipv6 packets up to netfilter if > ipv6.disable=1 is set because users might be using ip6tables for > bridged traffic. If the br_netfilter module is in placed, then it's probably better to perform this check from there.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Aug 27, 2019 at 11:58:36PM +0200, Florian Westphal wrote: > > David Miller <davemdavemloft!net> wrote: > > > From: Leonardo Bras <leonardo@linux.ibm.com> > > > Date: Tue, 27 Aug 2019 14:34:14 -0300 > > > > > > > I could reproduce this bug on a host ('ipv6.disable=1') starting a > > > > guest with a virtio-net interface with 'filterref' over a virtual > > > > bridge. It crashes the host during guest boot (just before login). > > > > > > > > By that I could understand that a guest IPv6 network traffic > > > > (viavirtio-net) may cause this kernel panic. > > > > > > Really this is bad and I suspected bridging to be involved somehow. > > > > Thats a good point -- Leonardo, is the > > "net.bridge.bridge-nf-call-ip6tables" sysctl on? > > > > As much as i'd like to send a patch to remove br_netfilter, I fear > > we can't even stop passing ipv6 packets up to netfilter if > > ipv6.disable=1 is set because users might be using ip6tables for > > bridged traffic. > > If the br_netfilter module is in placed, then it's probably better to > perform this check from there. ipt6tables won't work for filtering anymore, so I don't think this is something we can do. Anyway, lets wait for Leonardo to confirm, else this is pointless speculation :-)
> Thats a good point -- Leonardo, is the > "net.bridge.bridge-nf-call-ip6tables" sysctl on? Running # sudo sysctl -a I can see: net.bridge.bridge-nf-call-ip6tables = 1 So this packets are sent to host iptables for processing? (Sorry for the delay, I did not received the previous e-mails. Please include me in to/cc.)
On Thu, 2019-08-29 at 17:04 -0300, Leonardo Bras wrote: > > Thats a good point -- Leonardo, is the > > "net.bridge.bridge-nf-call-ip6tables" sysctl on? > > Running > # sudo sysctl -a > I can see: > net.bridge.bridge-nf-call-ip6tables = 1 Also, doing # echo 0 > /proc/sys/net/bridge/bridge-nf-call-ip6tables And then trying to boot the guest will not crash the host. Which would make sense, since host iptables is not dealing with guest IPv6 packets. So, the real cause of this bug is the bridge making host ip6tables deal with guest IPv6 packets ? If so, would it be ok if write a patch testing ipv6_mod_enabled() before passing guest ipv6 packets to host ip6tables? Best regards, > > So this packets are sent to host iptables for processing? > > > (Sorry for the delay, I did not received the previous e-mails. > Please include me in to/cc.)
Leonardo Bras <leonardo@linux.ibm.com> wrote: > > Thats a good point -- Leonardo, is the > > "net.bridge.bridge-nf-call-ip6tables" sysctl on? > > Running > # sudo sysctl -a > I can see: > net.bridge.bridge-nf-call-ip6tables = 1 > > So this packets are sent to host iptables for processing? Yes, this is an hold hack that was made because ebtables is very feature-limited. However, as I mentioned before I don't think there is anything we can do here except audit all affected nft expressions and ip6tables matches and add this check where needed. ip6t_rpfilter.c comes to mind. In any case your patch looks ok to me. > (Sorry for the delay, I did not received the previous e-mails. > Please include me in to/cc.) Sorry about that.
Leonardo Bras <leonardo@linux.ibm.com> wrote: > On Thu, 2019-08-29 at 17:04 -0300, Leonardo Bras wrote: > > > Thats a good point -- Leonardo, is the > > > "net.bridge.bridge-nf-call-ip6tables" sysctl on? > > > > Running > > # sudo sysctl -a > > I can see: > > net.bridge.bridge-nf-call-ip6tables = 1 > > Also, doing > # echo 0 > /proc/sys/net/bridge/bridge-nf-call-ip6tables > And then trying to boot the guest will not crash the host. > > Which would make sense, since host iptables is not dealing with guest > IPv6 packets. Yes. > So, the real cause of this bug is the bridge making host ip6tables deal > with guest IPv6 packets ? > If so, would it be ok if write a patch testing ipv6_mod_enabled() > before passing guest ipv6 packets to host ip6tables? I'm not sure. This switch is very old, it was added 10 years ago in v2.6.31-rc1. Even if we disable call-ip6tables in br_netfilter we will at least in addition need a patch for nft_fib_netdev.c. From a "avoid calls to ipv6 stack when its disabled" standpoint, the safest fix is to disable call-ip6tables functionality if ipv6 module is off *and* fix nft_fib_netdev.c to BREAK in ipv6 is off case. I started to place a list of suspicous modules here, but that got out of hand quickly. So, given I don't want to plaster ipv6_mod_enabled() everywhere, I would suggest this course of action: 1. add a patch to BREAK in nft_fib_netdev.c for !ipv6_mod_enabled() 2. change net/bridge/br_netfilter_hooks.c, br_nf_pre_routing() to make sure ipv6_mod_enabled() is true before doing the ipv6 stack "emulation". Makes sense? Thanks, Florian
On Thu, 2019-08-29 at 22:58 +0200, Florian Westphal wrote: > In any case your patch looks ok to me. Great! Please give your feedback on v3: http://patchwork.ozlabs.org/patch/1154040/ [...] > > Even if we disable call-ip6tables in br_netfilter we will at least > in addition need a patch for nft_fib_netdev.c. > > From a "avoid calls to ipv6 stack when its disabled" standpoint, > the safest fix is to disable call-ip6tables functionality if ipv6 > module is off *and* fix nft_fib_netdev.c to BREAK in ipv6 is off case. > > I started to place a list of suspicous modules here, but that got out > of hand quickly. > > So, given I don't want to plaster ipv6_mod_enabled() everywhere, I > would suggest this course of action: > > 1. add a patch to BREAK in nft_fib_netdev.c for !ipv6_mod_enabled() > 2. change net/bridge/br_netfilter_hooks.c, br_nf_pre_routing() to > make sure ipv6_mod_enabled() is true before doing the ipv6 stack > "emulation". > > Makes sense? IMHO sure. Shortly, I will send a couple patches proposing the above changes. (Or my best understanding about them :) ) > > Thanks, > Florian Thank you, Leonardo Bras
On Thu, 2019-08-29 at 22:58 +0200, Florian Westphal wrote:
[...]
> 1. add a patch to BREAK in nft_fib_netdev.c for !ipv6_mod_enabled()
[...]
But this is still needed? I mean, in nft_fib_netdev_eval there are only
2 functions being called for IPv6 protocol : nft_fib6_eval and
nft_fib6_eval_type. Both are already protected by this current patch.
Is your 1st suggestion about this patch, or you think it's better to
move this change to nft_fib_netdev_eval ?
Best regards,
Leonardo Bras
Leonardo Bras <leonardo@linux.ibm.com> wrote: > On Thu, 2019-08-29 at 22:58 +0200, Florian Westphal wrote: > [...] > > 1. add a patch to BREAK in nft_fib_netdev.c for !ipv6_mod_enabled() > [...] > > But this is still needed? I mean, in nft_fib_netdev_eval there are only > 2 functions being called for IPv6 protocol : nft_fib6_eval and > nft_fib6_eval_type. Both are already protected by this current patch. > > Is your 1st suggestion about this patch, or you think it's better to > move this change to nft_fib_netdev_eval ? Ah, it was the latter. Making bridge netfilter not pass packets up with ipv6 off closes the problem for fib_ipv6 and inet, so only _netdev.c needs fixing.
On Thu, 2019-08-29 at 22:58 +0200, Florian Westphal wrote: > Ah, it was the latter. > Making bridge netfilter not pass packets up with ipv6 off closes > the problem for fib_ipv6 and inet, so only _netdev.c needs fixing. Ok then, preparing a v4. https://lkml.org/lkml/2019/8/30/843
diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c index 7ece86afd079..75acc417e2ff 100644 --- a/net/ipv6/netfilter/nft_fib_ipv6.c +++ b/net/ipv6/netfilter/nft_fib_ipv6.c @@ -125,6 +125,11 @@ void nft_fib6_eval_type(const struct nft_expr *expr, struct nft_regs *regs, u32 *dest = ®s->data[priv->dreg]; struct ipv6hdr *iph, _iph; + if (!ipv6_mod_enabled()) { + regs->verdict.code = NF_DROP; + return; + } + iph = skb_header_pointer(pkt->skb, noff, sizeof(_iph), &_iph); if (!iph) { regs->verdict.code = NFT_BREAK; @@ -150,6 +155,11 @@ void nft_fib6_eval(const struct nft_expr *expr, struct nft_regs *regs, struct rt6_info *rt; int lookup_flags; + if (!ipv6_mod_enabled()) { + regs->verdict.code = NF_DROP; + return; + } + if (priv->flags & NFTA_FIB_F_IIF) oif = nft_in(pkt); else if (priv->flags & NFTA_FIB_F_OIF)
If IPv6 is disabled on boot (ipv6.disable=1), but nft_fib_inet ends up dealing with a IPv6 package, it causes a kernel panic in fib6_node_lookup_1(), crashing in bad_page_fault. The panic is caused by trying to deference a very low address (0x38 in ppc64le), due to ipv6.fib6_main_tbl = NULL. BUG: Kernel NULL pointer dereference at 0x00000038 Fix this behavior by dropping IPv6 packages if !ipv6_mod_enabled(). Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com> --- Changes from v1: - Move drop logic from nft_fib_inet_eval() to nft_fib6_eval{,_type} so it can affect other usages of these functions. net/ipv6/netfilter/nft_fib_ipv6.c | 10 ++++++++++ 1 file changed, 10 insertions(+)