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] [thread-next>] [day] [month] [year] [list]

Date: Wed, 13 Sep 2023 14:15:51 +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


>> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>> index d3dc22509ea5..12fb5f4cff06 100644
>> --- a/drivers/net/team/team.c
>> +++ b/drivers/net/team/team.c
>> @@ -2127,7 +2127,10 @@ 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;
>> +	if (port_dev->type == ARPHRD_ETHER)
>> +		dev->header_ops	= &eth_header_ops;
>> +	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;
> 
> If I read correctly, for !vlan_hw_offload_capable() lower dev, egress
> packets going trough the team device will not contain the vlan tag. 
> 
> Additionally, why is vlan special? Why others lower devices with custom
> header_ops do not need any care? 

We have also got ipvlan device problem as following:

BUG: KASAN: slab-out-of-bounds in ipvlan_hard_header+0xd1/0xe0
Read of size 8 at addr ffff888018ee1de8 by task syz-executor.1/3469
...
Call Trace:
 <IRQ>
 dump_stack+0xbe/0xfd
 print_address_description.constprop.0+0x19/0x170
 ? ipvlan_hard_header+0xd1/0xe0
 __kasan_report.cold+0x6c/0x84
 ? ipvlan_hard_header+0xd1/0xe0
 kasan_report+0x3a/0x50
 ipvlan_hard_header+0xd1/0xe0
 ? ipvlan_get_iflink+0x40/0x40
 neigh_resolve_output+0x28f/0x410
 ip6_finish_output2+0x762/0xef0
 ? ip6_frag_init+0xf0/0xf0
 ? nf_nat_icmpv6_reply_translation+0x460/0x460
 ? do_add_counters+0x370/0x370
 ? do_add_counters+0x370/0x370
 ? ipv6_confirm+0x1ee/0x360
 ? nf_ct_bridge_unregister+0x50/0x50
 __ip6_finish_output.part.0+0x1a8/0x400
 ip6_finish_output+0x1a9/0x1e0
 ip6_output+0x146/0x2b0
 ? ip6_finish_output+0x1e0/0x1e0
 ? __ip6_finish_output+0xb0/0xb0
 ? __sanitizer_cov_trace_switch+0x50/0x90
 ? nf_hook_slow+0xa3/0x150
 mld_sendpack+0x3d9/0x670
 ? mca_alloc+0x210/0x210
 ? add_grhead+0xf5/0x140
 ? ipv6_icmp_sysctl_init+0xd0/0xd0
 ? add_grec+0x4e1/0xa90
 ? _raw_spin_lock_bh+0x85/0xe0
 ? _raw_read_unlock_irqrestore+0x30/0x30
 mld_send_cr+0x426/0x630
 ? migrate_swap_stop+0x400/0x400
 mld_ifc_timer_expire+0x22/0x240
 ? ipv6_mc_netdev_event+0x80/0x80
 call_timer_fn+0x3d/0x230
 ? ipv6_mc_netdev_event+0x80/0x80
 expire_timers+0x190/0x270
 run_timer_softirq+0x22c/0x560

ipvlan problem is slightly different from vlan.

// add ipvlan to team device
team_port_add
  team_dev_type_check_change
    team_setup_by_port // assign ipvlan_header_ops to team_dev->header_ops	
  netdev_rx_handler_register // fail out without restore team_dev->header_ops

// add other ether type device to team device
team_port_add
  team_dev_type_check_change // return directly because port_dev->type and team_dev->type are same

team_dev->head_ops has be assigned to ipvlan_header_ops. That will trigger excption.

> 
> 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?

> 
> Thanks!
> 
> Paolo
> 
> 
> 
> 
> .
>