netdev - Re: [PATCH net] vlan: fix memory leak in vlan

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:   Fri, 8 Jul 2022 21:45:17 -0400
From:   Xin Long <lucien.xin@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        network dev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] vlan: fix memory leak in vlan_newlink()

On Fri, Jul 8, 2022 at 11:11 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> Blamed commit added back a bug I fixed in commit 9bbd917e0bec
> ("vlan: fix memory leak in vlan_dev_set_egress_priority")
>
> If a memory allocation fails in vlan_changelink() after other allocations
> succeeded, we need to call vlan_dev_free_egress_priority()
> to free all allocated memory because after a failed ->newlink()
> we do not call any methods like ndo_uninit() or dev->priv_destructor().
>
> In following example, if the allocation for last element 2000:2001 fails,
> we need to free eight prior allocations:
>
> ip link add link dummy0 dummy0.100 type vlan id 100 \
>         egress-qos-map 1:2 2:3 3:4 4:5 5:6 6:7 7:8 8:9 2000:2001
BTW, it seems that:

# ip link change link dummy0 dummy0.100 type vlan id 100
egress-qos-map 8:9 2003:2004

instead of changing qos-map to {8:1 2003:2004}, this cmd can only be
able to append the new qos-map "2003:2004".

Is this expected?

Thanks.

>
> syzbot report was:
>
> BUG: memory leak
> unreferenced object 0xffff888117bd1060 (size 32):
> comm "syz-executor408", pid 3759, jiffies 4294956555 (age 34.090s)
> hex dump (first 32 bytes):
> 09 00 00 00 00 a0 00 00 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:
> [<ffffffff83fc60ad>] kmalloc include/linux/slab.h:600 [inline]
> [<ffffffff83fc60ad>] vlan_dev_set_egress_priority+0xed/0x170 net/8021q/vlan_dev.c:193
> [<ffffffff83fc6628>] vlan_changelink+0x178/0x1d0 net/8021q/vlan_netlink.c:128
> [<ffffffff83fc67c8>] vlan_newlink+0x148/0x260 net/8021q/vlan_netlink.c:185
> [<ffffffff838b1278>] rtnl_newlink_create net/core/rtnetlink.c:3363 [inline]
> [<ffffffff838b1278>] __rtnl_newlink+0xa58/0xdc0 net/core/rtnetlink.c:3580
> [<ffffffff838b1629>] rtnl_newlink+0x49/0x70 net/core/rtnetlink.c:3593
> [<ffffffff838ac66c>] rtnetlink_rcv_msg+0x21c/0x5c0 net/core/rtnetlink.c:6089
> [<ffffffff839f9c37>] netlink_rcv_skb+0x87/0x1d0 net/netlink/af_netlink.c:2501
> [<ffffffff839f8da7>] netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
> [<ffffffff839f8da7>] netlink_unicast+0x397/0x4c0 net/netlink/af_netlink.c:1345
> [<ffffffff839f9266>] netlink_sendmsg+0x396/0x710 net/netlink/af_netlink.c:1921
> [<ffffffff8384dbf6>] sock_sendmsg_nosec net/socket.c:714 [inline]
> [<ffffffff8384dbf6>] sock_sendmsg+0x56/0x80 net/socket.c:734
> [<ffffffff8384e15c>] ____sys_sendmsg+0x36c/0x390 net/socket.c:2488
> [<ffffffff838523cb>] ___sys_sendmsg+0x8b/0xd0 net/socket.c:2542
> [<ffffffff838525b8>] __sys_sendmsg net/socket.c:2571 [inline]
> [<ffffffff838525b8>] __do_sys_sendmsg net/socket.c:2580 [inline]
> [<ffffffff838525b8>] __se_sys_sendmsg net/socket.c:2578 [inline]
> [<ffffffff838525b8>] __x64_sys_sendmsg+0x78/0xf0 net/socket.c:2578
> [<ffffffff845ad8d5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> [<ffffffff845ad8d5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> [<ffffffff8460006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> Fixes: 37aa50c539bc ("vlan: introduce vlan_dev_free_egress_priority")
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Xin Long <lucien.xin@...il.com>
> ---
>  net/8021q/vlan_netlink.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
> index 53b1955b027f89245eb8dd46ede9d7bfd6e553a3..214532173536b790cf032615f73fb3d868d2aae1 100644
> --- a/net/8021q/vlan_netlink.c
> +++ b/net/8021q/vlan_netlink.c
> @@ -182,10 +182,14 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
>         else if (dev->mtu > max_mtu)
>                 return -EINVAL;
>
> +       /* Note: If this initial vlan_changelink() fails, we need
> +        * to call vlan_dev_free_egress_priority() to free memory.
> +        */
>         err = vlan_changelink(dev, tb, data, extack);
> -       if (err)
> -               return err;
> -       err = register_vlan_dev(dev, extack);
> +
> +       if (!err)
> +               err = register_vlan_dev(dev, extack);
> +
>         if (err)
>                 vlan_dev_free_egress_priority(dev);
>         return err;
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>