netdev - Re: [PATCH] intel/i40e: Fix potential memory leak in i40e_init_recovery

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Dec 2022 11:28:44 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Yuan Can <yuancan@...wei.com>
Cc:     jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, jeffrey.t.kirsher@...el.com,
        alice.michael@...el.com, piotr.marczak@...el.com,
        intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [PATCH] intel/i40e: Fix potential memory leak in
 i40e_init_recovery_mode()

Tue, Dec 06, 2022 at 10:26:13AM CET, yuancan@...wei.com wrote:
>If i40e_vsi_mem_alloc() failed in i40e_init_recovery_mode(), the pf will be
>freed with the pf->vsi leaked.
>Fix by free pf->vsi in the error handling path.
>
>Fixes: 4ff0ee1af016 ("i40e: Introduce recovery mode support")
>Signed-off-by: Yuan Can <yuancan@...wei.com>
>---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>index b5dcd15ced36..d23081c224d6 100644
>--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>@@ -15536,6 +15536,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw)
> 	pci_disable_pcie_error_reporting(pf->pdev);
> 	pci_release_mem_regions(pf->pdev);
> 	pci_disable_device(pf->pdev);
>+	kfree(pf->vsi);

This is not the only thing which is wrong on this error path. Just
quickly looking at the code:
- kfree(pf->qp_pile); should be called here as well
- if i40e_setup_misc_vector_for_recovery_mode() fails,
  unregister_netdev() needs to be called.
- if register_netdev() fails, netdev needs to be freed at least.
Basically the whole error path is completely wrong.
I suggest to:

1) rely on error path of i40e_probe() when call of
   i40e_init_recovery_mode() fails and don't do the cleanup of
   previously inited things in i40e_probe() locally.
2) implement a proper local error path in i40e_init_recovery_mode()


> 	kfree(pf);
> 
> 	return err;
>-- 
>2.17.1
>