[PATCH] Fix infinite loop on dev_mc_unsync()

[PATCH] Fix infinite loop on dev_mc_unsync()

[Luis R. Rodriguez]

While reviewing net/core/dev_mcast.c I found what I think is an 
infinite loop on dev_mc_unsync(). This fixes it. We make use of
this guy on mac80211 in ieee80211_stop(). This is untested.

Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org>

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 15241cf..5373c03 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
+		if (!da->da_synced) {
+			da = next;
 			continue;
+		}
 		__dev_addr_delete(&to->mc_list, &to->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
 		da->da_synced = 0;

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Luis R. Rodriguez]

Sorry, forgot to CC David.

On Fri, Nov 09, 2007 at 10:11:35AM -0500, Luis R. Rodriguez wrote:

While reviewing net/core/dev_mcast.c I found what I think is an 
infinite loop on dev_mc_unsync(). This fixes it. We make use of
this guy on mac80211 in ieee80211_stop(). This is untested.

Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org>

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 15241cf..5373c03 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
+		if (!da->da_synced) {
+			da = next;
 			continue;
+		}
 		__dev_addr_delete(&to->mc_list, &to->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
 		da->da_synced = 0;

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Joe Perches]

On Fri, 2007-11-09 at 13:51 -0500, Luis R. Rodriguez wrote:
> While reviewing net/core/dev_mcast.c I found what I think is an 
> infinite loop on dev_mc_unsync(). This fixes it. We make use of
> this guy on mac80211 in ieee80211_stop(). This is untested.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org>
> 
> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
> index 15241cf..5373c03 100644
> --- a/net/core/dev_mcast.c
> +++ b/net/core/dev_mcast.c
> @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
>  	da = from->mc_list;
>  	while (da != NULL) {
>  		next = da->next;
> -		if (!da->da_synced)
> +		if (!da->da_synced) {
> +			da = next;
>  			continue;
> +		}
>  		__dev_addr_delete(&to->mc_list, &to->mc_count,
>  				  da->da_addr, da->da_addrlen, 0);
>  		da->da_synced = 0;
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Perhaps this is clearer as:

void dev_mc_unsync(struct net_device *to, struct net_device *from)
{
	struct dev_addr_list *da;

	netif_tx_lock_bh(from);
	netif_tx_lock_bh(to);

	da = from->mc_list;
	while (da) {
		if (da->da_synced) {
			__dev_addr_delete(&to->mc_list, &to->mc_count,
					  da->da_addr, da->da_addrlen, 0);
			__dev_addr_delete(&from->mc_list, &from->mc_count,
					  da->da_addr, da->da_addrlen, 0);
			da->da_synced = 0;
		}
		da = da->next;
	}

	__dev_set_rx_mode(to);

	netif_tx_unlock_bh(to);
	netif_tx_unlock_bh(from);
}
EXPORT_SYMBOL(dev_mc_unsync);

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Luis R. Rodriguez]

On Fri, Nov 09, 2007 at 11:07:16AM -0800, Joe Perches wrote:
> On Fri, 2007-11-09 at 13:51 -0500, Luis R. Rodriguez wrote:
> > While reviewing net/core/dev_mcast.c I found what I think is an 
> > infinite loop on dev_mc_unsync(). This fixes it. We make use of
> > this guy on mac80211 in ieee80211_stop(). This is untested.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org>
> > 
> > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
> > index 15241cf..5373c03 100644
> > --- a/net/core/dev_mcast.c
> > +++ b/net/core/dev_mcast.c
> > @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
> >  	da = from->mc_list;
> >  	while (da != NULL) {
> >  		next = da->next;
> > -		if (!da->da_synced)
> > +		if (!da->da_synced) {
> > +			da = next;
> >  			continue;
> > +		}
> >  		__dev_addr_delete(&to->mc_list, &to->mc_count,
> >  				  da->da_addr, da->da_addrlen, 0);
> >  		da->da_synced = 0;
> > -
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Perhaps this is clearer as:
> 
> void dev_mc_unsync(struct net_device *to, struct net_device *from)
> {
> 	struct dev_addr_list *da;
> 
> 	netif_tx_lock_bh(from);
> 	netif_tx_lock_bh(to);
> 
> 	da = from->mc_list;
> 	while (da) {
> 		if (da->da_synced) {
> 			__dev_addr_delete(&to->mc_list, &to->mc_count,
> 					  da->da_addr, da->da_addrlen, 0);
> 			__dev_addr_delete(&from->mc_list, &from->mc_count,
> 					  da->da_addr, da->da_addrlen, 0);
> 			da->da_synced = 0;
> 		}
> 		da = da->next;
> 	}
> 
> 	__dev_set_rx_mode(to);
> 
> 	netif_tx_unlock_bh(to);
> 	netif_tx_unlock_bh(from);
> }
> EXPORT_SYMBOL(dev_mc_unsync);

Sure, or better with a for loop and do away with next pointer then:

void dev_mc_unsync(struct net_device *to, struct net_device *from)
{
        struct dev_addr_list *da;

        netif_tx_lock_bh(from);
        netif_tx_lock_bh(to);

        for (da = from->mc_list; da; da = da->next) {
                if (!da->da_synced)
                        continue;
                __dev_addr_delete(&to->mc_list, &to->mc_count,
                                  da->da_addr, da->da_addrlen, 0);
                da->da_synced = 0;
                __dev_addr_delete(&from->mc_list, &from->mc_count,
                                  da->da_addr, da->da_addrlen, 0);
        }
        __dev_set_rx_mode(to);

        netif_tx_unlock_bh(to);
        netif_tx_unlock_bh(from);
}
EXPORT_SYMBOL(dev_mc_unsync);

Patch below.

Signed-off-by: Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/EHDe48wsgrGvP@public.gmane.org>

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 15241cf..2aea8e1 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync);
  */
 void dev_mc_unsync(struct net_device *to, struct net_device *from)
 {
-	struct dev_addr_list *da, *next;
+	struct dev_addr_list *da;
 
 	netif_tx_lock_bh(from);
 	netif_tx_lock_bh(to);
 
-	da = from->mc_list;
-	while (da != NULL) {
-		next = da->next;
+	for (da = from->mc_list; da; da = da->next) {
 		if (!da->da_synced)
 			continue;
 		__dev_addr_delete(&to->mc_list, &to->mc_count,
@@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 		da->da_synced = 0;
 		__dev_addr_delete(&from->mc_list, &from->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
-		da = next;
 	}
 	__dev_set_rx_mode(to);
 

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Patrick McHardy]

Luis R. Rodriguez wrote:
> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
> index 15241cf..2aea8e1 100644
> --- a/net/core/dev_mcast.c
> +++ b/net/core/dev_mcast.c
> @@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync);
>   */
>  void dev_mc_unsync(struct net_device *to, struct net_device *from)
>  {
> -	struct dev_addr_list *da, *next;
> +	struct dev_addr_list *da;
>  
>  	netif_tx_lock_bh(from);
>  	netif_tx_lock_bh(to);
>  
> -	da = from->mc_list;
> -	while (da != NULL) {
> -		next = da->next;
> +	for (da = from->mc_list; da; da = da->next) {
>   

This may cause a use-after-free since __dev_addr_delete frees the address
when all references are gone.

>  		if (!da->da_synced)
>  			continue;
>  		__dev_addr_delete(&to->mc_list, &to->mc_count,
> @@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
>  		da->da_synced = 0;
>  		__dev_addr_delete(&from->mc_list, &from->mc_count,
>  				  da->da_addr, da->da_addrlen, 0);
> -		da = next;
>  	}
>   

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Joe Perches]

On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote:
> This may cause a use-after-free since __dev_addr_delete frees the address
> when all references are gone.

How about a comment then?  Perhaps:

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index ae35405..63576aa 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	netif_tx_lock_bh(from);
 	netif_tx_lock_bh(to);
 
+	/*
+	  This while loop can't be written as
+		for (da = from->mc_list; da; da = da->next)
+	  da = from->mc_list and __dev_addr_delete can kfree(from->mc_list)
+	  which could cause a use-after-free of da->next
+	*/
+
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
-			continue;
-		__dev_addr_delete(&to->mc_list, &to->mc_count,
-				  da->da_addr, da->da_addrlen, 0);
-		da->da_synced = 0;
-		__dev_addr_delete(&from->mc_list, &from->mc_count,
-				  da->da_addr, da->da_addrlen, 0);
+		if (da->da_synced) {
+			__dev_addr_delete(&to->mc_list, &to->mc_count,
+					  da->da_addr, da->da_addrlen, 0);
+			da->da_synced = 0;
+			__dev_addr_delete(&from->mc_list, &from->mc_count,
+					  da->da_addr, da->da_addrlen, 0);
+		}
 		da = next;
 	}
 	__dev_set_rx_mode(to);

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Patrick McHardy]

Joe Perches wrote:
> On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote:
>   
>> This may cause a use-after-free since __dev_addr_delete frees the address
>> when all references are gone.
>>     
>
> How about a comment then?  Perhaps:
>
> diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
> index ae35405..63576aa 100644
> --- a/net/core/dev_mcast.c
> +++ b/net/core/dev_mcast.c
> @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
>  	netif_tx_lock_bh(from);
>  	netif_tx_lock_bh(to);
>  
> +	/*
> +	  This while loop can't be written as
> +		for (da = from->mc_list; da; da = da->next)
> +	  da = from->mc_list and __dev_addr_delete can kfree(from->mc_list)
> +	  which could cause a use-after-free of da->next
> +	*/
>   

Seems unnecessary to me, we also don't comment each list_for_each_entry_safe
iteration. I consider the use of a seperate next variable self-explanatory.

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[David Miller]

From: Patrick McHardy <kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org>
Date: Sat, 10 Nov 2007 01:13:42 +0100

> Joe Perches wrote:
> > On Sat, 2007-11-10 at 00:12 +0100, Patrick McHardy wrote:
> >   
> >> This may cause a use-after-free since __dev_addr_delete frees the address
> >> when all references are gone.
> >>     
> >
> > How about a comment then?  Perhaps:
> >
> > diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
> > index ae35405..63576aa 100644
> > --- a/net/core/dev_mcast.c
> > +++ b/net/core/dev_mcast.c
> > @@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
> >  	netif_tx_lock_bh(from);
> >  	netif_tx_lock_bh(to);
> >  
> > +	/*
> > +	  This while loop can't be written as
> > +		for (da = from->mc_list; da; da = da->next)
> > +	  da = from->mc_list and __dev_addr_delete can kfree(from->mc_list)
> > +	  which could cause a use-after-free of da->next
> > +	*/
> >   
> 
> Seems unnecessary to me, we also don't comment each list_for_each_entry_safe
> iteration. I consider the use of a seperate next variable self-explanatory.

Agreed, this comment is pointless.

I'll apply Joe's patch without the comment.