netdev - Re: [PATCH 1/1] be2net: Detach interface for avoiding a system crash

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:   Tue, 2 Apr 2019 16:32:55 +0800
From:   Firo <fyang@...e.com>
To:     Saeed Mahameed <saeedm@...lanox.com>,
        "sathya.perla@...adcom.com" <sathya.perla@...adcom.com>,
        "sriharsha.basavapatna@...adcom.com" 
        <sriharsha.basavapatna@...adcom.com>,
        "somnath.kotur@...adcom.com" <somnath.kotur@...adcom.com>,
        "ajit.khaparde@...adcom.com" <ajit.khaparde@...adcom.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Cc:     "firogm@...il.com" <firogm@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/1] be2net: Detach interface for avoiding a system crash



On 4/2/19 12:25 AM, Saeed Mahameed wrote:
> On Mon, 2019-04-01 at 20:24 +0800, Firo Yang wrote:
>> This crash is triggered by a user-after-free since lake of
>> the synchronization of a race condition between   
>> be_update_queues() modifying multi-purpose channels of
>> network device and be_tx_timeout().
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> Call Trace:
>> be_tx_timeout+0xa5/0x360 [be2net]
>> dev_watchdog+0x1d8/0x210
>> call_timer_fn+0x32/0x140
>>
>> To fix it, detach the interface before modifying
>> multi-purpose channels of network device.
>>
>> Signed-off-by: Firo Yang <fyang@...e.com>
>> ---
>>  drivers/net/ethernet/emulex/benet/be_main.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c
>> b/drivers/net/ethernet/emulex/benet/be_main.c
>> index d5026909dec5..25d0128bf684 100644
>> --- a/drivers/net/ethernet/emulex/benet/be_main.c
>> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
>> @@ -4705,6 +4705,8 @@ int be_update_queues(struct be_adapter
>> *adapter)
>>  	struct net_device *netdev = adapter->netdev;
>>  	int status;
>>    
>> +	netif_device_detach(netdev);
>> +
>   
> This will reduce the probability, but will not do the trick.
> since this will not guarantee that the dev_watchdog is disabled.
Great catch!

>   
> what you need is proper locking mechanism and/or scheduling the
> tx_timeout handling out of atomic context if a mutex will be required.
Thank you; I will modify this patch.

Best,
Firo

>   
> netif_device_detach is a too heavy hammer for such synchronizations
> tasks.
>   
>>  	if (netif_running(netdev))
>>  		be_close(netdev);
>>    
>> @@ -4719,21 +4721,21 @@ int be_update_queues(struct be_adapter
>> *adapter)
>>  	be_clear_queues(adapter);
>>  	status = be_cmd_if_destroy(adapter, adapter->if_handle,  0);
>>  	if (status)
>> -		return status;
>> +		goto out;
>>    
>>  	if (!msix_enabled(adapter)) {
>>  		status = be_msix_enable(adapter);
>>  		if (status)
>> -			return status;
>> +			goto out;
>>  	}
>>    
>>  	status = be_if_create(adapter);
>>  	if (status)
>> -		return status;
>> +		goto out;
>>    
>>  	status = be_setup_queues(adapter);
>>  	if (status)
>> -		return status;
>> +		goto out;
>>    
>>  	be_schedule_worker(adapter);
>>    
>> @@ -4748,6 +4750,8 @@ int be_update_queues(struct be_adapter
>> *adapter)
>>  	if (netif_running(netdev))
>>  		status = be_open(netdev);
>>    
>> +out:
>> +	netif_device_attach(netdev);
>>  	return status;
>>  }
>>    



Download attachment "pEpkey.asc" of type "application/pgp-keys" (1746 bytes)