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

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

[Ziyang Xuan]

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.

Assign eth_header_ops to header_ops of 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")
Suggested-by: Hangbin Liu <liuhangbin@gmail.com>
Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
v4:
  - Add Reviewed-by tag.
v3:
  - Export eth_header_ops to fix modpost error.
v2:
  - Just modify header_ops to eth_header_ops not use ether_setup().
---
 drivers/net/team/team.c | 5 ++++-
 net/ethernet/eth.c      | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

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;
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 2edc8b796a4e..157833509adb 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -347,6 +347,7 @@ const struct header_ops eth_header_ops ____cacheline_aligned = {
 	.cache_update	= eth_header_cache_update,
 	.parse_protocol	= eth_header_parse_protocol,
 };
+EXPORT_SYMBOL(eth_header_ops);
 
 /**
  * ether_setup - setup Ethernet network device
-- 
2.25.1

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

[Paolo Abeni]

On Mon, 2023-09-11 at 17:46 +0800, Ziyang Xuan wrote:
> 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.
> 
> Assign eth_header_ops to header_ops of 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")
> Suggested-by: Hangbin Liu <liuhangbin@gmail.com>
> Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
> v4:
>   - Add Reviewed-by tag.
> v3:
>   - Export eth_header_ops to fix modpost error.
> v2:
>   - Just modify header_ops to eth_header_ops not use ether_setup().
> ---
>  drivers/net/team/team.c | 5 ++++-
>  net/ethernet/eth.c      | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> 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? 

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().

Thanks!

Paolo

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

[Ziyang Xuan (William)]

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

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

[Paolo Abeni]

On Wed, 2023-09-13 at 14:15 +0800, Ziyang Xuan (William) wrote:
> > > 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.

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.

Cheers,

Paolo

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

[Jiri Pirko]

Wed, Sep 13, 2023 at 08:28:13AM CEST, pabeni@redhat.com wrote:
>On Wed, 2023-09-13 at 14:15 +0800, Ziyang Xuan (William) wrote:
>> > > 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.
>
>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.

Agreed. That is why ether_setup() was not used in the first place.


>
>Cheers,
>
>Paolo
>

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

[Ziyang Xuan (William)]

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

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

[Jakub Acs]

On Wed, Sep 13, 2023 at 08:28:13AM +0200, Paolo Abeni 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.
> 

Hi Paolo, we're seeing syzkaller to run into this exact issue (type
confusion) in ip6gre_header, when adding ip6gre device as a port for a
team device.

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

I tried implementing such trampoline for header_ops.create op for team,
and have the following problem: 
At the time header_ops.create is called on a team device, I don't know
which port device will be used to transmit the skb in the end. 

Could you elaborate what would be the solution for that, please?

Kind Regards,
Jakub

P.S.: Sorry for reviving 2y old thread, but I felt it's better than 
opening a new one since the context is here. Correct me if I'm wrong or
if you'd prefer otherwise, please :)



Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597