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:   Sat, 9 Jul 2022 12:24:58 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Xin Long <lucien.xin@...il.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 Sat, Jul 9, 2022 at 3:45 AM Xin Long <lucien.xin@...il.com> wrote:
>
> 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?
>

I think this is expected, kernel only supports additions of new items.
Otherwise a real RCU protection would be needed.

Speaking of (lack of) RCU, it seems we need the following orthogonal fix.

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index e00c4ee81ff7f82e4343fe45c14d8e5d81d80e95..bbe2c73ca74aeaccb91d2bde71c59c60c53ec515
100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -202,8 +202,8 @@ vlan_dev_get_egress_qos_mask(struct net_device
*dev, u32 skprio)
        struct vlan_priority_tci_mapping *mp;

        smp_rmb(); /* coupled with smp_wmb() in
vlan_dev_set_egress_priority() */
-
-       mp = vlan_dev_priv(dev)->egress_priority_map[(skprio & 0xF)];
+       /* Paired with WRITE_ONCE() in vlan_dev_set_egress_priority() */
+       mp = READ_ONCE(vlan_dev_priv(dev)->egress_priority_map[skprio & 0xF]);
        while (mp) {
                if (mp->priority == skprio) {
                        return mp->vlan_qos; /* This should already be shifted
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 035812b0461cc4b403f1a80bfdb9cfd9f44e4b45..5cb4d5121a71b012000de14f4e919cd3bf42a44b
100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -202,7 +202,8 @@ int vlan_dev_set_egress_priority(const struct
net_device *dev,
         * coupled with smp_rmb() in vlan_dev_get_egress_qos_mask()
         */
        smp_wmb();
-       vlan->egress_priority_map[skb_prio & 0xF] = np;
+       /* Paired with READ_ONCE() in vlan_dev_get_egress_qos_mask() */
+       WRITE_ONCE(vlan->egress_priority_map[skb_prio & 0xF], np);
        if (vlan_qos)
                vlan->nr_egress_mappings++;
        return 0;