[PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

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(+)

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 = &regs->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)
-- 
2.20.1

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Pablo Neira Ayuso]

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 = &regs->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.

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

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 = &regs->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?


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Pablo Neira Ayuso]

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.

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

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,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[David Miller]

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.

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Florian Westphal]

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.

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Pablo Neira Ayuso]

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.

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Florian Westphal]

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 :-)

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

> 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.)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

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.)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Florian Westphal]

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

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Florian Westphal]

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.

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Leonardo Bras]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] netfilter: nf_tables: fib: Drop IPV6 packages if IPv6 is disabled on boot

[Florian Westphal]

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.