Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jay Vosburgh <jay.vosburgh@canonical.com>
To:     Yevhen Orlov <yevhen.orlov@plvision.eu>
cc:     netdev@vger.kernel.org,
        Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>,
        Taras Chornyi <taras.chornyi@plvision.eu>,
        Mickey Rachamim <mickeyr@marvell.com>,
        Serhiy Pshyk <serhiy.pshyk@plvision.eu>,
        Maksym Glubokiy <maksym.glubokiy@plvision.eu>,
        Veaceslav Falico <vfalico@gmail.com>,
        Andy Gospodarek <andy@greyhouse.net>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] net: bonding: fix use-after-free after 802.3ad slave unbind
In-reply-to: <20220629012914.361-1-yevhen.orlov@plvision.eu>
References: <20220629012914.361-1-yevhen.orlov@plvision.eu>
Comments: In-reply-to Yevhen Orlov <yevhen.orlov@plvision.eu>
   message dated "Wed, 29 Jun 2022 04:29:14 +0300."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30285.1656529132.1@famine>
Content-Transfer-Encoding: quoted-printable
Date:   Wed, 29 Jun 2022 11:58:52 -0700
Message-ID: <30286.1656529132@famine>
Precedence: bulk

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, wh=
en
>num_of_ports!=3D0. 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=3D=3DNULL. So, here w=
e
>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() =3D=3D 0 then the agg is empty, whic=
h
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] =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>[  767.630776] BUG: KASAN: use-after-free in bond_3ad_state_machine_handl=
er+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_3a=
d.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) =3D=3D 0) {
> 					select_new_active_agg =3D temp_aggregator->is_active;
>-					ad_clear_agg(temp_aggregator);
>+					if (temp_aggregator->num_of_ports =3D=3D 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
>