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]

Message-ID: <06082c443dbaf83495dde16c33884adc30872ec8.camel@redhat.com>
Date: Wed, 13 Sep 2023 08:28:13 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: "Ziyang Xuan (William)" <william.xuanziyang@...wei.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

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