diff mbox series

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

Message ID 20230902092007.3038132-1-william.xuanziyang@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] team: fix null-ptr-deref when team device type is changed | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1330 this patch: 1330
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1353 this patch: 1353
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1353 this patch: 1353
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 27 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ziyang Xuan (William) Sept. 2, 2023, 9:20 a.m. UTC 
Get a null-ptr-deref bug as follows with reproducer [1].

BUG: kernel NULL pointer dereference, address: 0000000000000228
...
RIP: 0010:vlan_dev_hard_header+0x35/0x140 [8021q]
...
Call Trace:
 <TASK>
 ? __die+0x24/0x70
 ? page_fault_oops+0x82/0x150
 ? exc_page_fault+0x69/0x150
 ? asm_exc_page_fault+0x26/0x30
 ? vlan_dev_hard_header+0x35/0x140 [8021q]
 ? vlan_dev_hard_header+0x8e/0x140 [8021q]
 neigh_connected_output+0xb2/0x100
 ip6_finish_output2+0x1cb/0x520
 ? nf_hook_slow+0x43/0xc0
 ? ip6_mtu+0x46/0x80
 ip6_finish_output+0x2a/0xb0
 mld_sendpack+0x18f/0x250
 mld_ifc_work+0x39/0x160
 process_one_work+0x1e6/0x3f0
 worker_thread+0x4d/0x2f0
 ? __pfx_worker_thread+0x10/0x10
 kthread+0xe5/0x120
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x34/0x50
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1b/0x30

[1]
$ 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(-)

Comments

Hangbin Liu Sept. 4, 2023, 10:23 a.m. UTC | #1 
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
Jiri Pirko Sept. 4, 2023, 11 a.m. UTC | #2 
Mon, Sep 04, 2023 at 12:23:15PM CEST, liuhangbin@gmail.com wrote:
>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;

Yes, this sounds better.


>
>[1] https://lore.kernel.org/netdev/20230718101741.2751799-3-liuhangbin@gmail.com/
>
>Thanks
>Hangbin
Ziyang Xuan (William) Sept. 5, 2023, 3:36 a.m. UTC | #3 
> 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;
> 
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
> 
> .
>
diff mbox series

Patch

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);