netdev - Re: [PATCH] bonding: avoid adding slave device with IFF

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>] [day] [month] [year] [list]

Date:   Wed, 23 Jun 2021 02:11:34 +0000
From:   "zhudi (J)" <zhudi21@...wei.com>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
CC:     "vfalico@...il.com" <vfalico@...il.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Chenxiang (EulerOS)" <rose.chen@...wei.com>
Subject: Re: [PATCH] bonding: avoid adding slave device with IFF_MASTER flag

> 
> >From: Di Zhu <zhudi21@...wei.com>
> >
> >The following steps will definitely cause the kernel to crash:
> >	ip link add vrf1 type vrf table 1
> >	modprobe bonding.ko max_bonds=1
> >	echo "+vrf1" >/sys/class/net/bond0/bonding/slaves
> >	rmmod bonding
> >
> >The root cause is that: When the VRF is added to the slave device,
> >it will fail, and some cleaning work will be done. because VRF device
> >has IFF_MASTER flag, cleanup process  will not clear the IFF_BONDING flag.
> >Then, when we unload the bonding module,
> unregister_netdevice_notifier()
> >will treat the VRF device as a bond master device and treat netdev_priv()
> >as struct bonding{} which actually is struct net_vrf{}.
> >
> >By analyzing the processing logic of bond_enslave(), it seems that
> >it is not allowed to add the slave device with the IFF_MASTER flag, so
> >we need to add a code check for this situation.
> 
> 	I don't believe the statement just above is correct; nesting
> bonds has historically been permitted, even if it is of questionable
> value these days.  I've not tested nesting in a while, but last I recall
> it did function.
> 
> 	Leaving aside the question of whether it's really useful to nest
> bonds or not, my concern with disabling this is that it will break
> existing configurations that currently work fine.
> 
> 	However, it should be possible to use netif_is_bonding_master
> (which tests dev->flags & IFF_MASTER and dev->priv_flags & IFF_BONDING)
> to exclude IFF_MASTER devices that are not bonds (which seem to be vrf
> and eql), e.g.,
> 
> 	if ((slave_dev->flags & IFF_MASTER) &&
> 		!netif_is_bond_master(slave_dev))
> 	
> 	Or we can just go with this patch and see if anything breaks.
> 
> 	-J

	Thank you for your advice, as Eric dumazet described: since there is a usage scenario
about nesting bonding, we should not break it.

> 
> >Signed-off-by: Di Zhu <zhudi21@...wei.com>
> >---
> > drivers/net/bonding/bond_main.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> >diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> >index c5a646d06102..16840c9bc00d 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1601,6 +1601,12 @@ int bond_enslave(struct net_device *bond_dev,
> struct net_device *slave_dev,
> > 	int link_reporting;
> > 	int res = 0, i;
> >
> >+	if (slave_dev->flags & IFF_MASTER) {
> >+		netdev_err(bond_dev,
> >+			   "Error: Device with IFF_MASTER cannot be
> enslaved\n");
> >+		return -EPERM;
> >+	}
> >+
> > 	if (!bond->params.use_carrier &&
> > 	    slave_dev->ethtool_ops->get_link == NULL &&
> > 	    slave_ops->ndo_do_ioctl == NULL) {
> >--
> >2.23.0
> >
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@...onical.com