diff mbox series

[net-next] bridge: Fix possible use-after-free when deleting bridge port

Message ID 20190422093307.10206-1-idosch@mellanox.com
State Accepted
Delegated to: David Miller
Headers show
Series [net-next] bridge: Fix possible use-after-free when deleting bridge port | expand

Commit Message

Ido Schimmel April 22, 2019, 9:33 a.m. UTC 
When a bridge port is being deleted, do not dereference it later in
br_vlan_port_event() as it can result in a use-after-free [1] if the RCU
callback was executed before invoking the function.

[1]
[  129.638551] ==================================================================
[  129.646904] BUG: KASAN: use-after-free in br_vlan_port_event+0x53c/0x5fd
[  129.654406] Read of size 8 at addr ffff8881e4aa1ae8 by task ip/483
[  129.663008] CPU: 0 PID: 483 Comm: ip Not tainted 5.1.0-rc5-custom-02265-ga946bd73daac #1383
[  129.672359] Hardware name: Mellanox Technologies Ltd. MSN2100-CB2FO/SA001017, BIOS 5.6.5 06/07/2016
[  129.682484] Call Trace:
[  129.685242]  dump_stack+0xa9/0x10e
[  129.689068]  print_address_description.cold.2+0x9/0x25e
[  129.694930]  kasan_report.cold.3+0x78/0x9d
[  129.704420]  br_vlan_port_event+0x53c/0x5fd
[  129.728300]  br_device_event+0x2c7/0x7a0
[  129.741505]  notifier_call_chain+0xb5/0x1c0
[  129.746202]  rollback_registered_many+0x895/0xe90
[  129.793119]  unregister_netdevice_many+0x48/0x210
[  129.803384]  rtnl_delete_link+0xe1/0x140
[  129.815906]  rtnl_dellink+0x2a3/0x820
[  129.844166]  rtnetlink_rcv_msg+0x397/0x910
[  129.868517]  netlink_rcv_skb+0x137/0x3a0
[  129.882013]  netlink_unicast+0x49b/0x660
[  129.900019]  netlink_sendmsg+0x755/0xc90
[  129.915758]  ___sys_sendmsg+0x761/0x8e0
[  129.966315]  __sys_sendmsg+0xf0/0x1c0
[  129.988918]  do_syscall_64+0xa4/0x470
[  129.993032]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  129.998696] RIP: 0033:0x7ff578104b58
...
[  130.073811] Allocated by task 479:
[  130.077633]  __kasan_kmalloc.constprop.5+0xc1/0xd0
[  130.083008]  kmem_cache_alloc_trace+0x152/0x320
[  130.088090]  br_add_if+0x39c/0x1580
[  130.092005]  do_set_master+0x1aa/0x210
[  130.096211]  do_setlink+0x985/0x3100
[  130.100224]  __rtnl_newlink+0xc52/0x1380
[  130.104625]  rtnl_newlink+0x6b/0xa0
[  130.108541]  rtnetlink_rcv_msg+0x397/0x910
[  130.113136]  netlink_rcv_skb+0x137/0x3a0
[  130.117538]  netlink_unicast+0x49b/0x660
[  130.121939]  netlink_sendmsg+0x755/0xc90
[  130.126340]  ___sys_sendmsg+0x761/0x8e0
[  130.130645]  __sys_sendmsg+0xf0/0x1c0
[  130.134753]  do_syscall_64+0xa4/0x470
[  130.138864]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

[  130.146195] Freed by task 0:
[  130.149421]  __kasan_slab_free+0x125/0x170
[  130.154016]  kfree+0xf3/0x310
[  130.157349]  kobject_put+0x1a8/0x4c0
[  130.161363]  rcu_core+0x859/0x19b0
[  130.165175]  __do_softirq+0x250/0xa26
[  130.170956] The buggy address belongs to the object at ffff8881e4aa1ae8
                which belongs to the cache kmalloc-1k of size 1024
[  130.184972] The buggy address is located 0 bytes inside of
                1024-byte region [ffff8881e4aa1ae8, ffff8881e4aa1ee8)

Fixes: 9c0ec2e7182a ("bridge: support binding vlan dev link state to vlan member bridge ports")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Cc: Mike Manning <mmanning@vyatta.att-mail.com>
---
 net/bridge/br.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Nikolay Aleksandrov April 22, 2019, 9:44 a.m. UTC | #1 
On 22/04/2019 12:33, Ido Schimmel wrote:
> When a bridge port is being deleted, do not dereference it later in
> br_vlan_port_event() as it can result in a use-after-free [1] if the RCU
> callback was executed before invoking the function.
> 
[snip]
> 
> Fixes: 9c0ec2e7182a ("bridge: support binding vlan dev link state to vlan member bridge ports")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Cc: Mike Manning <mmanning@vyatta.att-mail.com>
> ---
>  net/bridge/br.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index e69fc87a13e0..3c8e4b38f054 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -129,7 +129,8 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>  		break;
>  	}
>  
> -	br_vlan_port_event(p, event);
> +	if (event != NETDEV_UNREGISTER)
> +		br_vlan_port_event(p, event);
>  
>  	/* Events that may cause spanning tree to refresh */
>  	if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||
> 

Right, the br_del_if is just above. :)

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Mike Manning April 22, 2019, 5:24 p.m. UTC | #2 
On 22/04/2019 10:44, Nikolay Aleksandrov wrote:
> On 22/04/2019 12:33, Ido Schimmel wrote:
>> When a bridge port is being deleted, do not dereference it later in
>> br_vlan_port_event() as it can result in a use-after-free [1] if the RCU
>> callback was executed before invoking the function.
>>
> [snip]
>> Fixes: 9c0ec2e7182a ("bridge: support binding vlan dev link state to vlan member bridge ports")
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> Cc: Mike Manning <mmanning@vyatta.att-mail.com>
>> ---
>>  net/bridge/br.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bridge/br.c b/net/bridge/br.c
>> index e69fc87a13e0..3c8e4b38f054 100644
>> --- a/net/bridge/br.c
>> +++ b/net/bridge/br.c
>> @@ -129,7 +129,8 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>>  		break;
>>  	}
>>  
>> -	br_vlan_port_event(p, event);
>> +	if (event != NETDEV_UNREGISTER)
>> +		br_vlan_port_event(p, event);
>>  
>>  	/* Events that may cause spanning tree to refresh */
>>  	if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||
>>
> Right, the br_del_if is just above. :)
>
> Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Thanks for the fix, to avoid this I should have placed the check for the
br opt under the case handling in br_vlan_port_event().

Acked-by: Mike Manning <mmanning@vyatta.att-mail.com>
David Miller April 23, 2019, 5:18 a.m. UTC | #3 
From: Ido Schimmel <idosch@mellanox.com>
Date: Mon, 22 Apr 2019 09:33:19 +0000

> When a bridge port is being deleted, do not dereference it later in
> br_vlan_port_event() as it can result in a use-after-free [1] if the RCU
> callback was executed before invoking the function.
> 
> [1]
 ...
> Fixes: 9c0ec2e7182a ("bridge: support binding vlan dev link state to vlan member bridge ports")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Cc: Mike Manning <mmanning@vyatta.att-mail.com>

Applied, thanks.
diff mbox series

Patch

diff --git a/net/bridge/br.c b/net/bridge/br.c
index e69fc87a13e0..3c8e4b38f054 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -129,7 +129,8 @@  static int br_device_event(struct notifier_block *unused, unsigned long event, v
 		break;
 	}
 
-	br_vlan_port_event(p, event);
+	if (event != NETDEV_UNREGISTER)
+		br_vlan_port_event(p, event);
 
 	/* Events that may cause spanning tree to refresh */
 	if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||