[PATCH net] ipv4: Avoid crashing in ip_error

[PATCH net] ipv4: Avoid crashing in ip_error

[Eric W. Biederman]

ip_error does not check if in_dev is NULL before dereferencing it.

IThe following sequence of calls is possible:
CPU A                          CPU B
ip_rcv_finish
    ip_route_input_noref()
        ip_route_input_slow()
                               inetdev_destroy()
    dst_input()

With the result that a network device can be destroyed while processing
an input packet.

A crash was triggered with only unicast packets in flight, and
forwarding enabled on the only network device.   The error condition
was created by the removal of the network device.

As such it is likely the that error code was -EHOSTUNREACH, and the
action taken by ip_error (if in_dev had been accessible) would have
been to not increment any counters and to have tried and likely failed
to send an icmp error as the network device is going away.

Therefore handle this weird case by just dropping the packet if
!in_dev.  It will result in dropping the packet sooner, and will not
result in an actual change of behavior.

Fixes: 251da4130115b ("ipv4: Cache ip_error() routes even when not forwarding.")
Reported-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
Tested-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 net/ipv4/route.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 9e15f5ca4495..f6055984c307 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -900,6 +900,10 @@ static int ip_error(struct sk_buff *skb)
 	bool send;
 	int code;
 
+	/* IP on this device is disabled. */
+	if (!in_dev)
+		goto out;
+
 	net = dev_net(rt->dst.dev);
 	if (!IN_DEV_FORWARD(in_dev)) {
 		switch (rt->dst.error) {
-- 
2.2.1

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Eric Dumazet]

On Fri, 2015-05-22 at 04:58 -0500, Eric W. Biederman wrote:
> ip_error does not check if in_dev is NULL before dereferencing it.

> 
> Therefore handle this weird case by just dropping the packet if
> !in_dev.  It will result in dropping the packet sooner, and will not
> result in an actual change of behavior.
> 
> Fixes: 251da4130115b ("ipv4: Cache ip_error() routes even when not forwarding.")
> Reported-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
> Tested-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks !

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Vittorio G (VittGam)]

Il 22.05.2015 14:08 Eric Dumazet ha scritto:
> On Fri, 2015-05-22 at 04:58 -0500, Eric W. Biederman wrote:
>> ip_error does not check if in_dev is NULL before dereferencing it.
>
>>
>> Therefore handle this weird case by just dropping the packet if
>> !in_dev.  It will result in dropping the packet sooner, and will not
>> result in an actual change of behavior.
>>
>> Fixes: 251da4130115b ("ipv4: Cache ip_error() routes even when not forwarding.")
>> Reported-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
>> Tested-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>
> Acked-by: Eric Dumazet <edumazet@google.com>
>
> Thanks !

I've no problem using my real name here, so:

Reported-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
Tested-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>

Signed-off too, since this is actually v2 of my patch with the warning removed.

Thanks!
Vittorio

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Fri, 22 May 2015 04:58:12 -0500

> ip_error does not check if in_dev is NULL before dereferencing it.
> 
> IThe following sequence of calls is possible:
> CPU A                          CPU B
> ip_rcv_finish
>     ip_route_input_noref()
>         ip_route_input_slow()
>                                inetdev_destroy()
>     dst_input()
> 
> With the result that a network device can be destroyed while processing
> an input packet.
> 
> A crash was triggered with only unicast packets in flight, and
> forwarding enabled on the only network device.   The error condition
> was created by the removal of the network device.
> 
> As such it is likely the that error code was -EHOSTUNREACH, and the
> action taken by ip_error (if in_dev had been accessible) would have
> been to not increment any counters and to have tried and likely failed
> to send an icmp error as the network device is going away.
> 
> Therefore handle this weird case by just dropping the packet if
> !in_dev.  It will result in dropping the packet sooner, and will not
> result in an actual change of behavior.
> 
> Fixes: 251da4130115b ("ipv4: Cache ip_error() routes even when not forwarding.")
> Reported-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
> Tested-by: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Looks good, applied and queued up for -stable, thanks!

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Vittorio G (VittGam)]

Il 22.05.2015 20:24 David Miller ha scritto:
> Looks good, applied and queued up for -stable, thanks!

That's great!

I think this should also be backported up to Linux mainline 3.10, since
the original commit that introduced the bug first appeared in 3.6.

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[David Miller]

From: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
Date: Fri, 22 May 2015 20:48:38 +0200

> Il 22.05.2015 20:24 David Miller ha scritto:
>> Looks good, applied and queued up for -stable, thanks!
> 
> That's great!
> 
> I think this should also be backported up to Linux mainline 3.10,
> since
> the original commit that introduced the bug first appeared in 3.6.

Ummm, that's what "queued up for -stable" means.  It means that I'll
submit it to -stable for all active -stable series.

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Vittorio G (VittGam)]

Il 22.05.2015 21:05 David Miller ha scritto:
> From: "Vittorio G (VittGam)" <linuxbugs@vittgam.net>
> Date: Fri, 22 May 2015 20:48:38 +0200
>
>> I think this should also be backported up to Linux mainline 3.10,
>> since
>> the original commit that introduced the bug first appeared in 3.6.
>
> Ummm, that's what "queued up for -stable" means.  It means that I'll
> submit it to -stable for all active -stable series.

Uh, sorry. I'm not (yet) entirely familiar with the words used here. ;)

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Julian Anastasov]

	Hello,

On Fri, 22 May 2015, Eric W. Biederman wrote:

> ip_error does not check if in_dev is NULL before dereferencing it.
> 
> IThe following sequence of calls is possible:
> CPU A                          CPU B
> ip_rcv_finish
>     ip_route_input_noref()
>         ip_route_input_slow()
>                                inetdev_destroy()
>     dst_input()
> 
> With the result that a network device can be destroyed while processing
> an input packet.

	How can that happen in practice? May be network
driver is missing a napi_disable() call in ndo_stop()?
napi_disable is supposed to wait backlog before NETDEV_DOWN
and NETDEV_UNREGISTER (where inetdev_destroy is called) handlers
are notified. Any link to original crash report, info for used
driver, etc?

	After commit 8030f54499925d ("[IPV4] devinet: Register inetdev 
earlier.") dev->ip_ptr is always present while packets are
processed, only in_dev->ifa_list can be NULL.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Eric W. Biederman]

Julian Anastasov <ja@ssi.bg> writes:

> 	Hello,
>
> On Fri, 22 May 2015, Eric W. Biederman wrote:
>
>> ip_error does not check if in_dev is NULL before dereferencing it.
>> 
>> IThe following sequence of calls is possible:
>> CPU A                          CPU B
>> ip_rcv_finish
>>     ip_route_input_noref()
>>         ip_route_input_slow()
>>                                inetdev_destroy()
>>     dst_input()
>> 
>> With the result that a network device can be destroyed while processing
>> an input packet.
>
> 	How can that happen in practice? May be network
> driver is missing a napi_disable() call in ndo_stop()?
> napi_disable is supposed to wait backlog before NETDEV_DOWN
> and NETDEV_UNREGISTER (where inetdev_destroy is called) handlers
> are notified. Any link to original crash report, info for used
> driver, etc?
>
> 	After commit 8030f54499925d ("[IPV4] devinet: Register inetdev 
> earlier.") dev->ip_ptr is always present while packets are
> processed, only in_dev->ifa_list can be NULL.

Interesting I see the dev_close in unregister_netdevice that should have
prevented this scenario.

The reproducer was apparently:
ip netns add ns0
ip netns add ns1
ip link add test0 type veth peer name test1
ip link set test0 netns ns0
ip link set test1 netns ns1
ip netns exec ns0 sysctl net.ipv4.ip_forward=1
ip netns exec ns1 sysctl net.ipv4.ip_forward=1
ip netns exec ns0 sysctl net.ipv4.conf.all.accept_local=1
ip netns exec ns1 sysctl net.ipv4.conf.all.accept_local=1
ip netns exec ns0 sysctl net.ipv4.conf.test0.accept_local=1
ip netns exec ns1 sysctl net.ipv4.conf.test1.accept_local=1
ip netns exec ns0 sysctl net.ipv4.conf.all.send_redirects=0
ip netns exec ns1 sysctl net.ipv4.conf.all.send_redirects=0
ip netns exec ns0 sysctl net.ipv4.conf.test0.send_redirects=0
ip netns exec ns1 sysctl net.ipv4.conf.test1.send_redirects=0
ip netns exec ns0 ip addr add 10.10.10.1/24 dev test0
ip netns exec ns1 ip addr add 10.10.10.2/24 dev test1
ip netns exec ns0 ip link set up dev test0
ip netns exec ns1 ip link set up dev test1
ip netns exec ns0 ip route add 10.20.30.40 via 10.10.10.2 dev test0
ip netns exec ns1 ip route add 10.20.30.40 via 10.10.10.1 dev test1
ip netns exec ns0 iptables -t mangle -A PREROUTING -i test0 -j TTL --ttl-inc 1
ip netns exec ns1 iptables -t mangle -A PREROUTING -i test1 -j TTL --ttl-inc 1
ip netns exec ns0 ping -s 1400 -f -w 5 10.20.30.40
ip netns delete ns0
ip netns delete ns1

With a NULL pointer dereference in ip_error at offset 0x108 which
appeared to correspond with the line:

	if (!IN_DEV_FORWARD(in_dev)) {

As did the debug information.

Plus there is not much other than in_dev that could be NULL in that
function.

Perhaps it is possible to transmit into a veth pair even when the
carrier is down?  Which could be a bug in the veth driver.

Eric

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Vittorio G (VittGam)]

Il 23.05.2015 16:40 ebiederm@xmission.com ha scritto:
> The reproducer was apparently:
> ip netns add ns0
> ip netns add ns1
> ip link add test0 type veth peer name test1
> ip link set test0 netns ns0
> ip link set test1 netns ns1
> ip netns exec ns0 sysctl net.ipv4.ip_forward=1
> ip netns exec ns1 sysctl net.ipv4.ip_forward=1
> ip netns exec ns0 sysctl net.ipv4.conf.all.accept_local=1
> ip netns exec ns1 sysctl net.ipv4.conf.all.accept_local=1
> ip netns exec ns0 sysctl net.ipv4.conf.test0.accept_local=1
> ip netns exec ns1 sysctl net.ipv4.conf.test1.accept_local=1
> ip netns exec ns0 sysctl net.ipv4.conf.all.send_redirects=0
> ip netns exec ns1 sysctl net.ipv4.conf.all.send_redirects=0
> ip netns exec ns0 sysctl net.ipv4.conf.test0.send_redirects=0
> ip netns exec ns1 sysctl net.ipv4.conf.test1.send_redirects=0
> ip netns exec ns0 ip addr add 10.10.10.1/24 dev test0
> ip netns exec ns1 ip addr add 10.10.10.2/24 dev test1
> ip netns exec ns0 ip link set up dev test0
> ip netns exec ns1 ip link set up dev test1
> ip netns exec ns0 ip route add 10.20.30.40 via 10.10.10.2 dev test0
> ip netns exec ns1 ip route add 10.20.30.40 via 10.10.10.1 dev test1
> ip netns exec ns0 iptables -t mangle -A PREROUTING -i test0 -j TTL --ttl-inc 1
> ip netns exec ns1 iptables -t mangle -A PREROUTING -i test1 -j TTL --ttl-inc 1
> ip netns exec ns0 ping -s 1400 -f -w 5 10.20.30.40
> ip netns delete ns0
> ip netns delete ns1

Yes, and the commands were ultimately wrapped in a "while true; do ... done" loop,
since the crash would not happen every time, but it eventually will, without the
patch.

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Julian Anastasov]

	Hello,

On Sat, 23 May 2015, Eric W. Biederman wrote:

> Perhaps it is possible to transmit into a veth pair even when the
> carrier is down?  Which could be a bug in the veth driver.

	Not sure if that can help but both RCU_INIT_POINTER
calls in veth_dellink can be before the unregister_netdevice_queue
ones. May be it will help for the case when veth_dellink is
called with head=NULL. As result, veth_xmit will free all
flying skbs to peer that is unregistered first.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Eric Dumazet]

On Sat, 2015-05-23 at 21:48 +0300, Julian Anastasov wrote:
> 	Hello,
> 
> On Sat, 23 May 2015, Eric W. Biederman wrote:
> 
> > Perhaps it is possible to transmit into a veth pair even when the
> > carrier is down?  Which could be a bug in the veth driver.
> 
> 	Not sure if that can help but both RCU_INIT_POINTER
> calls in veth_dellink can be before the unregister_netdevice_queue
> ones. May be it will help for the case when veth_dellink is
> called with head=NULL. As result, veth_xmit will free all
> flying skbs to peer that is unregistered first.

Good point, but note that we never call dellink() method with head==NULL
in current kernels.

Re: [PATCH net] ipv4: Avoid crashing in ip_error

[Julian Anastasov]

	Hello,

On Sat, 23 May 2015, Eric Dumazet wrote:

> > 	Not sure if that can help but both RCU_INIT_POINTER
> > calls in veth_dellink can be before the unregister_netdevice_queue
> > ones. May be it will help for the case when veth_dellink is
> > called with head=NULL. As result, veth_xmit will free all
> > flying skbs to peer that is unregistered first.
> 
> Good point, but note that we never call dellink() method with head==NULL
> in current kernels.

	Can it be a general problem with process_backlog
delivering packets even after the ndo_stop call?
It can happen even for devices that use napi_disable.
synchronize_net simply misses packets processed on
another CPU because process_backlog is not always in
RCU read section.

	What about moving flush_backlog before
NETDEV_UNREGISTER, it was after NETDEV_UNREGISTER_FINAL.
It will work like a synchronize_net call.

rollback_registered_many:
1. __dev_close_many: Commit netif_running, ndo_stop
2. on_each_cpu(flush_backlog, dev, 1): may be its place
can be additionally tuned...
3. synchronize_net: not needed anymore?
4. call_netdevice_notifiers(NETDEV_UNREGISTER, dev)

Compile-tested only:

diff --git a/net/core/dev.c b/net/core/dev.c
index c7ba038..e402661 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6034,6 +6036,7 @@ static void rollback_registered_many(struct list_head *head)
 		unlist_netdevice(dev);
 
 		dev->reg_state = NETREG_UNREGISTERING;
+		on_each_cpu(flush_backlog, dev, 1);
 	}
 
 	synchronize_net();
@@ -6657,8 +6660,6 @@ void netdev_run_todo(void)
 
 		dev->reg_state = NETREG_UNREGISTERED;
 
-		on_each_cpu(flush_backlog, dev, 1);
-
 		netdev_wait_allrefs(dev);
 
 		/* paranoia */
Regards

--
Julian Anastasov <ja@ssi.bg>