[PATCH iwl-net v1] ixgbe: fix memory leak and use-after-free in ixgbe_recovery_probe()

[PATCH iwl-net v1] ixgbe: fix memory leak and use-after-free in ixgbe_recovery_probe()

[Kohei Enju]

The error path of ixgbe_recovery_probe() has two memory bugs.

For non-E610 adapters, the function jumps to clean_up_probe without
calling devlink_free(), leaking the devlink instance and its embedded
adapter structure.

For E610 adapters, devlink_free() is called at shutdown_aci, but
clean_up_probe then accesses adapter->state, sometimes triggering
use-after-free because adapter is embedded in devlink. This UAF is
similar to the one recently reported in ixgbe_remove(). (Link)

Fix both issues by moving devlink_free() after adapter->state access,
aligning with the cleanup order in ixgbe_probe().

Link: https://lore.kernel.org/intel-wired-lan/20250828020558.1450422-1-den@valinux.co.jp/
Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
Cc: Koichiro Den <den@valinux.co.jp>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ff6e8ebda5ba..08368e2717c2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -11510,10 +11510,10 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 shutdown_aci:
 	mutex_destroy(&adapter->hw.aci.lock);
 	ixgbe_release_hw_control(adapter);
-	devlink_free(adapter->devlink);
 clean_up_probe:
 	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
 	free_netdev(netdev);
+	devlink_free(adapter->devlink);
 	pci_release_mem_regions(pdev);
 	if (disable_dev)
 		pci_disable_device(pdev);
-- 
2.51.0

RE: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix memory leak and use-after-free in ixgbe_recovery_probe()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kohei Enju
> Sent: Sunday, August 31, 2025 10:33 PM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Wegrzyn, Stefan <stefan.wegrzyn@intel.com>;
> Mateusz Polchlopek <mateusz.polchlopek@intel.com>; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; kohei.enju@gmail.com; Kohei Enju
> <enjuk@amazon.com>; Koichiro Den <den@valinux.co.jp>
> Subject: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix memory leak
> and use-after-free in ixgbe_recovery_probe()
> 
> The error path of ixgbe_recovery_probe() has two memory bugs.
> 
> For non-E610 adapters, the function jumps to clean_up_probe without
> calling devlink_free(), leaking the devlink instance and its embedded
> adapter structure.
> 
> For E610 adapters, devlink_free() is called at shutdown_aci, but
> clean_up_probe then accesses adapter->state, sometimes triggering
> use-after-free because adapter is embedded in devlink. This UAF is
> similar to the one recently reported in ixgbe_remove(). (Link)
> 
> Fix both issues by moving devlink_free() after adapter->state access,
> aligning with the cleanup order in ixgbe_probe().
> 
> Link: https://lore.kernel.org/intel-wired-lan/20250828020558.1450422-
> 1-den@valinux.co.jp/
> Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery
> mode")
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
> Cc: Koichiro Den <den@valinux.co.jp>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ff6e8ebda5ba..08368e2717c2 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -11510,10 +11510,10 @@ static int ixgbe_recovery_probe(struct
> ixgbe_adapter *adapter)
>  shutdown_aci:
>  	mutex_destroy(&adapter->hw.aci.lock);
>  	ixgbe_release_hw_control(adapter);
> -	devlink_free(adapter->devlink);
>  clean_up_probe:
>  	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter-
> >state);
>  	free_netdev(netdev);
I'd add a guard here: if (adapter->devlink)
What do you think?
> +	devlink_free(adapter->devlink);
>  	pci_release_mem_regions(pdev);
>  	if (disable_dev)
>  		pci_disable_device(pdev);
> --
> 2.51.0

Re: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix memory leak and use-after-free in ixgbe_recovery_probe()

[Kohei Enju]

On Mon, 1 Sep 2025 07:11:26 +0000, Loktionov, Aleksandr wrote:

>> [...]
>> 
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index ff6e8ebda5ba..08368e2717c2 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -11510,10 +11510,10 @@ static int ixgbe_recovery_probe(struct
>> ixgbe_adapter *adapter)
>>  shutdown_aci:
>>  	mutex_destroy(&adapter->hw.aci.lock);
>>  	ixgbe_release_hw_control(adapter);
>> -	devlink_free(adapter->devlink);
>>  clean_up_probe:
>>  	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter-
>> >state);
>>  	free_netdev(netdev);
>I'd add a guard here: if (adapter->devlink)
>What do you think?

Thank you for the review.

Currently ixgbe_recovery_probe() is only called from one location,
ixgbe_probe(), and also always adapter->devlink is not NULL in this path
since this is called after ixgbe_allocate_devlink(). In other words, if
ixgbe_allocate_devlink() fails, ixgbe_recovery_probe() never be called.

So I've thought that the guard might not be necessary like error
handling in ixgbe_probe(), but could you let me know your concern if I'm
overlooking something?

Thanks,
Kohei.

>> +	devlink_free(adapter->devlink);
>>  	pci_release_mem_regions(pdev);
>>  	if (disable_dev)
>>  		pci_disable_device(pdev);
>> --
>> 2.51.0

RE: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix memory leak and use-after-free in ixgbe_recovery_probe()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Kohei Enju <enjuk@amazon.com>
> Sent: Monday, September 1, 2025 10:38 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: andrew+netdev@lunn.ch; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; davem@davemloft.net; den@valinux.co.jp;
> edumazet@google.com; enjuk@amazon.com; intel-wired-
> lan@lists.osuosl.org; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; kohei.enju@gmail.com; kuba@kernel.org;
> mateusz.polchlopek@intel.com; netdev@vger.kernel.org;
> pabeni@redhat.com; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>;
> Wegrzyn, Stefan <stefan.wegrzyn@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix memory
> leak and use-after-free in ixgbe_recovery_probe()
> 
> On Mon, 1 Sep 2025 07:11:26 +0000, Loktionov, Aleksandr wrote:
> 
> 
> 
> >> [...]
> 
> >>
> 
> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> 
> >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> 
> >> index ff6e8ebda5ba..08368e2717c2 100644
> 
> >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> 
> >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> 
> >> @@ -11510,10 +11510,10 @@ static int ixgbe_recovery_probe(struct
> 
> >> ixgbe_adapter *adapter)
> 
> >>  shutdown_aci:
> 
> >>  	mutex_destroy(&adapter->hw.aci.lock);
> 
> >>  	ixgbe_release_hw_control(adapter);
> 
> >> -	devlink_free(adapter->devlink);
> 
> >>  clean_up_probe:
> 
> >>  	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter-
> 
> >> >state);
> 
> >>  	free_netdev(netdev);
> 
> >I'd add a guard here: if (adapter->devlink)
> 
> >What do you think?
> 
> 
> 
> Thank you for the review.
> 
> 
> 
> Currently ixgbe_recovery_probe() is only called from one location,
> 
> ixgbe_probe(), and also always adapter->devlink is not NULL in this
> path
> 
> since this is called after ixgbe_allocate_devlink(). In other words,
> if
> 
> ixgbe_allocate_devlink() fails, ixgbe_recovery_probe() never be
> called.
> 
> 
> 
> So I've thought that the guard might not be necessary like error
> 
> handling in ixgbe_probe(), but could you let me know your concern if
> I'm
> 
> overlooking something?
Good day, Kohei

Thank you for the explanation.
You are technically right (today), and not having the guard doesn't make the patch incorrect!
That said, error-paths tend to evolve. A tiny if (adapter->devlink) { devlink_free(...); adapter->devlink = NULL; }
makes this code resilient to future refactors or partial init/unwind changes and prevents potential double-free if another label ever frees it earlier.
I'm fine either way for this fix, but I'd prefer the guard+NULL for robustness and consistency with typical probe unwind patterns.
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

> 
> 
> 
> Thanks,
> 
> Kohei.
> 
> 
> 
> >> +	devlink_free(adapter->devlink);
> 
> >>  	pci_release_mem_regions(pdev);
> 
> >>  	if (disable_dev)
> 
> >>  		pci_disable_device(pdev);
> 
> >> --
> 
> >> 2.51.0

RE: [PATCH iwl-net v1] ixgbe: fix memory leak and use-after-free in ixgbe_recovery_probe()

[Jagielski, Jedrzej]

From: Kohei Enju <enjuk@amazon.com> 
Sent: Sunday, August 31, 2025 10:33 PM

>The error path of ixgbe_recovery_probe() has two memory bugs.
>
>For non-E610 adapters, the function jumps to clean_up_probe without
>calling devlink_free(), leaking the devlink instance and its embedded
>adapter structure.
>
>For E610 adapters, devlink_free() is called at shutdown_aci, but
>clean_up_probe then accesses adapter->state, sometimes triggering
>use-after-free because adapter is embedded in devlink. This UAF is
>similar to the one recently reported in ixgbe_remove(). (Link)
>
>Fix both issues by moving devlink_free() after adapter->state access,
>aligning with the cleanup order in ixgbe_probe().
>
>Link: https://lore.kernel.org/intel-wired-lan/20250828020558.1450422-1-den@valinux.co.jp/
>Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
>Signed-off-by: Kohei Enju <enjuk@amazon.com>
>---
>Cc: Koichiro Den <den@valinux.co.jp>
>---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>index ff6e8ebda5ba..08368e2717c2 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>@@ -11510,10 +11510,10 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
> shutdown_aci:
> 	mutex_destroy(&adapter->hw.aci.lock);
> 	ixgbe_release_hw_control(adapter);
>-	devlink_free(adapter->devlink);
> clean_up_probe:
> 	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
> 	free_netdev(netdev);
>+	devlink_free(adapter->devlink);
> 	pci_release_mem_regions(pdev);
> 	if (disable_dev)
> 		pci_disable_device(pdev);
>-- 
>2.51.0


Much thanks for finding and fixing it!

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>