| Message ID | 20211113172013.19959-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dpaa2-eth: fix use-after-free in dpaa2_eth_remove | expand |
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
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);
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
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; }
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(-)