[PATCH net-next] net: ipconfig: avoid use-after-free in ic_close_devs

[PATCH net-next] net: ipconfig: avoid use-after-free in ic_close_devs

[Vladimir Oltean]

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>
---
 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;
 	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

Re: [PATCH net-next] net: ipconfig: avoid use-after-free in ic_close_devs

[Florian Fainelli]

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

Re: [PATCH net-next] net: ipconfig: avoid use-after-free in ic_close_devs

[patchwork-bot+netdevbpf]

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

Re: [PATCH net-next] net: ipconfig: avoid use-after-free in ic_close_devs

[Heiko Thiery]

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

Re: [PATCH net-next] net: ipconfig: avoid use-after-free in ic_close_devs

[Vladimir Oltean]

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/