netdev - Re: [PATCH net] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <dc1f51d9-ac48-6155-46d4-760781ea202c@huawei.com>
Date: Tue, 5 Sep 2023 11:36:36 +0800
From: "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
To: Hangbin Liu <liuhangbin@...il.com>
CC: <jiri@...nulli.us>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net] team: fix null-ptr-deref when team device type is
 changed

> 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@...wei.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;
> 
That looks good to me too. I will send patch v2 later.

Thank you!
William Xuan

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