Re: [PATCH v4 0/2] net: bonding: fix type-confusion in bonding header_ops

[Jay Vosburgh]

Kota Toda <kota.toda@xxxxxxxxxxxxxxxxxxxxx> 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.

	A few days ago, Jiayuan Chen <jiayuan.chen@xxxxxxxxx> posted a
fix for what sounds like the same problem[0].  Their solution appears to
be much less complicated.

	I also wonder how this bug was discovered.  The code in question
hasn't changed in many years, and now there are two independent fixes
within a week.

[0] https://lore.kernel.org/netdev/20260228095854.391093-1-jiayuan.chen@xxxxxxxxx/

>Patch 1 stores the slave's header_ops in struct bonding and sets
>wrapper callbacks in bond_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.
>
>Patch 2 uses READ_ONCE when loading header_ops callbacks
>to avoid races with concurrent updates.

	With the READ_ONCE changes in a separate patch, does that mean
that patch 1 by itself is subject to race conditions that would result
in errors?  If so, that's not acceptable, every patch must stand alone
and not break the kernel.

	-J

>Fixes: 1284cd3a2b74 ("bonding: two small fixes for IPoIB support")
>Signed-off-by: Kota Toda <kota.toda@xxxxxxxxxxxxxxxxxxxxx>
>Co-developed-by: Yuki Koike <yuki.koike@xxxxxxxxxxxxxxxxxxxxx>
>Signed-off-by: Yuki Koike <yuki.koike@xxxxxxxxxxxxxxxxxxxxx>
>
>Kota Toda (2):
>  net: bonding: fix type-confusion in bonding header_ops
>  net: add READ_ONCE for header_ops callbacks
>
> drivers/net/bonding/bond_main.c | 67 ++++++++++++++++++++++++++++++++-
> include/linux/netdevice.h       | 41 ++++++++++++++------
> include/net/bonding.h           |  5 +++
> include/net/cfg802154.h         |  2 +-
> net/core/neighbour.c            |  6 +--
> net/ipv4/arp.c                  |  2 +-
> net/ipv6/ndisc.c                |  2 +-
> 7 files changed, 106 insertions(+), 19 deletions(-)
>
>-- 
>2.53.0
>
>

---
	-Jay Vosburgh, jv@xxxxxxxxxxxxx