linux-kernel - Re: [PATCH net-next 4/6] net: use core MTU range checking in core net infra

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 linux-hardening linux-cve-announce 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:   Wed, 19 Oct 2016 11:46:38 -0400
From:   Jarod Wilson <jarod@...hat.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Tom Herbert <tom@...bertland.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        Paolo Abeni <pabeni@...hat.com>, Jiri Benc <jbenc@...hat.com>,
        WANG Cong <xiyou.wangcong@...il.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        Pravin B Shelar <pshelar@....org>,
        Patrick McHardy <kaber@...sh.net>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Pravin Shelar <pshelar@...ira.com>
Subject: Re: [PATCH net-next 4/6] net: use core MTU range checking in core
 net infra

On Wed, Oct 19, 2016 at 05:28:00PM +0200, Sabrina Dubroca wrote:
> 2016-10-19, 10:40:06 -0400, Jarod Wilson wrote:
> > On Wed, Oct 19, 2016 at 03:55:29PM +0200, Sabrina Dubroca wrote:
> > > 2016-10-18, 22:33:31 -0400, Jarod Wilson wrote:
...
> > I'm thinking more and more that we ought to back out the patch that sets
> > min/max in ether_setup, save it for last, after we're sure everyone that
> > calls it has been prepared.
> 
> I'm not sure how that would work now, if some of the patches that
> already went in for ethernet drivers assume that ether_setup will
> configure a basic {min,max}_mtu pair (at least e100 makes that
> assumption, but that might be the only one).

Argh. Yeah. Hrm. Would have to do the revert *and* have e100 and possibly
others set their own min/max pair. So I guess it's a race to fix all the
fallout... Crap.

> > > [...]
> > > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> > > > index 89a687f..81fc79a 100644
> > > > --- a/net/bridge/br_device.c
> > > > +++ b/net/bridge/br_device.c
> > > > @@ -184,17 +184,15 @@ static struct rtnl_link_stats64 *br_get_stats64(struct net_device *dev,
> > > >  
> > > >  static int br_change_mtu(struct net_device *dev, int new_mtu)
> > > >  {
> > > > +#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> > > >  	struct net_bridge *br = netdev_priv(dev);
> > > > -	if (new_mtu < 68 || new_mtu > br_min_mtu(br))
> > > > -		return -EINVAL;
> > > > -
> > > > -	dev->mtu = new_mtu;
> > > >  
> > > > -#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> > > >  	/* remember the MTU in the rtable for PMTU */
> > > >  	dst_metric_set(&br->fake_rtable.dst, RTAX_MTU, new_mtu);
> > > >  #endif
> > > >  
> > > > +	dev->mtu = new_mtu;
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -390,6 +388,7 @@ void br_dev_setup(struct net_device *dev)
> > > >  	dev->hw_features = COMMON_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
> > > >  			   NETIF_F_HW_VLAN_STAG_TX;
> > > >  	dev->vlan_features = COMMON_FEATURES;
> > > > +	dev->max_mtu = br_min_mtu(br);
> > > 
> > > br_min_mtu uses br->port_list, which is only initialized a few lines
> > > later (right after the spin_lock_init() at the end of the context of
> > > this diff).
> > 
> > Ah, okay, I'd just grouped it with the other dev->foo settings.
> > 
> > > Besides, I don't think this works: br_min_mtu(br) changes when you add
> > > and remove ports, or when you change the MTU of an enslaved
> > > device. But this makes the max MTU for the bridge fixed (to 1500).
> > 
> > Okay, how about this: set no max_mtu (or set it to IP_MAX_MTU/65535), and
> > then retain a check against the possibly ever-changing br_min_mtu(br) in
> > br_change_mtu()?
> 
> Sounds good to me.

I think I have something here locally that looks sane. Working on a few
other similar cases now.

-- 
Jarod Wilson
jarod@...hat.com