[PATCH] net/wireless/nl80211.c: fix endless Netlink callback loop.

[PATCH] net/wireless/nl80211.c: fix endless Netlink callback loop.

[Julius Volz]

Although I only tested similar code (I don't use any of this wireless
code), the state maintainance between Netlink dump callback invocations
seems wrong here and should lead to an endless loop. There are also other
examples in the same file which might have the same problem. Perhaps someone
can actually test this (or refute my logic).

Take the simple example with only one element in the list (which should fit
into the message):

1. invocation:
  Start:
    idx = 0, start = 0
  Loop:
    condition (++idx < start) => (1 < 0) => false
    => no continue, fill one entry, exit loop, return skb->len > 0

2. invocation:
  Start:
    idx = 0, start = 1
  Loop:
    condition (++idx < start) => (1 < 1) => false
    => no continue, fill the same entry again, exit loop, return skb->len > 0

3. invocation:
  Same as 2. invocation, endless invocation of callback.

Also, iterations where the filling of an element fails should not be counted as
completed, so idx should not be incremented in this case.

Signed-off-by: Julius Volz <juliusv@google.com>

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fb75f26..b7fefff 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -194,22 +194,24 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags,
 static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	int idx = 0;
 	int start = cb->args[0];
 	struct cfg80211_registered_device *dev;
 
 	mutex_lock(&cfg80211_drv_mutex);
 	list_for_each_entry(dev, &cfg80211_drv_list, list) {
-		if (++idx < start)
+		if (++idx <= start)
 			continue;
 		if (nl80211_send_wiphy(skb, NETLINK_CB(cb->skb).pid,
 				       cb->nlh->nlmsg_seq, NLM_F_MULTI,
-				       dev) < 0)
+				       dev) < 0) {
+			idx--;
 			break;
+		}
 	}
 	mutex_unlock(&cfg80211_drv_mutex);
 
 	cb->args[0] = idx;
 
 	return skb->len;
 }
 
-- 
1.5.4.5

Re: [PATCH] net/wireless/nl80211.c: fix endless Netlink callback loop.

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

On Tue, 2008-07-08 at 14:02 +0200, Julius Volz wrote:
> Although I only tested similar code (I don't use any of this wireless
> code), the state maintainance between Netlink dump callback invocations
> seems wrong here and should lead to an endless loop. There are also other
> examples in the same file which might have the same problem. Perhaps someone
> can actually test this (or refute my logic).
> 
> Take the simple example with only one element in the list (which should fit
> into the message):

> Also, iterations where the filling of an element fails should not be counted as
> completed, so idx should not be incremented in this case.

Seems to be the case on both points, however,

> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -194,22 +194,24 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags,
>  static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb)
>  {
>  	int idx = 0;
>  	int start = cb->args[0];
>  	struct cfg80211_registered_device *dev;
>  
>  	mutex_lock(&cfg80211_drv_mutex);
>  	list_for_each_entry(dev, &cfg80211_drv_list, list) {
> -		if (++idx < start)
> +		if (++idx <= start)
>  			continue;
>  		if (nl80211_send_wiphy(skb, NETLINK_CB(cb->skb).pid,
>  				       cb->nlh->nlmsg_seq, NLM_F_MULTI,
> -				       dev) < 0)
> +				       dev) < 0) {
> +			idx--;
>  			break;
> +		}

I see much of the problem stemming from incrementing 'idx' at the
beginning of the loop, can't we just move it to the end?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] net/wireless/nl80211.c: fix endless Netlink callback loop.

[Julius Volz]

On Tue, Jul 8, 2008, Johannes Berg wrote:
>> Also, iterations where the filling of an element fails should not be counted as
>> completed, so idx should not be incremented in this case.
>
> Seems to be the case on both points, however,
>
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -194,22 +194,24 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags,
>>  static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb)
>>  {
>>       int idx = 0;
>>       int start = cb->args[0];
>>       struct cfg80211_registered_device *dev;
>>
>>       mutex_lock(&cfg80211_drv_mutex);
>>       list_for_each_entry(dev, &cfg80211_drv_list, list) {
>> -             if (++idx < start)
>> +             if (++idx <= start)
>>                       continue;
>>               if (nl80211_send_wiphy(skb, NETLINK_CB(cb->skb).pid,
>>                                      cb->nlh->nlmsg_seq, NLM_F_MULTI,
>> -                                    dev) < 0)
>> +                                    dev) < 0) {
>> +                     idx--;
>>                       break;
>> +             }
>
> I see much of the problem stemming from incrementing 'idx' at the
> beginning of the loop, can't we just move it to the end?

idx still needs to be incremented in the 'continue' case, so that
alone wouldn't help. I'm not sure if there is a way to make this look
more intuitive?

Julius

-- 
Google Switzerland GmbH

Re: [PATCH] net/wireless/nl80211.c: fix endless Netlink callback loop.

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]


> >>       int idx = 0;
> >>       int start = cb->args[0];
> >>       struct cfg80211_registered_device *dev;
> >>
> >>       mutex_lock(&cfg80211_drv_mutex);
> >>       list_for_each_entry(dev, &cfg80211_drv_list, list) {
> >> -             if (++idx < start)
> >> +             if (++idx <= start)
> >>                       continue;
> >>               if (nl80211_send_wiphy(skb, NETLINK_CB(cb->skb).pid,
> >>                                      cb->nlh->nlmsg_seq, NLM_F_MULTI,
> >> -                                    dev) < 0)
> >> +                                    dev) < 0) {
> >> +                     idx--;
> >>                       break;
> >> +             }
> >
> > I see much of the problem stemming from incrementing 'idx' at the
> > beginning of the loop, can't we just move it to the end?
> 
> idx still needs to be incremented in the 'continue' case, so that
> alone wouldn't help. I'm not sure if there is a way to make this look
> more intuitive?

Good point. The only ways I can come up with add further to the already
quite deep indentation:

list_for_each_entry(...) {
	if (idx > start)
		if (nl80211_send_wiphy(...) < 0)
			break;
	idx++;
}

or hide the call into an if ():

list_for_each_entry(...) {
	if (idx > start && nl80211_send_wiphy(...))
		break;
	idx++;
}

Not sure. I guess the code isn't touched often so your patch is fine.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] net/wireless/nl80211.c: fix endless Netlink callback loop.

[Julius Volz]

On Tue, Jul 8, 2008, Johannes Berg wrote:
> Not sure. I guess the code isn't touched often so your patch is fine.

Ok, thanks!

The other dump functions in that file look like they might have the
same problem, but they are a bit more involved.

Julius

-- 
Google Switzerland GmbH