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]

Date:   Thu, 27 Apr 2017 19:54:09 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>,
        "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

On Thu, Apr 27, 2017 at 07:29:34PM +0200, Paolo Abeni 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>
> ---

Thanks Paolo.

>  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
>