[Patch next] octeontx2-pf: fix potential double free in rvu_rep_create()

[Patch next] octeontx2-pf: fix potential double free in rvu_rep_create()

[cxxz16]

In rvu_rep_create(), the netdev is allocated via alloc_etherdev()
and assigned to rep->netdev. This rep structure is then stored
in the priv->reps array indexed by rep_id.

If either rvu_rep_devlink_port_register() or register_netdev() fails,
the function frees ndev using free_netdev(ndev) before jumping to 
the 'exit:' label. However, in the 'exit:' section, the function 
iterates over priv->reps[] and again frees rep->netdev, which points 
to the same ndev.

This results in a potential double free of the same netdev pointer,
which can cause memory corruption or crashes.

To fix this, avoid calling free_netdev(ndev) before jumping to 'exit:'.
The cleanup logic at 'exit:' should handle the freeing safely.

Signed-off-by: cxxz16 <990492108@qq.com>
---
 drivers/net/ethernet/marvell/octeontx2/nic/rep.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
index 04e08e06f30f..de9a50f2fc39 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
@@ -681,7 +681,6 @@ int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack *extack)
 		eth_hw_addr_random(ndev);
 		err = rvu_rep_devlink_port_register(rep);
 		if (err) {
-			free_netdev(ndev);
 			goto exit;
 		}
 
@@ -691,7 +690,6 @@ int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack *extack)
 			NL_SET_ERR_MSG_MOD(extack,
 					   "PFVF representor registration failed");
 			rvu_rep_devlink_port_unregister(rep);
-			free_netdev(ndev);
 			goto exit;
 		}
 
-- 
2.34.1

Re: [Patch next] octeontx2-pf: fix potential double free in rvu_rep_create()

[Abdun Nihaal]

On Sun, Apr 13, 2025 at 02:16:39PM +0800, cxxz16 wrote:
> If either rvu_rep_devlink_port_register() or register_netdev() fails,
> the function frees ndev using free_netdev(ndev) before jumping to 
> the 'exit:' label. However, in the 'exit:' section, the function 
> iterates over priv->reps[] and again frees rep->netdev, which points 
> to the same ndev.
> 
> This results in a potential double free of the same netdev pointer,
> which can cause memory corruption or crashes.
> 
> To fix this, avoid calling free_netdev(ndev) before jumping to 'exit:'.
> The cleanup logic at 'exit:' should handle the freeing safely.
> 
> Signed-off-by: cxxz16 <990492108@qq.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/rep.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index 04e08e06f30f..de9a50f2fc39 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> @@ -681,7 +681,6 @@ int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack *extack)
>  		eth_hw_addr_random(ndev);
>  		err = rvu_rep_devlink_port_register(rep);
>  		if (err) {
> -			free_netdev(ndev);
>  			goto exit;
>  		}
>  
> @@ -691,7 +690,6 @@ int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack *extack)
>  			NL_SET_ERR_MSG_MOD(extack,
>  					   "PFVF representor registration failed");
>  			rvu_rep_devlink_port_unregister(rep);
> -			free_netdev(ndev);
>  			goto exit;
>  		}

There is no potential double free here. If you notice the loop at the
exit label has a predecrement (--rep_id) so if rep_id is say 7 when the
rvu_rep_devlink_port_unregister or register_netdev fails, then the loop
at the exit label would free from rep_id = 6 to 0. And so the
free_netdev calls on those two lines are required.

exit:
	while (--rep_id >= 0) {
		rep = priv->reps[rep_id];
		unregister_netdev(rep->netdev);
		rvu_rep_devlink_port_unregister(rep);
		free_netdev(rep->netdev);
	}

(De)allocations in loops are quite tricky.

Nacked-by: Abdun Nihaal <abdun.nihaal@gmail.com>

Re: [Patch next] octeontx2-pf: fix potential double free in rvu_rep_create()

[Markus Elfring]

…
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
…
> > @@ -691,7 +690,6 @@ int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack *extack)
> >  			NL_SET_ERR_MSG_MOD(extack,
> >  					   "PFVF representor registration failed");
> >  			rvu_rep_devlink_port_unregister(rep);
> > -			free_netdev(ndev);
> >  			goto exit;
> >  		}
>
> There is no potential double free here. If you notice the loop at the
…
> (De)allocations in loops are quite tricky.
>
> Nacked-by: Abdun Nihaal <abdun.nihaal@gmail.com>

Would you ever become interested to avoid a duplicate free_netdev(ndev) call
by using an additional label instead?

See also:
[PATCH net v2 1/2] octeontx2-pf: fix netdev memory leak in rvu_rep_create()
https://lore.kernel.org/netdev/8d54b21b-7ca9-4126-ba13-bbd333d6ba0c@web.de/

Regards,
Markus

Re: [Patch next] octeontx2-pf: fix potential double free in rvu_rep_create()

[Abdun Nihaal]

On Tue, Apr 15, 2025 at 09:32:07AM +0200, Markus Elfring wrote:
> Would you ever become interested to avoid a duplicate free_netdev(ndev) call
> by using an additional label instead?
> 
> See also:
> [PATCH net v2 1/2] octeontx2-pf: fix netdev memory leak in rvu_rep_create()
> https://lore.kernel.org/netdev/8d54b21b-7ca9-4126-ba13-bbd333d6ba0c@web.de/

As Dan also pointed out (https://lore.kernel.org/netdev/116fc5cb-cc46-4e0f-9990-499ae7ef90ee@stanley.mountain/),
the best practice is to undo the current iteration inside the loop
and then cleanup the previous iterations after the goto label.

I think the idea is that it makes it easier to review. We look at the
loop and can tell that when it jumps to the error label, the current
iteration is cleaned up. And when we look at the error label, we can see
that it cleans up all previous iterations.

Whereas, if we had additional goto labels, we would have to go back and
forth between the loop and the goto labels, to tell if cleanup is
performed correctly.

Regards,
Nihaal

Re: [Patch] octeontx2-pf: fix potential double free in rvu_rep_create()

[Markus Elfring]

…
> Signed-off-by: cxxz16 <990492108@qq.com>

I guess that an other personal name would be more desirable according to
the Developer's Certificate of Origin.
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc2#n436

Regards,
Markus