Messages in this thread | | | From | Jay Vosburgh <> | Subject | Re: [PATCH net] net: bonding: fix use-after-free after 802.3ad slave unbind | Date | Wed, 29 Jun 2022 11:58:52 -0700 |
| |
Yevhen Orlov <yevhen.orlov@plvision.eu> wrote:
>commit 0622cab0341c ("bonding: fix 802.3ad aggregator reselection"), >resolve case, when there is several aggregation groups in the same bond. >bond_3ad_unbind_slave will invalidate (clear) aggregator when >__agg_active_ports return zero. So, ad_clear_agg can be executed even, when >num_of_ports!=0. Than bond_3ad_unbind_slave can be executed again for, >previously cleared aggregator. NOTE: at this time bond_3ad_unbind_slave >will not update slave ports list, because lag_ports==NULL. So, here we >got slave ports, pointing to freed aggregator memory. > >Fix with checking actual number of ports in group (as was before >commit 0622cab0341c ("bonding: fix 802.3ad aggregator reselection") ), >before ad_clear_agg().
To be clear, what it looks like is going on is that, after 0622cab0341c, we're getting to this point with an aggregator that contains the port being removed and a non-zero number of inactive ports. The extant logic is for the "old way" (no inactive ports in an agg), and presumes that if __agg_active_ports() == 0 then the agg is empty, which isn't a safe assumption in the current code.
Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>
-J
>The KASAN logs are as follows: > >[ 767.617392] ================================================================== >[ 767.630776] BUG: KASAN: use-after-free in bond_3ad_state_machine_handler+0x13dc/0x1470 >[ 767.638764] Read of size 2 at addr ffff00011ba9d430 by task kworker/u8:7/767 >[ 767.647361] CPU: 3 PID: 767 Comm: kworker/u8:7 Tainted: G O 5.15.11 #15 >[ 767.655329] Hardware name: DNI AmazonGo1 A7040 board (DT) >[ 767.660760] Workqueue: lacp_1 bond_3ad_state_machine_handler >[ 767.666468] Call trace: >[ 767.668930] dump_backtrace+0x0/0x2d0 >[ 767.672625] show_stack+0x24/0x30 >[ 767.675965] dump_stack_lvl+0x68/0x84 >[ 767.679659] print_address_description.constprop.0+0x74/0x2b8 >[ 767.685451] kasan_report+0x1f0/0x260 >[ 767.689148] __asan_load2+0x94/0xd0 >[ 767.692667] bond_3ad_state_machine_handler+0x13dc/0x1470 > >Fixes: 0622cab0341c ("bonding: fix 802.3ad aggregator reselection") >Co-developed-by: Maksym Glubokiy <maksym.glubokiy@plvision.eu> >Signed-off-by: Maksym Glubokiy <maksym.glubokiy@plvision.eu> >Signed-off-by: Yevhen Orlov <yevhen.orlov@plvision.eu> >--- > drivers/net/bonding/bond_3ad.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c >index a86b1f71762e..d7fb33c078e8 100644 >--- a/drivers/net/bonding/bond_3ad.c >+++ b/drivers/net/bonding/bond_3ad.c >@@ -2228,7 +2228,8 @@ void bond_3ad_unbind_slave(struct slave *slave) > 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 (temp_aggregator->num_of_ports == 0) >+ ad_clear_agg(temp_aggregator); > if (select_new_active_agg) { > slave_info(bond->dev, slave->dev, "Removing an active aggregator\n"); > /* select new active aggregator */ >-- >2.17.1 >
| |