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

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Ziyang,

On Sat, Sep 02, 2023 at 05:20:07PM +0800, Ziyang Xuan wrote:
> $ teamd -t team0 -d -c '{"runner": {"name": "loadbalance"}}'
> $ ip link add name t-dummy type dummy
> $ ip link add link t-dummy name t-dummy.100 type vlan id 100
> $ ip link add name t-nlmon type nlmon
> $ ip link set t-nlmon master team0
> $ ip link set t-nlmon nomaster
> $ ip link set t-dummy up
> $ ip link set team0 up
> $ ip link set t-dummy.100 down
> $ ip link set t-dummy.100 master team0
> 
> When enslave a vlan device to team device and team device type is changed
> from non-ether to ether, header_ops of team device is changed to
> vlan_header_ops. That is incorrect and will trigger null-ptr-deref
> for vlan->real_dev in vlan_dev_hard_header() because team device is not
> a vlan device.
> 
> Use ether_setup() for team device when its type is changed from non-ether
> to ether to fix the bug.
> 
> Fixes: 1d76efe1577b ("team: add support for non-ethernet devices")
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
>  drivers/net/team/team.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index d3dc22509ea5..560e04860aa7 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -2127,14 +2127,19 @@ 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;
> -	dev->type = port_dev->type;
> -	dev->hard_header_len = port_dev->hard_header_len;
> -	dev->needed_headroom = port_dev->needed_headroom;
> -	dev->addr_len = port_dev->addr_len;
> -	dev->mtu = port_dev->mtu;
> -	memcpy(dev->broadcast, port_dev->broadcast, port_dev->addr_len);
> -	eth_hw_addr_inherit(dev, port_dev);
> +	if (port_dev->type == ARPHRD_ETHER) {
> +		ether_setup(dev);
> +		eth_hw_addr_random(dev);
> +	} 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;
> +		dev->addr_len = port_dev->addr_len;
> +		dev->mtu = port_dev->mtu;
> +		memcpy(dev->broadcast, port_dev->broadcast, port_dev->addr_len);
> +		eth_hw_addr_inherit(dev, port_dev);
> +	}
>  
>  	if (port_dev->flags & IFF_POINTOPOINT) {
>  		dev->flags &= ~(IFF_BROADCAST | IFF_MULTICAST);

Thanks for the report. This fix is similar with what I do in my PATCHv3 [1].
And this will go back to the discussion of MTU update. How about just update
the header_ops for ARPHRD_ETHER? e.g.

	if (port_dev->type == ARPHRD_ETHER)
		dev->header_ops	= &eth_header_ops;
	else
		dev->header_ops	= port_dev->header_ops;

[1] https://lore.kernel.org/netdev/20230718101741.2751799-3-liuhangbin@gmail.com/

Thanks
Hangbin