netdev - RE: [EXTERNAL] Re: [PATCH net v3 1/7] octeon_ep: Add checks to fix double free crashes.

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PH0PR18MB473440FBC843E4A88229A38CC7592@PH0PR18MB4734.namprd18.prod.outlook.com>
Date: Tue, 12 Nov 2024 13:03:48 +0000
From: Shinas Rasheed <srasheed@...vell.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: Haseeb Gani <hgani@...vell.com>, Sathesh B Edara <sedara@...vell.com>,
        Vimlesh Kumar <vimleshk@...vell.com>,
        "thaller@...hat.com"
	<thaller@...hat.com>,
        "wizhao@...hat.com" <wizhao@...hat.com>,
        "kheib@...hat.com" <kheib@...hat.com>,
        "egallen@...hat.com"
	<egallen@...hat.com>,
        "konguyen@...hat.com" <konguyen@...hat.com>,
        "horms@...nel.org" <horms@...nel.org>,
        "frank.feng@...axg.com"
	<frank.feng@...axg.com>,
        Veerasenareddy Burru <vburru@...vell.com>,
        Andrew
 Lunn <andrew+netdev@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni
	<pabeni@...hat.com>,
        Abhijit Ayarekar <aayarekar@...vell.com>,
        Satananda
 Burla <sburla@...vell.com>
Subject: RE: [EXTERNAL] Re: [PATCH net v3 1/7] octeon_ep: Add checks to fix
 double free crashes.

Hi Vadim,

Replying to the V2 patch comments:

On 07/11/2024 13:28, Shinas Rasheed wrote:
>> From: Vimlesh Kumar <vimleshk@...vell.com>
>> 
>> Add required checks to avoid double free. Crashes were
>> observed due to the same on reset scenarios
>> 
>> Signed-off-by: Vimlesh Kumar <vimleshk@...vell.com>
>> Signed-off-by: Shinas Rasheed <srasheed@...vell.com>
>> ---
>> V2:
>>    - No changes
>> 
>> V1: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20241101103416.1064930-2D2-2Dsrasheed-40marvell.com_&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4&m=qcKdIM1zsbBD4TFAsa19yJzKNkTlFLrDfM4ffSlTitSWElqZqgdFqodALcL83iWB&s=M_kpJB7LL7JqIBk-MBXonCyf_BWg3fcwHuMc8FQpun0&e=
>> 
>>   .../ethernet/marvell/octeon_ep/octep_main.c   | 39 +++++++++++--------
>>   .../net/ethernet/marvell/octeon_ep/octep_tx.c |  2 +
>>   2 files changed, 25 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
>> index 549436efc204..ff72b796bd25 100644
>> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
>> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
>>@@ -154,9 +154,11 @@ static int octep_enable_msix_range(struct octep_device *oct)
>>  */
>>   static void octep_disable_msix(struct octep_device *oct)
>>   {
>> -	pci_disable_msix(oct->pdev);
>> -	kfree(oct->msix_entries);
>> -	oct->msix_entries = NULL;
>> +	if (oct->msix_entries) {
>> +		pci_disable_msix(oct->pdev);
>> +		kfree(oct->msix_entries);
>> +		oct->msix_entries = NULL;
>> +	}
>>   	dev_info(&oct->pdev->dev, "Disabled MSI-X\n");
>
>How can this function crash? pci_disable_msix() will have checks for
>already disabled msix, kfree can properly deal with NULL pointer.
>Do you have stack trace of the crash here?
>

I think you're right. This won't perhaps be the actual part of the code "fixing"
the crash, and might just as a protection. I shall remove this.

>>  }
>>   
>> @@ -496,16 +498,18 @@ static void octep_free_irqs(struct octep_device *oct)
>>  {
>>   	int i;
>>   
>> -	/* First few MSI-X interrupts are non queue interrupts; free them */
>> -	for (i = 0; i < CFG_GET_NON_IOQ_MSIX(oct->conf); i++)
>> -		free_irq(oct->msix_entries[i].vector, oct);
>> -	kfree(oct->non_ioq_irq_names);
>> -
>> -	/* Free IRQs for Input/Output (Tx/Rx) queues */
>> -	for (i = CFG_GET_NON_IOQ_MSIX(oct->conf); i < oct->num_irqs; i++) {
>> -		irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
>> -		free_irq(oct->msix_entries[i].vector,
>> -			 oct->ioq_vector[i - CFG_GET_NON_IOQ_MSIX(oct->conf)]);
>> +	if (oct->msix_entries) {
>> +		/* First few MSI-X interrupts are non queue interrupts; free them */
>> +		for (i = 0; i < CFG_GET_NON_IOQ_MSIX(oct->conf); i++)
>> +			free_irq(oct->msix_entries[i].vector, oct);
>> +		kfree(oct->non_ioq_irq_names);
>> +
>> +		/* Free IRQs for Input/Output (Tx/Rx) queues */
>> +		for (i = CFG_GET_NON_IOQ_MSIX(oct->conf); i < oct->num_irqs; i++) {
>> +			irq_set_affinity_hint(oct->msix_entries[i].vector, NULL);
>> +			free_irq(oct->msix_entries[i].vector,
>> +				 oct->ioq_vector[i - CFG_GET_NON_IOQ_MSIX(oct->conf)]);
>> +		}
>>   	}
>>   	netdev_info(oct->netdev, "IRQs freed\n");
>>   }
>
>Have you considered fast return option? like
>
>if (!octep_disable_msix)
>	return;
>
>It will make less intendation and less changes in LoC but will presume
>the same behavior.
>

Do you mean this:
if (!oct->msix_entries)
	return;
	
I'll make that change as well.

Thanks!
Shinas