lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 7 Jul 2014 13:34:32 -0700
From:	Loic Prylli <loicp@...gle.com>
To:	Dan Aloni <dan@...nelim.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	timo.teras@....fi, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH] net: Fix NETDEV_CHANGE notifier usage causing spurious
 arp flush

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).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists