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

[PATCH net v3] 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>
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
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 v3] team: fix null-ptr-deref when team device type is changed

[Paolo Abeni]

On Tue, 2023-09-05 at 16:10 +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>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>

I'm sorry to note that this submission does not fit our process:

https://elixir.bootlin.com/linux/latest/source/Documentation/process/maintainer-netdev.rst#L353

this specific kind of process violations tend to make reviewers quite
unhappy, please be more careful.

Regards,

Paolo

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

[Ziyang Xuan (William)]

> On Tue, 2023-09-05 at 16:10 +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>
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> 
> I'm sorry to note that this submission does not fit our process:
> 
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/maintainer-netdev.rst#L353
> 
> this specific kind of process violations tend to make reviewers quite
> unhappy, please be more careful.
> 
Sorry for the inconvenience caused to everyone. It's my fault. I will improve.

> Regards,
> 
> Paolo
> 
> .
> 

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

[Hangbin Liu]

On Tue, Sep 05, 2023 at 04:10:56PM +0800, Ziyang Xuan wrote:
> ---
>  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
> 

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

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

[Hangbin Liu]

Hi Ziyang,

You patch status is Changes Requested[1]. I think you need to repost this
patch.

[1] https://patchwork.kernel.org/project/netdevbpf/patch/20230905081056.3365013-1-william.xuanziyang@huawei.com/

Thanks
Hangbin
On Tue, Sep 05, 2023 at 04:10:56PM +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>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
> 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 v3] team: fix null-ptr-deref when team device type is changed

[Ziyang Xuan (William)]

> Hi Ziyang,
> 
> You patch status is Changes Requested[1]. I think you need to repost this
> patch.
> 
> [1] https://patchwork.kernel.org/project/netdevbpf/patch/20230905081056.3365013-1-william.xuanziyang@huawei.com/
> 
Thanks for reminding. Patch v4 is sent.

> Thanks
> Hangbin
> On Tue, Sep 05, 2023 at 04:10:56PM +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>
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>> ---
>> 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
>>
> 
> .
>