[PATCH] net: Fix NETDEV_CHANGE notifier usage causing spurious arp flush

[PATCH] net: Fix NETDEV_CHANGE notifier usage causing spurious arp flush

[Loic Prylli]

A bug was introduced in NETDEV_CHANGE notifier sequence causing the
arp table to be sometimes spuriously cleared (including manual arp
entries marked permanent), upon network link carrier changes.

The changed argument for the notifier was applied only to a single
caller of NETDEV_CHANGE, missing among others netdev_state_change().
So upon net_carrier events induced by the network, which are
triggering a call to netdev_state_change(), arp_netdev_event() would
decide whether to clear or not arp cache based on random/junk stack
values (a kind of read buffer overflow).

Fixes: be9efd365328 ("net: pass changed flags along with NETDEV_CHANGE event")
Fixes: 6c8b4e3ff81b ("arp: flush arp cache on IFF_NOARP change")
Signed-off-by: Loic Prylli <loicp@google.com>
---
 net/core/dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 30eedf6..63129d4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -148,6 +148,9 @@ struct list_head ptype_all __read_mostly;	/* Taps */
 static struct list_head offload_base __read_mostly;
 
 static int netif_rx_internal(struct sk_buff *skb);
+static int call_netdevice_notifiers_info(unsigned long val,
+					 struct net_device *dev,
+					 struct netdev_notifier_info *info);
 
 /*
  * The @dev_base_head list is protected by @dev_base_lock and the rtnl
@@ -1207,7 +1210,11 @@ EXPORT_SYMBOL(netdev_features_change);
 void netdev_state_change(struct net_device *dev)
 {
 	if (dev->flags & IFF_UP) {
-		call_netdevice_notifiers(NETDEV_CHANGE, dev);
+		struct netdev_notifier_change_info change_info;
+
+		change_info.flags_changed = 0;
+		call_netdevice_notifiers_info(NETDEV_CHANGE, dev,
+					      &change_info.info);
 		rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
 	}
 }
-- 
2.0.0.526.g5318336

Re: [PATCH] net: Fix NETDEV_CHANGE notifier usage causing spurious arp flush

[Dan Aloni]

On Tue, Jul 01, 2014 at 09:39:43PM -0700, Loic Prylli wrote:
> A bug was introduced in NETDEV_CHANGE notifier sequence causing the
> arp table to be sometimes spuriously cleared (including manual arp
> entries marked permanent), upon network link carrier changes.
> 
> The changed argument for the notifier was applied only to a single
> caller of NETDEV_CHANGE, missing among others netdev_state_change().
> So upon net_carrier events induced by the network, which are
> triggering a call to netdev_state_change(), arp_netdev_event() would
> decide whether to clear or not arp cache based on random/junk stack
> values (a kind of read buffer overflow).
[..]
>  {
>  	if (dev->flags & IFF_UP) {
> -		call_netdevice_notifiers(NETDEV_CHANGE, dev);
> +		struct netdev_notifier_change_info change_info;
> +
> +		change_info.flags_changed = 0;

I think it would be safer to do:

   struct netdev_notifier_change_info change_info = {};

So that when future fields are added to the struct and this call-site
happens to be forgotten, they will get 0 by default rather than
random stack values.

-- 
Dan Aloni

Re: [PATCH] net: Fix NETDEV_CHANGE notifier usage causing spurious arp flush

[Loic Prylli]

On Wed, Jul 02, 2014 at 10:03:49AM +0300, Dan Aloni wrote:
> On Tue, Jul 01, 2014 at 09:39:43PM -0700, Loic Prylli wrote:
> > A bug was introduced in NETDEV_CHANGE notifier sequence causing the
> > arp table to be sometimes spuriously cleared (including manual arp
> > entries marked permanent), upon network link carrier changes.
> > 
> > The changed argument for the notifier was applied only to a single
> > caller of NETDEV_CHANGE, missing among others netdev_state_change().
> > So upon net_carrier events induced by the network, which are
> > triggering a call to netdev_state_change(), arp_netdev_event() would
> > decide whether to clear or not arp cache based on random/junk stack
> > values (a kind of read buffer overflow).
> [..]
> >  {
> >  	if (dev->flags & IFF_UP) {
> > -		call_netdevice_notifiers(NETDEV_CHANGE, dev);
> > +		struct netdev_notifier_change_info change_info;
> > +
> > +		change_info.flags_changed = 0;
> 
> I think it would be safer to do:
> 
>    struct netdev_notifier_change_info change_info = {};
> 
> So that when future fields are added to the struct and this call-site
> happens to be forgotten, they will get 0 by default rather than
> random stack values.

Thanks for the review. Will take into account suggestion. For the
record, another (possibly bigger) trap from the preexisting code that
remains (and caused the bug) is NETDEV_CHANGE being the only netdev
notifier with a different special calling sequence. Since calls to
NETDEV_CHANGE notifier have been reduced over time to the two
instances in net/core/dev.c, it hopefully won't be a problem (and
fixing that maintainability issue would be out-of-scope of this simple
low-risk bug fix).

Re: [PATCH] net: Fix NETDEV_CHANGE notifier usage causing spurious arp flush

[David Miller]

From: Loic Prylli <loicp@google.com>
Date: Tue,  1 Jul 2014 21:39:43 -0700

> A bug was introduced in NETDEV_CHANGE notifier sequence causing the
> arp table to be sometimes spuriously cleared (including manual arp
> entries marked permanent), upon network link carrier changes.
> 
> The changed argument for the notifier was applied only to a single
> caller of NETDEV_CHANGE, missing among others netdev_state_change().
> So upon net_carrier events induced by the network, which are
> triggering a call to netdev_state_change(), arp_netdev_event() would
> decide whether to clear or not arp cache based on random/junk stack
> values (a kind of read buffer overflow).
> 
> Fixes: be9efd365328 ("net: pass changed flags along with NETDEV_CHANGE event")
> Fixes: 6c8b4e3ff81b ("arp: flush arp cache on IFF_NOARP change")
> Signed-off-by: Loic Prylli <loicp@google.com>

Applied, thanks.

We should probably make plain call_netdevice_notifiers() BUG if it is
invoked for NETDEV_CHANGE.