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 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. Fixes: 1284cd3a2b74 ("bonding: two small fixes for IPoIB support") Signed-off-by: Kota Toda Co-developed-by: Yuki Koike Signed-off-by: Yuki Koike --- drivers/net/bonding/bond_main.c | 67 ++++++++++++++++++++++++++++++++- include/net/bonding.h | 5 +++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index f17a170d1..14d3e5298 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1616,14 +1616,71 @@ 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); + void (*cache_update)(struct hh_cache *hh, + const struct net_device *dev, + const unsigned char *haddr); + struct net_device *slave_dev; + + slave_dev = bond->header_slave_dev; + if (!slave_dev->header_ops) + return; + cache_update = READ_ONCE(slave_dev->header_ops->cache_update); + if (!cache_update) + return; + 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) { + WRITE_ONCE(bond->bond_header_ops.parse, slave_dev->header_ops->parse); + WRITE_ONCE(bond->bond_header_ops.cache, slave_dev->header_ops->cache); + WRITE_ONCE(bond->bond_header_ops.validate, slave_dev->header_ops->validate); + WRITE_ONCE(bond->bond_header_ops.parse_protocol, + slave_dev->header_ops->parse_protocol); + } else { + WRITE_ONCE(bond->bond_header_ops.parse, NULL); + WRITE_ONCE(bond->bond_header_ops.cache, NULL); + WRITE_ONCE(bond->bond_header_ops.validate, NULL); + WRITE_ONCE(bond->bond_header_ops.parse_protocol, NULL); + } + + WRITE_ONCE(bond_dev->header_ops, &bond->bond_header_ops); + bond->header_slave_dev = slave_dev; bond_dev->type = slave_dev->type; bond_dev->hard_header_len = slave_dev->hard_header_len; @@ -2682,6 +2739,14 @@ static int bond_release_and_destroy(struct net_device *bond_dev, struct bonding *bond = netdev_priv(bond_dev); int ret; + /* If slave_dev is the earliest registered one, we must clear + * the variables related to header_ops to avoid dangling pointer. + */ + if (bond->header_slave_dev == slave_dev) { + WRITE_ONCE(bond_dev->header_ops, NULL); + bond->header_slave_dev = NULL; + } + ret = __bond_release_one(bond_dev, slave_dev, false, true); if (ret == 0 && !bond_has_slaves(bond) && bond_dev->reg_state != NETREG_UNREGISTERING) { diff --git a/include/net/bonding.h b/include/net/bonding.h index 95f67b308..cf8206187 100644 --- a/include/net/bonding.h +++ b/include/net/bonding.h @@ -215,6 +215,11 @@ struct bond_ipsec { */ struct bonding { struct net_device *dev; /* first - useful for panic debug */ + struct net_device *header_slave_dev; /* slave net_device for header_ops */ + /* maintained as a non-const variable + * because bond's header_ops should change depending on slaves. + */ + struct header_ops bond_header_ops; struct slave __rcu *curr_active_slave; struct slave __rcu *current_arp_slave; struct slave __rcu *primary_slave; -- 2.53.0