[PATCH] net: dpaa2-eth: fix use-after-free in dpaa2_eth_remove

[PATCH] net: dpaa2-eth: fix use-after-free in dpaa2_eth_remove

[Pavel Skripkin]

Access to netdev after free_netdev() will cause use-after-free bug.
Move debug log before free_netdev() call to avoid it.

Cc: stable@vger.kernel.org # v4.19+
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Note about Fixes: tag. The commit introduced it was before this driver
was moved out from staging. I guess, Fixes tag cannot be used here.
Please, let me know if I am wrong.

Cc: Dan Carpenter <dan.carpenter@oracle.com>

@Dan, is there a smatch checker for straigthforward use after free bugs?
Like acessing pointer after free was called? I think, adding
free_netdev() to check list might be good idea

I've skimmed througth smatch source and didn't find one, so can you,
please, point out to it if it exists.



With regards,
Pavel Skripkin

---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index e0c3c58e2ac7..abd833d94eb3 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -4540,10 +4540,10 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev)
 
 	fsl_mc_portal_free(priv->mc_io);
 
-	free_netdev(net_dev);
-
 	dev_dbg(net_dev->dev.parent, "Removed interface %s\n", net_dev->name);
 
+	free_netdev(net_dev);
+
 	return 0;
 }
 
-- 
2.33.1

Re: [PATCH] net: dpaa2-eth: fix use-after-free in dpaa2_eth_remove

[Dan Carpenter]

On Sat, Nov 13, 2021 at 08:20:13PM +0300, Pavel Skripkin wrote:
> Access to netdev after free_netdev() will cause use-after-free bug.
> Move debug log before free_netdev() call to avoid it.
> 
> Cc: stable@vger.kernel.org # v4.19+
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
> 
> Note about Fixes: tag. The commit introduced it was before this driver
> was moved out from staging. I guess, Fixes tag cannot be used here.
> Please, let me know if I am wrong.
> 

You should still use the Fixes tag.

> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> 
> @Dan, is there a smatch checker for straigthforward use after free bugs?
> Like acessing pointer after free was called? I think, adding
> free_netdev() to check list might be good idea
> 
> I've skimmed througth smatch source and didn't find one, so can you,
> please, point out to it if it exists.

It's check_free_strict.c.

It does cross function analysis but free_netdev() is tricky because it
doesn't free directly, it just drops the reference count.  Also it
delays freeing in the NETREG_UNREGISTERING path so this check might
cause false positives? I'll add free_netdev() to the list of free
functions and test it overnight tonight.

	register_free_hook("free_netdev", &match_free, 0);

regards,
dan carpenter

Re: [PATCH] net: dpaa2-eth: fix use-after-free in dpaa2_eth_remove

[Jakub Kicinski]

On Mon, 15 Nov 2021 11:08:17 +0300 Dan Carpenter wrote:
> > @Dan, is there a smatch checker for straigthforward use after free bugs?
> > Like acessing pointer after free was called? I think, adding
> > free_netdev() to check list might be good idea
> > 
> > I've skimmed througth smatch source and didn't find one, so can you,
> > please, point out to it if it exists.  
> 
> It's check_free_strict.c.
> 
> It does cross function analysis but free_netdev() is tricky because it
> doesn't free directly, it just drops the reference count.  Also it
> delays freeing in the NETREG_UNREGISTERING path so this check might
> cause false positives?

I'd ignore that path, it's just special casing that's supposed to keep
the driver-visible API sane. Nobody should be touching netdev past
free_netdev(). Actually if you can it'd be interesting to add checks
for using whatever netdev_priv(ndev) returned past free_netdev(ndev).

Most UAFs that come to mind from the past were people doing something
like:

	struct my_priv *mine = netdev_priv(ndev);

	netdev_unregister(ndev);
	free_netdev(ndev);

	free(mine->bla); /* UAF, free_netdev() frees the priv */

> I'll add free_netdev() to the list of free
> functions and test it overnight tonight.
> 
> 	register_free_hook("free_netdev", &match_free, 0);

Re: [PATCH] net: dpaa2-eth: fix use-after-free in dpaa2_eth_remove

[Pavel Skripkin]

On 11/16/21 04:27, Jakub Kicinski wrote:
> I'd ignore that path, it's just special casing that's supposed to keep
> the driver-visible API sane. Nobody should be touching netdev past
> free_netdev(). Actually if you can it'd be interesting to add checks
> for using whatever netdev_priv(ndev) returned past free_netdev(ndev).
> 
> Most UAFs that come to mind from the past were people doing something
> like:
> 
> 	struct my_priv *mine = netdev_priv(ndev);
> 
> 	netdev_unregister(ndev);
> 	free_netdev(ndev);
> 
> 	free(mine->bla); /* UAF, free_netdev() frees the priv */
> 
I've implemented this checker couple of months ago. The latest smatch 
(v1.72) should warn about this type of bugs. All reported bugs are fixed 
already :)

My checker warns about using priv pointer after free_netdev() and 
free_candev() calls. There are a few more wrappers like 
free_sja1000dev(), so it worth to add them to check list too. Will add 
them today later


Important thing, that there are complex situations like

	struct priv *priv = get_priv_from_smth(smth);

	free_netdev(priv->netdev);
	clean_up_priv(priv);

and for now I have no idea how to handle it (ex: ems_usb_disconnect).




With regards,
Pavel Skripkin