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

Kota Toda posted 2 patches 3 weeks, 6 days ago
[PATCH v4 1/2] net: bonding: fix type-confusion in bonding header_ops
Posted by Kota Toda 3 weeks, 6 days ago
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 <kota.toda@gmo-cybersecurity.com>
Co-developed-by: Yuki Koike <yuki.koike@gmo-cybersecurity.com>
Signed-off-by: Yuki Koike <yuki.koike@gmo-cybersecurity.com>
---
 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