Message-ID: <22643.1687456107@famine>
Date: Thu, 22 Jun 2023 10:48:27 -0700
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: Eric Dumazet <edumazet@...gle.com>
cc: "David S . Miller" <davem@...emloft.net>,
    Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
    netdev@...r.kernel.org, eric.dumazet@...il.com,
    syzbot <syzkaller@...glegroups.com>, Jarod Wilson <jarod@...hat.com>,
    Moshe Tal <moshet@...dia.com>, Jussi Maki <joamaki@...il.com>,
    Andy Gospodarek <andy@...yhouse.net>,
    Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net] bonding: do not assume skb mac_header is set

Eric Dumazet <edumazet@...gle.com> wrote:

>Drivers must not assume in their ndo_start_xmit() that
>skbs have their mac_header set. skb->data is all what is needed.
>
>bonding seems to be one of the last offender as caught by syzbot:
>
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>Modules linked in:
>CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
>Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
>RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
>RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
>RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
>RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
>RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
>RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
>RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
>R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
>R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
>FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>Call Trace:
><TASK>
>[<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
>[<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
>[<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
>[<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
>[<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
>[<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
>[<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
>[<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
>[<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
>[<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
>[<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
>[<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
>[<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>[<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
>[<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>Fixes: 7b8fc0103bb5 ("bonding: add a vlan+srcmac tx hashing option")
>Reported-by: syzbot <syzkaller@...glegroups.com>
>Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>Cc: Jarod Wilson <jarod@...hat.com>
>Cc: Moshe Tal <moshet@...dia.com>
>Cc: Jussi Maki <joamaki@...il.com>
>Cc: Jay Vosburgh <j.vosburgh@...il.com>
>Cc: Andy Gospodarek <andy@...yhouse.net>
>Cc: Vladimir Oltean <vladimir.oltean@....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 edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> 		return skb->hash;
> 
> 	return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
>-				skb_mac_offset(skb), skb_network_offset(skb),
>+				0, skb_network_offset(skb),
> 				skb_headlen(skb));
> }

	Is the MAC header guaranteed to be at skb->data, then?  If not,
then isn't replacing skb_mac_offset() with 0 going to break the hash (as
it might or might not be looking at the actual MAC header)?

	Also, assuming for the moment that this change is ok, this makes
all callers of __bond_xmit_hash() supply zero for the mhoff parameter,
and a complete fix should therefore remove the unused parameter and its
various references.

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com

Powered by blists - more mailing lists