Re: [PATCH net] net: bonding: fix use-after-free in bond_xmit_broadcast()

[Xiang Mei <xmei5@asu.edu>]

On Tue, Mar 24, 2026 at 2:08 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 3/19/26 12:23 AM, Xiang Mei wrote:
> > bond_xmit_broadcast() reuses the original skb for the last slave
> > (determined by bond_is_last_slave()) and clones it for others.
> > Concurrent slave enslave/release can mutate the slave list during
> > RCU-protected iteration, changing which slave is "last" mid-loop.
> > This causes the original skb to be either double-consumed or freed.
> >
> > Always clone the skb for every slave and unconditionally free the
> > original after the loop, removing the racy bond_is_last_slave()
> > optimization.
> >
> > The UAF can trigger the following crash:
> >
> > ==================================================================
> > BUG: KASAN: slab-use-after-free in skb_clone
> > Read of size 8 at addr ffff888100ef8d40 by task exploit/147
> >
> > CPU: 1 UID: 0 PID: 147 Comm: exploit Not tainted 7.0.0-rc3+ #4 PREEMPTLAZY
> > Call Trace:
> >  <TASK>
> >  dump_stack_lvl (lib/dump_stack.c:123)
> >  print_report (mm/kasan/report.c:379 mm/kasan/report.c:482)
> >  kasan_report (mm/kasan/report.c:597)
> >  skb_clone (include/linux/skbuff.h:1724 include/linux/skbuff.h:1792 include/linux/skbuff.h:3396 net/core/skbuff.c:2108)
> >  bond_xmit_broadcast (drivers/net/bonding/bond_main.c:5334)
> >  bond_start_xmit (drivers/net/bonding/bond_main.c:5567 drivers/net/bonding/bond_main.c:5593)
> >  dev_hard_start_xmit (include/linux/netdevice.h:5325 include/linux/netdevice.h:5334 net/core/dev.c:3871 net/core/dev.c:3887)
> >  __dev_queue_xmit (include/linux/netdevice.h:3601 net/core/dev.c:4838)
> >  ip6_finish_output2 (include/net/neighbour.h:540 include/net/neighbour.h:554 net/ipv6/ip6_output.c:136)
> >  ip6_finish_output (net/ipv6/ip6_output.c:208 net/ipv6/ip6_output.c:219)
> >  ip6_output (net/ipv6/ip6_output.c:250)
> >  ip6_send_skb (net/ipv6/ip6_output.c:1985)
> >  udp_v6_send_skb (net/ipv6/udp.c:1442)
> >  udpv6_sendmsg (net/ipv6/udp.c:1733)
> >  __sys_sendto (net/socket.c:730 net/socket.c:742 net/socket.c:2206)
> >  __x64_sys_sendto (net/socket.c:2209)
> >  do_syscall_64 (arch/x86/entry/syscall_64.c:63 arch/x86/entry/syscall_64.c:94)
> >  entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
> >  </TASK>
> >
> > Allocated by task 147:
> >
> > Freed by task 147:
> >
> > The buggy address belongs to the object at ffff888100ef8c80
> >  which belongs to the cache skbuff_head_cache of size 224
> > The buggy address is located 192 bytes inside of
> >  freed 224-byte region [ffff888100ef8c80, ffff888100ef8d60)
> >
> > Memory state around the buggy address:
> >  ffff888100ef8c00: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
> >  ffff888100ef8c80: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> >> ffff888100ef8d00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> >                                                     ^
> >  ffff888100ef8d80: fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb fb
> >  ffff888100ef8e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ==================================================================
> >
> > Fixes: 4e5bd03ae346 ("net: bonding: fix bond_xmit_broadcast return value error bug")
> > Reported-by: Weiming Shi <bestswngs@gmail.com>
> > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > ---
> >  drivers/net/bonding/bond_main.c | 19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 33f414d03ab91..8e57bee13e4ad 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -5310,7 +5310,6 @@ static netdev_tx_t bond_xmit_broadcast(struct sk_buff *skb,
> >       struct bonding *bond = netdev_priv(bond_dev);
> >       struct bond_up_slave *slaves;
> >       bool xmit_suc = false;
> > -     bool skb_used = false;
> >       int slaves_count, i;
> >
> >       if (all_slaves)
> > @@ -5326,24 +5325,18 @@ static netdev_tx_t bond_xmit_broadcast(struct sk_buff *skb,
> >               if (!(bond_slave_is_up(slave) && slave->link == BOND_LINK_UP))
> >                       continue;
> >
> > -             if (bond_is_last_slave(bond, slave)) {
> > -                     skb2 = skb;
> > -                     skb_used = true;
> > -             } else {
> > -                     skb2 = skb_clone(skb, GFP_ATOMIC);
> > -                     if (!skb2) {
> > -                             net_err_ratelimited("%s: Error: %s: skb_clone() failed\n",
> > -                                                 bond_dev->name, __func__);
> > -                             continue;
> > -                     }
> > +             skb2 = skb_clone(skb, GFP_ATOMIC);
>
> The additional clone is prone allocation failure (atomic scope) and adds
> additional overhead. Is really needed? Would something alike the
> following work instead of the current bond_is_last_slave() check?
>
>                 if (i + 1 == slaves_count)
>
Thanks so much for the advice. Your patch is a more elegant patch. V2
was sent based on your version.

Thanks,
Xiang
> /P
>