Linux Ethernet Bridging - Apr 2020 - [Bridge] [PATCH net v3] net: bridge: fix vlan stats use-after-free on destruction

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 at syzkaller.appspotmail.com
Signed-off-by: Nikolay Aleksandrov <nikolay at 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(-)

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;
 		}
-- 
2.17.2

From: Nikolay Aleksandrov <nikolay at 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 at syzkaller.appspotmail.com
> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Applied.

On Fri, 16 Nov 2018 18:50:01 +0200
Nikolay Aleksandrov <nikolay at 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 at syzkaller.appspotmail.com
> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Why not just use v->stats itself as the flag.
Since free of NULL is a nop it would be cleaner?

On Fri, 16 Nov 2018 18:50:01 +0200
Nikolay Aleksandrov <nikolay at 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.