Message-ID: <CAA3_Gnogt7GR0gZVZwQ4vXXav6TpXMK6t=QTLsqKOaX3Bo_tNA@mail.gmail.com>
Date: Mon, 26 May 2025 14:08:01 +0900
From: 戸田晃太 <kota.toda@...-cybersecurity.com>
To: 戸田晃太 <kota.toda@...-cybersecurity.com>
Cc: 小池悠生 <yuki.koike@...-cybersecurity.com>, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, pabeni@...hat.com, 
	edumazet@...gle.com
Subject: [PATCH net] bonding: Fix header_ops type confusion

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;
+
+    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;
+    } else {
+        bond->bond_header_ops.parse = NULL;
+        bond->bond_header_ops.cache = NULL;
+        bond->bond_header_ops.validate = NULL;
+        bond->bond_header_ops.parse_protocol = NULL;
+    }
+
+    bond->header_slave_dev      = slave_dev;
+    bond_dev->header_ops        = &bond->bond_header_ops;

     bond_dev->type            = slave_dev->type;
     bond_dev->hard_header_len   = slave_dev->hard_header_len;
@@ -2676,6 +2727,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) {
+        bond->header_slave_dev = NULL;
+        bond_dev->header_ops = 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 95f67b308c19..cf8206187ce9 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;

Powered by blists - more mailing lists