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; }
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
linux-stable-mirror@lists.linaro.org
-
Dan Carpenter -
Jakub Kicinski -
Pavel Skripkin