| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 06 Mar 2021 00:16:50 -0800
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: zhudi <zhudi21@...wei.com>
cc: davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
rose.chen@...wei.com
Subject: Re: [PATCH] bonding: 3ad: fix a use-after-free in bond_3ad_state_machine_handle
zhudi <zhudi21@...wei.com> wrote:
>From: Di Zhu <zhudi21@...wei.com>
>
>I use the similar test method described in link below with KASAN enabled:
>https://lore.kernel.org/netdev/4c5e467e07fb410ab4135b391d663ec1@huawei.com/
>soon after, KASAN reports:
>[ 9041.977110] ==================================================================
>[ 9041.977151] BUG: KASAN: use-after-free in bond_3ad_state_machine_handler+0x1c34/0x20b0 [bonding]
What line of code is this?
>[ 9041.977156] Read of size 2 at addr ffff80394b8d70b0 by task kworker/u192:2/78492
>
>[ 9041.977187] Workqueue: bond0 bond_3ad_state_machine_handler [bonding]
>[ 9041.977190] Call trace:
>[ 9041.977197] dump_backtrace+0x0/0x310
>[ 9041.977201] show_stack+0x28/0x38
>[ 9041.977207] dump_stack+0xec/0x15c
>[ 9041.977213] print_address_description+0x68/0x2d0
>[ 9041.977217] kasan_report+0x130/0x2f0
>[ 9041.977221] __asan_load2+0x80/0xa8
>[ 9041.977238] bond_3ad_state_machine_handler+0x1c34/0x20b0 [bonding]
>
>[ 9041.977261] Allocated by task 138336:
>[ 9041.977266] kasan_kmalloc+0xe0/0x190
>[ 9041.977271] kmem_cache_alloc_trace+0x1d8/0x468
>[ 9041.977288] bond_enslave+0x514/0x2160 [bonding]
>[ 9041.977305] bond_option_slaves_set+0x188/0x2c8 [bonding]
>[ 9041.977323] __bond_opt_set+0x1b0/0x740 [bonding]
>
>[ 9041.977420] Freed by task 105873:
>[ 9041.977425] __kasan_slab_free+0x120/0x228
>[ 9041.977429] kasan_slab_free+0x10/0x18
>[ 9041.977432] kfree+0x90/0x468
>[ 9041.977448] slave_kobj_release+0x7c/0x98 [bonding]
>[ 9041.977452] kobject_put+0x118/0x328
>[ 9041.977468] __bond_release_one+0x688/0xa08 [bonding]
>[ 9041.977660] pci_device_remove+0x80/0x198
>
>The root cause is that in bond_3ad_unbind_slave() the last step is
>detach the port from aggregator including it. if find this aggregator
>and it has not any active ports, it will call ad_clear_agg() to do clear
>things, especially set aggregator->lag_ports = NULL.
By "last step," are you referring to the following logic near
the end of bond_3ad_unbind_slave(), around line 2229 in the current
net-next?
temp_aggregator->num_of_ports--;
if (__agg_active_ports(temp_aggregator) == 0) {
select_new_active_agg = temp_aggregator->is_active;
ad_clear_agg(temp_aggregator);
if (select_new_active_agg) {
>But ports in aggregator->lag_ports list which is set to NULL previously
>still has pointer to this aggregator through port->aggregator, event after
>this aggregator has released.
This, I think, is the real problem. If aggregator->num_of_ports
is zero, then there should not be any port->aggregator pointing to it.
>The use-after-free problem will cause some puzzling situactions,
>i am not sure whether fix this problem can solve all the problems mentioned
>by the link described earlier, but it did solve all problems i encountered.
I'm not sure, either, although the issues may be related
somehow. I've been testing with the following, but in light of this
patch, I'm not sure that mine is an actual fix, either, since I'm not
able to reproduce the actual issue and have forced it. Or maybe there
are multiple issues here.
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6908822d9773..546adda42715 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1537,6 +1537,12 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
slave_err(bond->dev, port->slave->dev,
"Port %d did not find a suitable aggregator\n",
port->actor_port_number);
+
+ port->sm_vars |= AD_PORT_BEGIN;
+ aggregator = __get_first_agg(port);
+ ad_agg_selection_logic(aggregator, update_slave_arr);
+
+ return;
}
}
/* if all aggregator's ports are READY_N == TRUE, set ready=TRUE
--
2.17.1
-J
>Signed-off-by: Di Zhu <zhudi21@...wei.com>
>---
> drivers/net/bonding/bond_3ad.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 6908822d9773..5d5a903e899c 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -1793,6 +1793,8 @@ static void ad_agg_selection_logic(struct aggregator *agg,
> static void ad_clear_agg(struct aggregator *aggregator)
> {
> if (aggregator) {
>+ struct port *port;
>+
> aggregator->is_individual = false;
> aggregator->actor_admin_aggregator_key = 0;
> aggregator->actor_oper_aggregator_key = 0;
>@@ -1801,6 +1803,10 @@ static void ad_clear_agg(struct aggregator *aggregator)
> aggregator->partner_oper_aggregator_key = 0;
> aggregator->receive_state = 0;
> aggregator->transmit_state = 0;
>+ for (port = aggregator->lag_ports; port;
>+ port = port->next_port_in_aggregator)
>+ if (port->aggregator == aggregator)
>+ port->aggregator = NULL;
> aggregator->lag_ports = NULL;
> aggregator->is_active = 0;
> aggregator->num_of_ports = 0;
>--
>2.23.0
>
---
-Jay Vosburgh, jay.vosburgh@...onical.com
Powered by blists - more mailing lists