| 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
| ||
|
Message-ID: <CANn89iLVq=3d7Ra7gKmTpLcMzuWv+KamYs=KjUHH2z3cPpDBDA@mail.gmail.com>
Date: Mon, 26 May 2025 01:23:18 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: 戸田晃太 <kota.toda@...-cybersecurity.com>
Cc: 小池悠生 <yuki.koike@...-cybersecurity.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net] bonding: Fix header_ops type confusion
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.
> + } 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