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: <2910908aeafc8ff133168045ee19f290a7bb35e0.camel@redhat.com>
Date: Tue, 12 Sep 2023 11:32:14 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Ziyang Xuan <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 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@...il.com>
> Reviewed-by: Hangbin Liu <liuhangbin@...il.com>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@...wei.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