| Message ID | 20210210235703.1882205-1-olteanv@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: ipconfig: avoid use-after-free in ic_close_devs | expand |
On 2/10/2021 3:57 PM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > 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 <oliver.sang@intel.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Thu, 11 Feb 2021 01:57:03 +0200 you wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > 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. > > [...] Here is the summary with links: - [net-next] net: ipconfig: avoid use-after-free in ic_close_devs https://git.kernel.org/netdev/net-next/c/f68cbaed67cb You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Hi Vladimir and all, > 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 <oliver.sang@intel.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > 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 f9ab1fb219ec..47db1bfdaaa0 100644 > --- a/net/ipv4/ipconfig.c > +++ b/net/ipv4/ipconfig.c > @@ -309,6 +309,7 @@ static int __init ic_open_devs(void) > */ > static void __init ic_close_devs(void) > { > + struct net_device *selected_dev = ic_dev->dev; This will causes a kernel panic when ic_dev is not valid. See log output here: https://lavalab.kontron.com/scheduler/job/12453 > 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.25.1 I can fix my issue with this: diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c index 47db1bfdaaa0..6f1df4336153 100644 --- a/net/ipv4/ipconfig.c +++ b/net/ipv4/ipconfig.c @@ -309,7 +309,6 @@ static int __init ic_open_devs(void) */ static void __init ic_close_devs(void) { - struct net_device *selected_dev = ic_dev->dev; struct ic_device *d, *next; struct net_device *dev; @@ -323,10 +322,13 @@ static void __init ic_close_devs(void) next = d->next; dev = d->dev; - netdev_for_each_lower_dev(selected_dev, lower_dev, iter) { - if (dev == lower_dev) { - bring_down = false; - break; + if (ic_dev) { + struct net_device *selected_dev = ic_dev->dev; + netdev_for_each_lower_dev(selected_dev, lower_dev, iter) { + if (dev == lower_dev) { + bring_down = false; + break; + } } } if (bring_down) { What do you think? Is this valid? Thank you. -- Heiko
Hi Heiko, On Mon, Mar 22, 2021 at 11:57:33AM +0100, Heiko Thiery wrote: > Hi Vladimir and all, > > > 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 <oliver.sang@intel.com> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > --- > > 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 f9ab1fb219ec..47db1bfdaaa0 100644 > > --- a/net/ipv4/ipconfig.c > > +++ b/net/ipv4/ipconfig.c > > @@ -309,6 +309,7 @@ static int __init ic_open_devs(void) > > */ > > static void __init ic_close_devs(void) > > { > > + struct net_device *selected_dev = ic_dev->dev; > > This will causes a kernel panic when ic_dev is not valid. > > See log output here: https://lavalab.kontron.com/scheduler/job/12453 > > > 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.25.1 > > > I can fix my issue with this: > > diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c > index 47db1bfdaaa0..6f1df4336153 100644 > --- a/net/ipv4/ipconfig.c > +++ b/net/ipv4/ipconfig.c > @@ -309,7 +309,6 @@ static int __init ic_open_devs(void) > */ > static void __init ic_close_devs(void) > { > - struct net_device *selected_dev = ic_dev->dev; > struct ic_device *d, *next; > struct net_device *dev; > > @@ -323,10 +322,13 @@ static void __init ic_close_devs(void) > next = d->next; > dev = d->dev; > > - netdev_for_each_lower_dev(selected_dev, lower_dev, iter) { > - if (dev == lower_dev) { > - bring_down = false; > - break; > + if (ic_dev) { > + struct net_device *selected_dev = ic_dev->dev; > + netdev_for_each_lower_dev(selected_dev, lower_dev, iter) { > + if (dev == lower_dev) { > + bring_down = false; > + break; > + } > } > } > if (bring_down) { > > > What do you think? Is this valid? > > Thank you. > -- > Heiko I guess you're the right person to give me a Reviewed-by/Tested-by here: https://patchwork.kernel.org/project/netdevbpf/patch/20210322002637.3412657-1-olteanv@gmail.com/
diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c index f9ab1fb219ec..47db1bfdaaa0 100644 --- a/net/ipv4/ipconfig.c +++ b/net/ipv4/ipconfig.c @@ -309,6 +309,7 @@ static int __init ic_open_devs(void) */ 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;