netdev - Re: [PATCH net] bonding: Fix memory leak when changing bond type to Ethernet

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:   Tue, 18 Apr 2023 17:00:06 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Ido Schimmel <idosch@...dia.com>
cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, edumazet@...gle.com, andy@...yhouse.net,
        monis@...taire.COM, razor@...ckwall.org,
        mirsad.todorovac@....unizg.hr
Subject: Re: [PATCH net] bonding: Fix memory leak when changing bond type to Ethernet

Ido Schimmel <idosch@...dia.com> wrote:

>When a net device is put administratively up, its 'IFF_UP' flag is set
>(if not set already) and a 'NETDEV_UP' notification is emitted, which
>causes the 8021q driver to add VLAN ID 0 on the device. The reverse
>happens when a net device is put administratively down.
>
>When changing the type of a bond to Ethernet, its 'IFF_UP' flag is
>incorrectly cleared, resulting in the kernel skipping the above process
>and VLAN ID 0 being leaked [1].
>
>Fix by restoring the flag when changing the type to Ethernet, in a
>similar fashion to the restoration of the 'IFF_SLAVE' flag.
>
>The issue can be reproduced using the script in [2], with example out
>before and after the fix in [3].
>
>[1]
>unreferenced object 0xffff888103479900 (size 256):
>  comm "ip", pid 329, jiffies 4294775225 (age 28.561s)
>  hex dump (first 32 bytes):
>    00 a0 0c 15 81 88 ff ff 00 00 00 00 00 00 00 00  ................
>    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>  backtrace:
>    [<ffffffff81a6051a>] kmalloc_trace+0x2a/0xe0
>    [<ffffffff8406426c>] vlan_vid_add+0x30c/0x790
>    [<ffffffff84068e21>] vlan_device_event+0x1491/0x21a0
>    [<ffffffff81440c8e>] notifier_call_chain+0xbe/0x1f0
>    [<ffffffff8372383a>] call_netdevice_notifiers_info+0xba/0x150
>    [<ffffffff837590f2>] __dev_notify_flags+0x132/0x2e0
>    [<ffffffff8375ad9f>] dev_change_flags+0x11f/0x180
>    [<ffffffff8379af36>] do_setlink+0xb96/0x4060
>    [<ffffffff837adf6a>] __rtnl_newlink+0xc0a/0x18a0
>    [<ffffffff837aec6c>] rtnl_newlink+0x6c/0xa0
>    [<ffffffff837ac64e>] rtnetlink_rcv_msg+0x43e/0xe00
>    [<ffffffff839a99e0>] netlink_rcv_skb+0x170/0x440
>    [<ffffffff839a738f>] netlink_unicast+0x53f/0x810
>    [<ffffffff839a7fcb>] netlink_sendmsg+0x96b/0xe90
>    [<ffffffff8369d12f>] ____sys_sendmsg+0x30f/0xa70
>    [<ffffffff836a6d7a>] ___sys_sendmsg+0x13a/0x1e0
>unreferenced object 0xffff88810f6a83e0 (size 32):
>  comm "ip", pid 329, jiffies 4294775225 (age 28.561s)
>  hex dump (first 32 bytes):
>    a0 99 47 03 81 88 ff ff a0 99 47 03 81 88 ff ff  ..G.......G.....
>    81 00 00 00 01 00 00 00 cc cc cc cc cc cc cc cc  ................
>  backtrace:
>    [<ffffffff81a6051a>] kmalloc_trace+0x2a/0xe0
>    [<ffffffff84064369>] vlan_vid_add+0x409/0x790
>    [<ffffffff84068e21>] vlan_device_event+0x1491/0x21a0
>    [<ffffffff81440c8e>] notifier_call_chain+0xbe/0x1f0
>    [<ffffffff8372383a>] call_netdevice_notifiers_info+0xba/0x150
>    [<ffffffff837590f2>] __dev_notify_flags+0x132/0x2e0
>    [<ffffffff8375ad9f>] dev_change_flags+0x11f/0x180
>    [<ffffffff8379af36>] do_setlink+0xb96/0x4060
>    [<ffffffff837adf6a>] __rtnl_newlink+0xc0a/0x18a0
>    [<ffffffff837aec6c>] rtnl_newlink+0x6c/0xa0
>    [<ffffffff837ac64e>] rtnetlink_rcv_msg+0x43e/0xe00
>    [<ffffffff839a99e0>] netlink_rcv_skb+0x170/0x440
>    [<ffffffff839a738f>] netlink_unicast+0x53f/0x810
>    [<ffffffff839a7fcb>] netlink_sendmsg+0x96b/0xe90
>    [<ffffffff8369d12f>] ____sys_sendmsg+0x30f/0xa70
>    [<ffffffff836a6d7a>] ___sys_sendmsg+0x13a/0x1e0
>
>[2]
>ip link add name t-nlmon type nlmon
>ip link add name t-dummy type dummy
>ip link add name t-bond type bond mode active-backup
>
>ip link set dev t-bond up
>ip link set dev t-nlmon master t-bond
>ip link set dev t-nlmon nomaster
>ip link show dev t-bond
>ip link set dev t-dummy master t-bond
>ip link show dev t-bond
>
>ip link del dev t-bond
>ip link del dev t-dummy
>ip link del dev t-nlmon
>
>[3]
>Before:
>
>12: t-bond: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>    link/netlink
>12: t-bond: <BROADCAST,MULTICAST,MASTER,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>    link/ether 46:57:39:a4:46:a2 brd ff:ff:ff:ff:ff:ff
>
>After:
>
>12: t-bond: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>    link/netlink
>12: t-bond: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>    link/ether 66:48:7b:74:b6:8a brd ff:ff:ff:ff:ff:ff
>
>Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
>Fixes: 75c78500ddad ("bonding: remap muticast addresses without using dev_close() and dev_open()")
>Fixes: 9ec7eb60dcbc ("bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type change")
>Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
>Link: https://lore.kernel.org/netdev/78a8a03b-6070-3e6b-5042-f848dab16fb8@alu.unizg.hr/
>Tested-by: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
>Signed-off-by: Ido Schimmel <idosch@...dia.com>

	Note that this has nothing to do with nlmon specifically, it's
related to anything that's not ARPHRD_ETHER.

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

	-J

>---
> drivers/net/bonding/bond_main.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 8cc9a74789b7..7a7d584f378a 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1777,14 +1777,15 @@ void bond_lower_state_changed(struct slave *slave)
> 
> /* The bonding driver uses ether_setup() to convert a master bond device
>  * to ARPHRD_ETHER, that resets the target netdevice's flags so we always
>- * have to restore the IFF_MASTER flag, and only restore IFF_SLAVE if it was set
>+ * have to restore the IFF_MASTER flag, and only restore IFF_SLAVE and IFF_UP
>+ * if they were set
>  */
> static void bond_ether_setup(struct net_device *bond_dev)
> {
>-	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
>+	unsigned int flags = bond_dev->flags & (IFF_SLAVE | IFF_UP);
> 
> 	ether_setup(bond_dev);
>-	bond_dev->flags |= IFF_MASTER | slave_flag;
>+	bond_dev->flags |= IFF_MASTER | flags;
> 	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> }
> 
>-- 
>2.37.3

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