netdev - Re: [PATCH net v4] team: fix null-ptr-deref when team device type is changed

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4401cd29-0502-67b2-29b1-2db0a23b2042@huawei.com>
Date: Thu, 14 Sep 2023 16:41:32 +0800
From: "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
To: Paolo Abeni <pabeni@...hat.com>, <jiri@...nulli.us>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<netdev@...r.kernel.org>, <leon@...nel.org>, <ye.xingchen@....com.cn>,
	<liuhangbin@...il.com>
Subject: Re: [PATCH net v4] team: fix null-ptr-deref when team device type is
 changed

> On Wed, 2023-09-13 at 14:15 +0800, Ziyang Xuan (William) wrote:
> 
> To me both cases look the same in the end: the team driver sets and use
> header_ops of a different device that will assume dev_priv() being a
> different struct.
> 
> I'm guessing a generic solution could be implementing 'trampoline'
> header_ops that just call into the lower port corresponding op, and
> assigning such ops to the team device every time the lower has non
> ethernet header_ops.
> 
> team_dev_type_check_change() should then probably check both dev->type
> and dev->header_ops.
> 
>>> Exporting 'eth_header_ops' for team's sake only looks a bit too
>>> much to
>>> me. I think could instead cache the header_ops ptr after the
>>> initial
>>> ether_setup().
>>
>> Is it possible to use ether_setup() like bonding driver andmodify MTU
>> individually later?
> 
> That could be another option to get the eth_header_ops.
> 
> Note that in the end both are quite similar, you will have to cache
> some info (the mtu with the latter); ether_setup() possibly will have
> more side effects, as it touches many fields. I personally would use
> the thing I suggested above.
> 
Hi Pallo,

Is it possible to modify like this?

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index d3dc22509ea5..8e6a87ba85aa 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2127,7 +2127,12 @@ static const struct ethtool_ops team_ethtool_ops = {
 static void team_setup_by_port(struct net_device *dev,
                               struct net_device *port_dev)
 {
-       dev->header_ops = port_dev->header_ops;
+       struct team *team = netdev_priv(dev);
+
+       if (port_dev->type == ARPHRD_ETHER)
+               dev->header_ops = team->header_ops_cache;
+       else
+               dev->header_ops = port_dev->header_ops;
        dev->type = port_dev->type;
        dev->hard_header_len = port_dev->hard_header_len;
        dev->needed_headroom = port_dev->needed_headroom;
@@ -2174,8 +2179,11 @@ static int team_dev_type_check_change(struct net_device *dev,

 static void team_setup(struct net_device *dev)
 {
+       struct team *team = netdev_priv(dev);
+
        ether_setup(dev);
        dev->max_mtu = ETH_MAX_MTU;
+       team->header_ops_cache = dev->header_ops;

        dev->netdev_ops = &team_netdev_ops;
        dev->ethtool_ops = &team_ethtool_ops;
diff --git a/include/linux/if_team.h b/include/linux/if_team.h
index 8de6b6e67829..34bcba5a7067 100644
--- a/include/linux/if_team.h
+++ b/include/linux/if_team.h
@@ -189,6 +189,8 @@ struct team {
        struct net_device *dev; /* associated netdevice */
        struct team_pcpu_stats __percpu *pcpu_stats;

+       const struct header_ops *header_ops_cache;
+
        struct mutex lock; /* used for overall locking, e.g. port lists write */


Thank you.

> Cheers,
> 
> Paolo
> 
> .
>