From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: [PATCH net] net: bridge: fix per-port vlan stats use-after-free on destruction Date: Tue, 13 Nov 2018 03:01:54 +0200 Message-ID: <20181113010154.13935-1-nikolay@cumulusnetworks.com> References: <00000000000047feb5057a714975@google.com> Cc: roopa@cumulusnetworks.com, davem@davemloft.net, bridge@lists.linux-foundation.org, syzkaller-bugs@googlegroups.com, Nikolay Aleksandrov To: netdev@vger.kernel.org Return-path: Received: from mail-pf1-f196.google.com ([209.85.210.196]:32934 "EHLO mail-pf1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726028AbeKMK5m (ORCPT ); Tue, 13 Nov 2018 05:57:42 -0500 Received: by mail-pf1-f196.google.com with SMTP id v68-v6so5150989pfk.0 for ; Mon, 12 Nov 2018 17:02:02 -0800 (PST) In-Reply-To: <00000000000047feb5057a714975@google.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 flag is internally controlled by the kernel and user-space isn't allowed to set it. Fixes: 9163a0fc1f0c ("net: bridge: add support for per-port vlan stats") Reported-by: syzbot+04681da557a0e49a52e5@syzkaller.appspotmail.com Signed-off-by: Nikolay Aleksandrov --- include/uapi/linux/if_bridge.h | 3 +++ net/bridge/br_netlink.c | 7 ++++++- net/bridge/br_vlan.c | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h index e41eda3c71f1..fa1f72276712 100644 --- a/include/uapi/linux/if_bridge.h +++ b/include/uapi/linux/if_bridge.h @@ -130,6 +130,9 @@ enum { #define BRIDGE_VLAN_INFO_RANGE_BEGIN (1<<3) /* VLAN is start of vlan range */ #define BRIDGE_VLAN_INFO_RANGE_END (1<<4) /* VLAN is end of vlan range */ #define BRIDGE_VLAN_INFO_BRENTRY (1<<5) /* Global bridge VLAN entry */ +#define BRIDGE_VLAN_INFO_PORT_STATS (1<<6) /* Per-port VLAN stats */ + +#define BRIDGE_VLAN_INFO_PRIVATE_FLAGS BRIDGE_VLAN_INFO_PORT_STATS struct bridge_vlan_info { __u16 flags; diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 3345f1984542..c600fedcfb76 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -527,8 +527,12 @@ int br_getlink(struct sk_buff *skb, u32 pid, u32 seq, static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p, int cmd, struct bridge_vlan_info *vinfo, bool *changed) { + int err = -EINVAL; bool curr_change; - int err = 0; + + /* don't allow user-space control over private flags */ + if (vinfo->flags & BRIDGE_VLAN_INFO_PRIVATE_FLAGS) + return err; switch (cmd) { case RTM_SETLINK: @@ -548,6 +552,7 @@ static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p, break; case RTM_DELLINK: + err = 0; if (p) { if (!nbp_vlan_delete(p, vinfo->vid)) *changed = true; diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 8c9297a01947..004e1f8c5040 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->flags & BRIDGE_VLAN_INFO_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->flags |= BRIDGE_VLAN_INFO_PORT_STATS; } else { v->stats = masterv->stats; } -- 2.17.2