Re: [PATCH] bonding: 3ad: fix a use-after-free in bond_3ad_state_machine_handle

quoted

From: Di Zhu <redacted>

I use the similar test method described in link below with KASAN enabled:
https://lore.kernel.org/netdev/4c5e467e07fb410ab4135b391d663ec1@hua

wei.

quoted

com/
soon after, KASAN reports:
[ 9041.977110]
=========================================================

=========

quoted

[ 9041.977151] BUG: KASAN: use-after-free in
bond_3ad_state_machine_handler+0x1c34/0x20b0 [bonding]

	What line of code is this?

Around  line 1022 in current net-next , bond_3ad_state_machine_handler()->ad_mux_machine():
	} else {
				/* if port state hasn't changed make
				 * sure that a collecting distributing
				 * port in an active aggregator is enabled
				 */
				if (port->aggregator &&
-------------------->		port->aggregator->is_active &&
				    !__port_is_enabled(port)) {

					__enable_port(port);
				}
			}

the aggregator pointed by port->aggregator has released

quoted

[ 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

quoted

[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) {

	Yes,  it is right

quoted

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.

quoted

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.

	Maybe the issue your patch want to fix is likely another one from the point 
of view of code logic. we are still testing with the patch I submitted to see whether
all the problems have been dealt with. The recurrence probability is relatively low, 
so need a long time...

quoted hunk

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

quoted

Signed-off-by: Di Zhu <redacted>
---
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 @@

quoted

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@canonical.com

Thanks