netdev - Re: [PATCH net] bonding: avoid defaulting hard_header_len to ETH

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]

Message-ID: <27018.1493334502@famine>
Date:   Thu, 27 Apr 2017 16:08:22 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Paolo Abeni <pabeni@...hat.com>
cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Honggang LI <honli@...hat.com>, linux-rdma@...r.kernel.org
Subject: Re: [PATCH net] bonding: avoid defaulting hard_header_len to ETH_HLEN on slave removal

Paolo Abeni <pabeni@...hat.com> wrote:

>On slave list updates, the bonding driver computes its hard_header_len
>as the maximum of all enslaved devices's hard_header_len.
>If the slave list is empty, e.g. on last enslaved device removal,
>ETH_HLEN is used.
>
>Since the bonding header_ops are set only when the first enslaved
>device is attached, the above can lead to header_ops->create()
>being called with the wrong skb headroom in place.
>
>If bond0 is configured on top of ipoib devices, with the
>following commands:
>
>ifup bond0
>for slave in $BOND_SLAVES_LIST; do
>	ip link set dev $slave nomaster
>done
>ping -c 1 <ip on bond0 subnet>
>
>we will obtain a skb_under_panic() with a similar call trace:
>	skb_push+0x3d/0x40
>	push_pseudo_header+0x17/0x30 [ib_ipoib]
>	ipoib_hard_header+0x4e/0x80 [ib_ipoib]
>	arp_create+0x12f/0x220
>	arp_send_dst.part.19+0x28/0x50
>	arp_solicit+0x115/0x290
>	neigh_probe+0x4d/0x70
>	__neigh_event_send+0xa7/0x230
>	neigh_resolve_output+0x12e/0x1c0
>	ip_finish_output2+0x14b/0x390
>	ip_finish_output+0x136/0x1e0
>	ip_output+0x76/0xe0
>	ip_local_out+0x35/0x40
>	ip_send_skb+0x19/0x40
>	ip_push_pending_frames+0x33/0x40
>	raw_sendmsg+0x7d3/0xb50
>	inet_sendmsg+0x31/0xb0
>	sock_sendmsg+0x38/0x50
>	SYSC_sendto+0x102/0x190
>	SyS_sendto+0xe/0x10
>	do_syscall_64+0x67/0x180
>	entry_SYSCALL64_slow_path+0x25/0x25
>
>This change addresses the issue avoiding updating the bonding device
>hard_header_len when the slaves list become empty, forbidding to
>shrink it below the value used by header_ops->create().
>
>The bug is there since commit 54ef31371407 ("[PATCH] bonding: Handle large
>hard_header_len") but the panic can be triggered only since
>commit fc791b633515 ("IB/ipoib: move back IB LL address into the hard
>header").
>
>Reported-by: Norbert P <noe@...sik.uzh.ch>
>Fixes: 54ef31371407 ("[PATCH] bonding: Handle large hard_header_len")
>Fixes: fc791b633515 ("IB/ipoib: move back IB LL address into the hard header")
>Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
>Signed-off-by: Paolo Abeni <pabeni@...hat.com>

Signed-off-by: Jay Vosburgh <jay.vosburgh@...onical.com>


> drivers/net/bonding/bond_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 8a4ba8b..34481c9 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1104,11 +1104,11 @@ static void bond_compute_features(struct bonding *bond)
> 		gso_max_size = min(gso_max_size, slave->dev->gso_max_size);
> 		gso_max_segs = min(gso_max_segs, slave->dev->gso_max_segs);
> 	}
>+	bond_dev->hard_header_len = max_hard_header_len;
> 
> done:
> 	bond_dev->vlan_features = vlan_features;
> 	bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
>-	bond_dev->hard_header_len = max_hard_header_len;
> 	bond_dev->gso_max_segs = gso_max_segs;
> 	netif_set_gso_max_size(bond_dev, gso_max_size);
> 
>-- 
>2.9.3
>