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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20210322105733.20080-1-heiko.thiery@gmail.com>
Date:   Mon, 22 Mar 2021 11:57:33 +0100
From:   Heiko Thiery <heiko.thiery@...il.com>
To:     olteanv@...il.com
Cc:     andrew@...n.ch, davem@...emloft.net, f.fainelli@...il.com,
        kuba@...nel.org, netdev@...r.kernel.org, vivien.didelot@...il.com,
        yoshfuji@...ux-ipv6.org, heiko.thiery@...il.com, michael@...le.cc
Subject: Re: [PATCH net-next] net: ipconfig: avoid use-after-free in ic_close_devs

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@...el.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....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