Re: [PATCH net 1/2] bonding: reject frames with insufficient headroom in bond_header_create

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Qihang wrote:
> Hi Willem,
> 
> I'll resend this as v2 with a cover letter.
> 
> On scope: I grepped every header_ops->create.  Five devices delegate to a
> lower device (bond, team, macvlan, ipvlan, 6lowpan), but only bond and
> team re-select that lower device under RCU -- the rest bind it at netdev
> creation.  So bond and team are the full scope.
> 
> Re Fixes: I used 950803f because it introduced bond_header_create (where
> this check lives); before it the type-confusion BUG masked this race.
> Happy to point at 1284cd3a2b74 instead if you prefer.
> 
> On the fix location, I'd rather get your read before writing more.  The
> options I see:
> 
>   1. per-wrapper headroom reject in bond/team only -- small, safe,
>      backport-friendly, but not generic (the next stacked device needs
>      its own).
> 
>   2. central check in dev_hard_header on dev->hard_header_len -- one
>      place, but I think it's unsafe for bond: bond_dev->hard_header_len
>      only follows the first slave (bond_setup_by_slave),
>      bond_change_active_slave flips curr_active_slave without touching
>      it, and same-type slaves can have different hard_header_len (two GRE
>      tunnels).  So it can under-reject.  Would need bond to maintain
>      hard_header_len differently first.
> 
>   3. resolve the effective {dev, ops, hard_header_len} as one RCU triple
>      via a generic callback, so the caller never cares which subordinate
>      the master picks.  Truly generic and race-free, but it's a new ndo --
>      net-next material, awkward for stable backport.
> 
>   4. make header_ops->create() take an explicit headroom contract and
>      fail cleanly instead of pushing blind -- cleanest long-term, but
>      touches every create() in the tree.  Too heavy for now.
> 
> My v1 is option 1 (per-wrapper).  Want one of these, a combination,
> or something else entirely?

Thanks for the analysis.

I agree that 3 nd 4 are too invasive for the scope of the bug.

It's a bit odd that bond and team just take the hard_header_len of the
first slave. And that bond_create_header just passes to
curr_active_slave, while various bond modes like LAG will have
multiple concurrently active slaves.

That indicates that the intent is for all slaves to have the same
header length and header_ops->create callback.

But this is not enforced.

Given that this seems a bond specific issue, that was copied in teams,
fair to solve it there. Doubly so as due to RCU we have no easy path
to solve it elsewhere. So +1.
> Thanks,
> Qihang
> 
> 
> On Wed, Aug 26, 2026 at 10:02 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Qihang wrote:
> > > From: Qihang Tang <q.h.hack.winter@gmail.com>
> >
> > Thanks for working on this Qihang.
> >
> > Minor: a patch series should generally have a cover letter
> >
> > > AF_PACKET SOCK_DGRAM sends reserve skb headroom from a snapshot of
> > > bond_dev->hard_header_len.  A concurrent bond type change can switch the
> > > active slave to one with a larger hard_header_len between that snapshot
> > > and bond_header_create(), so the slave's create() pushes or writes past
> > > skb->head.
> >
> > Are we certain that bond and team are the only devices that implement
> > header_ops->create?
> >
> > > The hard_header_len snapshot series that fixed the SOCK_RAW send paths
> > > deferred this SOCK_DGRAM race: dev->header_ops is the stable
> > > bond_header_ops, so snapshotting header_ops in the caller does not help.
> > >
> > > Reject the frame if skb headroom is smaller than the active slave's
> > > hard_header_len, before delegating under the existing rcu_read_lock.
> > >
> > > Fixes: 950803f72547 ("bonding: fix type confusion in bond_setup_by_slave()")
> >
> > What's the rationale behind blaming this SHA1?
> >
> > > Cc: stable@vger.kernel.org
> > > Cc: Willem de Bruijn <willemb@google.com>
> > > Signed-off-by: Qihang Tang <q.h.hack.winter@gmail.com>
> > > ---
> > >  drivers/net/bonding/bond_main.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index 522eab060f9e..9ec663610dfd 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -1524,10 +1524,24 @@ static int bond_header_create(struct sk_buff *skb, struct net_device *bond_dev,
> > >       slave = rcu_dereference(bond->curr_active_slave);
> > >       if (slave) {
> > >               slave_ops = READ_ONCE(slave->dev->header_ops);
> > > -             if (slave_ops && slave_ops->create)
> > > +             if (slave_ops && slave_ops->create) {
> > > +                     unsigned int hlen = READ_ONCE(slave->dev->hard_header_len);
> > > +
> > > +                     /* Headroom was reserved from a snapshot of
> > > +                      * bond_dev->hard_header_len that may predate this
> > > +                      * slave (concurrent bond type change); reject if
> > > +                      * insufficient for the slave's create(), which
> > > +                      * pushes its own hlen.
> > > +                      */
> > > +                     if (skb_headroom(skb) < hlen) {
> > > +                             ret = -EINVAL;
> > > +                             goto unlock;
> > > +                     }
> > >                       ret = slave_ops->create(skb, slave->dev,
> > >                                               type, daddr, saddr, len);
> > > +             }
> >
> > Ideally we could fix this in dev_hard_header or PF_PACKET itself,
> > rather than in each implementation that's in scope.
> >
> > But there seems no way around holding this rcu reference on the slave
> > device while re-checking.
> >
> >
> >
> > >       }
> > > +unlock:
> > >       rcu_read_unlock();
> > >       return ret;
> > >  }
> > > --
> > > 2.50.1 (Apple Git-155)
> > >
> >
> >