netdev - Re: [PATCH net v2 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <c3682951-e172-421f-945e-ffe08d67ab66@linux.dev>
Date: Fri, 8 Nov 2024 15:11:42 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Shinas Rasheed <srasheed@...vell.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: hgani@...vell.com, sedara@...vell.com, vimleshk@...vell.com,
 thaller@...hat.com, wizhao@...hat.com, kheib@...hat.com, egallen@...hat.com,
 konguyen@...hat.com, horms@...nel.org, 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>
Subject: Re: [PATCH net v2 1/7] octeon_ep: Add checks to fix double free
 crashes.

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://lore.kernel.org/all/20241101103416.1064930-2-srasheed@marvell.com/
> 
>   .../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?

>   }
>   
> @@ -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.

> @@ -635,8 +639,10 @@ static void octep_napi_delete(struct octep_device *oct)
>   
>   	for (i = 0; i < oct->num_oqs; i++) {
>   		netdev_dbg(oct->netdev, "Deleting NAPI on Q-%d\n", i);
> -		netif_napi_del(&oct->ioq_vector[i]->napi);
> -		oct->oq[i]->napi = NULL;
> +		if (oct->oq[i]->napi) {
> +			netif_napi_del(&oct->ioq_vector[i]->napi);
> +			oct->oq[i]->napi = NULL;
> +		}
>   	}
>   }
>   

[ .. snip ..]