| Message ID | 20181116165001.30896-1-nikolay@cumulusnetworks.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v3] net: bridge: fix vlan stats use-after-free on destruction | expand |
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Date: Fri, 16 Nov 2018 18:50:01 +0200 > Syzbot reported a use-after-free of the global vlan context on port vlan > destruction. When I added per-port vlan stats I missed the fact that the > global vlan context can be freed before the per-port vlan rcu callback. > There're a few different ways to deal with this, I've chosen to add a > new private flag that is set only when per-port stats are allocated so > we can directly check it on destruction without dereferencing the global > context at all. The new field in net_bridge_vlan uses a hole. > > v2: cosmetic change, move the check to br_process_vlan_info where the > other checks are done > v3: add change log in the patch, add private (in-kernel only) flags in a > hole in net_bridge_vlan struct and use that instead of mixing > user-space flags with private flags > > Fixes: 9163a0fc1f0c ("net: bridge: add support for per-port vlan stats") > Reported-by: syzbot+04681da557a0e49a52e5@syzkaller.appspotmail.com > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Applied.
On Fri, 16 Nov 2018 18:50:01 +0200 Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > Syzbot reported a use-after-free of the global vlan context on port vlan > destruction. When I added per-port vlan stats I missed the fact that the > global vlan context can be freed before the per-port vlan rcu callback. > There're a few different ways to deal with this, I've chosen to add a > new private flag that is set only when per-port stats are allocated so > we can directly check it on destruction without dereferencing the global > context at all. The new field in net_bridge_vlan uses a hole. > > v2: cosmetic change, move the check to br_process_vlan_info where the > other checks are done > v3: add change log in the patch, add private (in-kernel only) flags in a > hole in net_bridge_vlan struct and use that instead of mixing > user-space flags with private flags > > Fixes: 9163a0fc1f0c ("net: bridge: add support for per-port vlan stats") > Reported-by: syzbot+04681da557a0e49a52e5@syzkaller.appspotmail.com > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Why not just use v->stats itself as the flag. Since free of NULL is a nop it would be cleaner?
On 24/04/2020 03:05, Stephen Hemminger wrote: > On Fri, 16 Nov 2018 18:50:01 +0200 > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > >> Syzbot reported a use-after-free of the global vlan context on port vlan >> destruction. When I added per-port vlan stats I missed the fact that the >> global vlan context can be freed before the per-port vlan rcu callback. >> There're a few different ways to deal with this, I've chosen to add a >> new private flag that is set only when per-port stats are allocated so >> we can directly check it on destruction without dereferencing the global >> context at all. The new field in net_bridge_vlan uses a hole. >> >> v2: cosmetic change, move the check to br_process_vlan_info where the >> other checks are done >> v3: add change log in the patch, add private (in-kernel only) flags in a >> hole in net_bridge_vlan struct and use that instead of mixing >> user-space flags with private flags >> >> Fixes: 9163a0fc1f0c ("net: bridge: add support for per-port vlan stats") >> Reported-by: syzbot+04681da557a0e49a52e5@syzkaller.appspotmail.com >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > Why not just use v->stats itself as the flag. > Since free of NULL is a nop it would be cleaner? > v->stats is *always* set while the vlan is published/visible, that's a guarantee I don't want to break because I'll have to add null checks in the fast-path. By the way this is a thread from 2018. :-) Cheers, Nik
On Fri, 16 Nov 2018 18:50:01 +0200 Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > + if (v->priv_flags & BR_VLFLAG_PER_PORT_STATS) > free_percpu(v->stats); Why not not v->stats == NULL as a flag instead? Then the fact that free_percpu(NULL) is a Nop would mean less code in the bridge driver.
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 2920e06a5403..04c19a37e500 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -102,12 +102,18 @@ struct br_tunnel_info { struct metadata_dst *tunnel_dst; }; +/* private vlan flags */ +enum { + BR_VLFLAG_PER_PORT_STATS = BIT(0), +}; + /** * struct net_bridge_vlan - per-vlan entry * * @vnode: rhashtable member * @vid: VLAN id * @flags: bridge vlan flags + * @priv_flags: private (in-kernel) bridge vlan flags * @stats: per-cpu VLAN statistics * @br: if MASTER flag set, this points to a bridge struct * @port: if MASTER flag unset, this points to a port struct @@ -127,6 +133,7 @@ struct net_bridge_vlan { struct rhash_head tnode; u16 vid; u16 flags; + u16 priv_flags; struct br_vlan_stats __percpu *stats; union { struct net_bridge *br; diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 8c9297a01947..e84be08b8285 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -197,7 +197,7 @@ static void nbp_vlan_rcu_free(struct rcu_head *rcu) v = container_of(rcu, struct net_bridge_vlan, rcu); WARN_ON(br_vlan_is_master(v)); /* if we had per-port stats configured then free them here */ - if (v->brvlan->stats != v->stats) + if (v->priv_flags & BR_VLFLAG_PER_PORT_STATS) free_percpu(v->stats); v->stats = NULL; kfree(v); @@ -264,6 +264,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) err = -ENOMEM; goto out_filt; } + v->priv_flags |= BR_VLFLAG_PER_PORT_STATS; } else { v->stats = masterv->stats; }
Syzbot reported a use-after-free of the global vlan context on port vlan destruction. When I added per-port vlan stats I missed the fact that the global vlan context can be freed before the per-port vlan rcu callback. There're a few different ways to deal with this, I've chosen to add a new private flag that is set only when per-port stats are allocated so we can directly check it on destruction without dereferencing the global context at all. The new field in net_bridge_vlan uses a hole. v2: cosmetic change, move the check to br_process_vlan_info where the other checks are done v3: add change log in the patch, add private (in-kernel only) flags in a hole in net_bridge_vlan struct and use that instead of mixing user-space flags with private flags Fixes: 9163a0fc1f0c ("net: bridge: add support for per-port vlan stats") Reported-by: syzbot+04681da557a0e49a52e5@syzkaller.appspotmail.com Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- After the jet lag mostly passed it has all come together in a cleaner and less future error-prone way. :) net/bridge/br_private.h | 7 +++++++ net/bridge/br_vlan.c | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-)