From f68cbaed67cb009e2974968c8da32bf01537c428 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Thu, 11 Feb 2021 01:57:03 +0200 Subject: [PATCH] net: ipconfig: avoid use-after-free in ic_close_devs Due to the fact that ic_dev->dev is kept open in ic_close_dev, I had thought that ic_dev will not be freed either. But that is not the case, but instead "everybody dies" when ipconfig cleans up, and just the net_device behind ic_dev->dev remains allocated but not ic_dev itself. This is a problem because in ic_close_devs, for every net device that we're about to close, we compare it against the list of lower interfaces of ic_dev, to figure out whether we should close it or not. But since ic_dev itself is subject to freeing, this means that at some point in the middle of the list of ipconfig interfaces, ic_dev will have been freed, and we would be still attempting to iterate through its list of lower interfaces while checking whether to bring down the remaining ipconfig interfaces. There are multiple ways to avoid the use-after-free: we could delay freeing ic_dev until the very end (outside the while loop). Or an even simpler one: we can observe that we don't need ic_dev when iterating through its lowers, only ic_dev->dev, structure which isn't ever freed. So, by keeping ic_dev->dev in a variable assigned prior to freeing ic_dev, we can avoid all use-after-free issues. Fixes: 46acf7bdbc72 ("Revert "net: ipv4: handle DSA enabled master network devices"") Reported-by: kernel test robot Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/ipv4/ipconfig.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c index f9ab1fb..47db1bf 100644 --- a/net/ipv4/ipconfig.c +++ b/net/ipv4/ipconfig.c @@ -309,6 +309,7 @@ have_carrier: */ static void __init ic_close_devs(void) { + struct net_device *selected_dev = ic_dev->dev; struct ic_device *d, *next; struct net_device *dev; @@ -322,7 +323,7 @@ static void __init ic_close_devs(void) next = d->next; dev = d->dev; - netdev_for_each_lower_dev(ic_dev->dev, lower_dev, iter) { + netdev_for_each_lower_dev(selected_dev, lower_dev, iter) { if (dev == lower_dev) { bring_down = false; break; -- 2.7.4