KASAN: use-after-free Read in nbp_vlan_rcu_free

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: e12e00e388de Merge tag 'kbuild-fixes-v4.20' of git://git.k..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14cdb6f5400000
kernel config: https://syzkaller.appspot.com/x/.config?x=8f215f21f041a0d7
dashboard link: https://syzkaller.appspot.com/bug?extid=04681da557a0e49a52e5
compiler: gcc (GCC) 8.0.1 20180413 (experimental)

Unfortunately, I don't have any reproducer for this crash yet.

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+04681d...@syzkaller.appspotmail.com

device bridge_slave_1 left promiscuous mode
bridge0: port 2(bridge_slave_1) entered disabled state
device bridge_slave_0 left promiscuous mode
bridge0: port 1(bridge_slave_0) entered disabled state
==================================================================
BUG: KASAN: use-after-free in nbp_vlan_rcu_free+0x152/0x160
net/bridge/br_vlan.c:200
Read of size 8 at addr ffff8881bbc44a18 by task swapper/1/0

CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.20.0-rc1+ #332
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x244/0x39d lib/dump_stack.c:113
print_address_description.cold.7+0x9/0x1ff mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report.cold.8+0x242/0x309 mm/kasan/report.c:412
__asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
nbp_vlan_rcu_free+0x152/0x160 net/bridge/br_vlan.c:200
__rcu_reclaim kernel/rcu/rcu.h:240 [inline]
rcu_do_batch kernel/rcu/tree.c:2437 [inline]
invoke_rcu_callbacks kernel/rcu/tree.c:2716 [inline]
rcu_process_callbacks+0x100a/0x1ac0 kernel/rcu/tree.c:2697
__do_softirq+0x308/0xb7e kernel/softirq.c:292
invoke_softirq kernel/softirq.c:373 [inline]
irq_exit+0x17f/0x1c0 kernel/softirq.c:413
exiting_irq arch/x86/include/asm/apic.h:536 [inline]
smp_apic_timer_interrupt+0x1cb/0x760 arch/x86/kernel/apic/apic.c:1061
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:804
</IRQ>
RIP: 0010:native_safe_halt+0x6/0x10 arch/x86/include/asm/irqflags.h:57
Code: e9 2c ff ff ff 48 89 c7 48 89 45 d8 e8 d3 43 f3 f9 48 8b 45 d8 e9 ca
fe ff ff 48 89 df e8 c2 43 f3 f9 eb 82 55 48 89 e5 fb f4 <5d> c3 0f 1f 84
00 00 00 00 00 55 48 89 e5 f4 5d c3 90 90 90 90 90
RSP: 0018:ffff8881d9b27cb8 EFLAGS: 00000286 ORIG_RAX: ffffffffffffff13
RAX: dffffc0000000000 RBX: 1ffff1103b364f9b RCX: 0000000000000000
RDX: 1ffffffff12a3f71 RSI: 0000000000000001 RDI: ffffffff8951fb88
RBP: ffff8881d9b27cb8 R08: ffff8881d9b14340 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8881d9b27d78
R13: ffffffff8a14e1e0 R14: 0000000000000000 R15: 0000000000000001
arch_safe_halt arch/x86/include/asm/paravirt.h:151 [inline]
default_idle+0xbf/0x490 arch/x86/kernel/process.c:498
arch_cpu_idle+0x10/0x20 arch/x86/kernel/process.c:489
default_idle_call+0x6d/0x90 kernel/sched/idle.c:93
cpuidle_idle_call kernel/sched/idle.c:153 [inline]
do_idle+0x49b/0x5c0 kernel/sched/idle.c:262
cpu_startup_entry+0x18/0x20 kernel/sched/idle.c:353
start_secondary+0x487/0x5f0 arch/x86/kernel/smpboot.c:271
secondary_startup_64+0xa4/0xb0 arch/x86/kernel/head_64.S:243

Allocated by task 13858:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
kasan_kmalloc+0xc7/0xe0 mm/kasan/kasan.c:553
kmem_cache_alloc_trace+0x152/0x750 mm/slab.c:3620
kmalloc include/linux/slab.h:546 [inline]
kzalloc include/linux/slab.h:741 [inline]
br_vlan_add+0x6e5/0x1340 net/bridge/br_vlan.c:650
br_vlan_init+0x339/0x3e0 net/bridge/br_vlan.c:1047
br_dev_init+0x10d/0x2a0 net/bridge/br_device.c:134
register_netdevice+0x332/0x11d0 net/core/dev.c:8459
br_dev_newlink+0x2a/0x130 net/bridge/br_netlink.c:1309
rtnl_newlink+0xf08/0x1da0 net/core/rtnetlink.c:3175
rtnetlink_rcv_msg+0x46a/0xc20 net/core/rtnetlink.c:4947
netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2477
rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:4965
netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
netlink_unicast+0x5a5/0x760 net/netlink/af_netlink.c:1336
netlink_sendmsg+0xa18/0xfc0 net/netlink/af_netlink.c:1917
sock_sendmsg_nosec net/socket.c:621 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:631
___sys_sendmsg+0x7fd/0x930 net/socket.c:2116
__sys_sendmsg+0x11d/0x280 net/socket.c:2154
__do_sys_sendmsg net/socket.c:2163 [inline]
__se_sys_sendmsg net/socket.c:2161 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2161
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 9:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/kasan.c:521
kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
__cache_free mm/slab.c:3498 [inline]
kfree+0xcf/0x230 mm/slab.c:3817
br_master_vlan_rcu_free+0xa8/0xe0 net/bridge/br_vlan.c:174
__rcu_reclaim kernel/rcu/rcu.h:240 [inline]
rcu_do_batch kernel/rcu/tree.c:2437 [inline]
invoke_rcu_callbacks kernel/rcu/tree.c:2716 [inline]
rcu_process_callbacks+0x100a/0x1ac0 kernel/rcu/tree.c:2697
__do_softirq+0x308/0xb7e kernel/softirq.c:292

The buggy address belongs to the object at ffff8881bbc44a00
which belongs to the cache kmalloc-96 of size 96
The buggy address is located 24 bytes inside of
96-byte region [ffff8881bbc44a00, ffff8881bbc44a60)
The buggy address belongs to the page:
page:ffffea0006ef1100 count:1 mapcount:0 mapping:ffff8881da8004c0 index:0x0
flags: 0x2fffc0000000200(slab)
raw: 02fffc0000000200 ffffea0007471a08 ffffea000757b908 ffff8881da8004c0
raw: 0000000000000000 ffff8881bbc44000 0000000100000020 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff8881bbc44900: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
ffff8881bbc44980: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> ffff8881bbc44a00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
^
ffff8881bbc44a80: 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc fc
ffff8881bbc44b00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.

[nik...@cumulusnetworks.com]

On 12 November 2018 06:51:02 CET, syzbot <syzbot+04681d...@syzkaller.appspotmail.com> wrote:
>Hello,
>
>syzbot found the following crash on:
>
>HEAD commit: e12e00e388de Merge tag 'kbuild-fixes-v4.20' of
>git://git.k..
>git tree: upstream
>console output:
>https://syzkaller.appspot.com/x/log.txt?x=14cdb6f5400000
>kernel config:
>https://syzkaller.appspot.com/x/.config?x=8f215f21f041a0d7
>dashboard link:
>https://syzkaller.appspot.com/bug?extid=04681da557a0e49a52e5
>compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
>Unfortunately, I don't have any reproducer for this crash yet.
>
>IMPORTANT: if you fix the bug, please add the following tag to the
>commit:
>Reported-by: syzbot+04681d...@syzkaller.appspotmail.com

Thanks, I'm about to fly out for LPC. Will take a look in a day.

[Nikolay Aleksandrov]

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+04681d...@syzkaller.appspotmail.com
Signed-off-by: Nikolay Aleksandrov <nik...@cumulusnetworks.com>
---
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

[Nikolay Aleksandrov]

On 11/13/18 3:01 AM, Nikolay Aleksandrov 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 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+04681d...@syzkaller.appspotmail.com
> Signed-off-by: Nikolay Aleksandrov <nik...@cumulusnetworks.com>
> ---

I'll post v2 with a cosmetic change - move the check up one level where
it's more logical to be and the rest of the checks are done.

[Nikolay Aleksandrov]

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+04681d...@syzkaller.appspotmail.com
Signed-off-by: Nikolay Aleksandrov <nik...@cumulusnetworks.com>
---

v2: cosmetic change, move the check to br_process_vlan_info where the
other checks are done

include/uapi/linux/if_bridge.h | 3 +++
net/bridge/br_netlink.c | 4 ++++
net/bridge/br_vlan.c | 3 ++-
3 files changed, 9 insertions(+), 1 deletion(-)

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..a017ed566b67 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -573,6 +573,10 @@ static int br_process_vlan_info(struct net_bridge *br,
if (!vinfo_curr->vid || vinfo_curr->vid >= VLAN_VID_MASK)
return -EINVAL;

+ /* don't allow user-space control over private flags */

+ if (vinfo_curr->flags & BRIDGE_VLAN_INFO_PRIVATE_FLAGS)
+ return -EINVAL;
+
if (vinfo_curr->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
/* check if we are already processing a range */
if (*vinfo_last)

[Nikolay Aleksandrov]

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+04681d...@syzkaller.appspotmail.com
Signed-off-by: Nikolay Aleksandrov <nik...@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;

[David Miller]

From: Nikolay Aleksandrov <nik...@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+04681d...@syzkaller.appspotmail.com
> Signed-off-by: Nikolay Aleksandrov <nik...@cumulusnetworks.com>

Applied.

[Stephen Hemminger]

On Fri, 16 Nov 2018 18:50:01 +0200
Nikolay Aleksandrov <nik...@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+04681d...@syzkaller.appspotmail.com
> Signed-off-by: Nikolay Aleksandrov <nik...@cumulusnetworks.com>

Why not just use v->stats itself as the flag.
Since free of NULL is a nop it would be cleaner?

[Nikolay Aleksandrov]

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

[Stephen Hemminger]

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