linux-kernel - Re: [PATCH net] bonding: Fix header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANn89iJN-fcx-szsR3Azp8wQ0zhXp0XiYJofQU1zqqtdj7SWTA@mail.gmail.com>
Date: Wed, 28 May 2025 08:10:08 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: 戸田晃太 <kota.toda@...-cybersecurity.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, pabeni@...hat.com, 
	小池悠生 <yuki.koike@...-cybersecurity.com>
Subject: Re: [PATCH net] bonding: Fix header_ops type confusion

On Wed, May 28, 2025 at 7:36 AM 戸田晃太 <kota.toda@...-cybersecurity.com> wrote:
>
> Thank you for your review.
>
> 2025年5月26日(月) 17:23 Eric Dumazet <edumazet@...gle.com>:
> >
> > On Sun, May 25, 2025 at 10:08 PM 戸田晃太 <kota.toda@...-cybersecurity.com> wrote:
> > >
> > > In bond_setup_by_slave(), the slave’s header_ops are unconditionally
> > > copied into the bonding device. As a result, the bonding device may invoke
> > > the slave-specific header operations on itself, causing
> > > netdev_priv(bond_dev) (a struct bonding) to be incorrectly interpreted
> > > as the slave's private-data type.
> > >
> > > This type-confusion bug can lead to out-of-bounds writes into the skb,
> > > resulting in memory corruption.
> > >
> > > This patch adds two members to struct bonding, bond_header_ops and
> > > header_slave_dev, to avoid type-confusion while keeping track of the
> > > slave's header_ops.
> > >
> > > Fixes: 1284cd3a2b740 (bonding: two small fixes for IPoIB support)
> > > Signed-off-by: Kota Toda <kota.toda@...-cybersecurity.com>
> > > Signed-off-by: Yuki Koike <yuki.koike@...-cybersecurity.com>
> > > Co-Developed-by: Yuki Koike <yuki.koike@...-cybersecurity.com>
> > > Reviewed-by: Paolo Abeni <pabeni@...hat.com>
> > > Reported-by: Kota Toda <kota.toda@...-cybersecurity.com>
> > > ---
> > >  drivers/net/bonding/bond_main.c | 61
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  include/net/bonding.h           |  5 +++++
> > >  2 files changed, 65 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index 8ea183da8d53..690f3e0971d0 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -1619,14 +1619,65 @@ static void bond_compute_features(struct bonding *bond)
> > >      netdev_change_features(bond_dev);
> > >  }
> > >
> > > +static int bond_hard_header(struct sk_buff *skb, struct net_device *dev,
> > > +        unsigned short type, const void *daddr,
> > > +        const void *saddr, unsigned int len)
> > > +{
> > > +    struct bonding *bond = netdev_priv(dev);
> > > +    struct net_device *slave_dev;
> > > +
> > > +    slave_dev = bond->header_slave_dev;
> > > +
> > > +    return dev_hard_header(skb, slave_dev, type, daddr, saddr, len);
> > > +}
> > > +
> > > +static void bond_header_cache_update(struct hh_cache *hh, const
> > > struct net_device *dev,
> > > +        const unsigned char *haddr)
> > > +{
> > > +    const struct bonding *bond = netdev_priv(dev);
> > > +    struct net_device *slave_dev;
> > > +
> > > +    slave_dev = bond->header_slave_dev;
> >
> > I do not see any barrier ?
> >
> > > +
> > > +    if (!slave_dev->header_ops || !slave_dev->header_ops->cache_update)
> > > +        return;
> > > +
> > > +    slave_dev->header_ops->cache_update(hh, slave_dev, haddr);
> > > +}
> > > +
> > >  static void bond_setup_by_slave(struct net_device *bond_dev,
> > >                  struct net_device *slave_dev)
> > >  {
> > > +    struct bonding *bond = netdev_priv(bond_dev);
> > >      bool was_up = !!(bond_dev->flags & IFF_UP);
> > >
> > >      dev_close(bond_dev);
> > >
> > > -    bond_dev->header_ops        = slave_dev->header_ops;
> > > +    /* Some functions are given dev as an argument
> > > +     * while others not. When dev is not given, we cannot
> > > +     * find out what is the slave device through struct bonding
> > > +     * (the private data of bond_dev). Therefore, we need a raw
> > > +     * header_ops variable instead of its pointer to const header_ops
> > > +     * and assign slave's functions directly.
> > > +     * For the other case, we set the wrapper functions that pass
> > > +     * slave_dev to the wrapped functions.
> > > +     */
> > > +    bond->bond_header_ops.create = bond_hard_header;
> > > +    bond->bond_header_ops.cache_update = bond_header_cache_update;
> > > +    if (slave_dev->header_ops) {
> > > +        bond->bond_header_ops.parse = slave_dev->header_ops->parse;
> > > +        bond->bond_header_ops.cache = slave_dev->header_ops->cache;
> > > +        bond->bond_header_ops.validate = slave_dev->header_ops->validate;
> > > +        bond->bond_header_ops.parse_protocol =
> > > slave_dev->header_ops->parse_protocol;
> >
> > All these updates probably need WRITE_ONCE(), and corresponding
> > READ_ONCE() on reader sides, at a very minimum ...
> >
> > RCU would even be better later.
> >
> I believe that locking is not necessary in this patch. The update of
> `header_ops` only happens when a slave is newly enslaved to a bond.
> Under such circumstances, members of `header_ops` are not called in
> parallel with updating. Therefore, there is no possibility of race
> conditions occurring.

bond_dev can certainly be live, and packets can flow.

I have seen enough syzbot reports hinting at this precise issue.