[PATCH] cfg80211: update information elements in cached BSS struct

[PATCH] cfg80211: update information elements in cached BSS struct

[Sven Neumann]

When a cached BSS struct is updated because a new beacon was received,
the code replaces the cached information elements by the IEs from the
new beacon. However it did not update the pub.information_elements
and pub.len_information_elements fields leaving them either pointing
to the old beacon IEs or in an inconsistent state where the data is
replaced by the new beacon IEs but len_information_elements still has
its value from the first beacon.

Fix this by updating the information elements fields if they are
pointing to beacon IEs.

Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>
---
 net/wireless/scan.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 503ebb8..629bfaa 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -465,6 +465,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 			size_t used = dev->wiphy.bss_priv_size + sizeof(*res);
 			size_t ielen = res->pub.len_beacon_ies;
 
+                        int information_elements_is_beacon_ies =
+                                (found->pub.information_elements == found->pub.beacon_ies);
+
 			if (found->pub.beacon_ies &&
 			    !found->beacon_ies_allocated &&
 			    ksize(found) >= used + ielen) {
@@ -487,6 +490,11 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 					found->pub.len_beacon_ies = ielen;
 				}
 			}
+
+                        if (information_elements_is_beacon_ies) {
+                                found->pub.information_elements = found->pub.beacon_ies;
+                                found->pub.len_information_elements = found->pub.len_beacon_ies;
+                        }
 		}
 
 		kref_put(&res->ref, bss_release);
-- 
1.7.1

Re: [PATCH] cfg80211: update information elements in cached BSS struct

[Johannes Berg]

On Thu, 2010-12-09 at 11:48 +0100, Sven Neumann wrote:
> When a cached BSS struct is updated because a new beacon was received,
> the code replaces the cached information elements by the IEs from the
> new beacon. However it did not update the pub.information_elements
> and pub.len_information_elements fields leaving them either pointing
> to the old beacon IEs or in an inconsistent state where the data is
> replaced by the new beacon IEs but len_information_elements still has
> its value from the first beacon.
> 
> Fix this by updating the information elements fields if they are
> pointing to beacon IEs.

Err, no, this is intentional -- we want data from probe responses to
"win" and be displayed preferentially. If you look at the code that
dumps them out, that's what will happen.

If you need the most up-to-date beacon IEs, use (the equivalent of) "iw
wlan0 scan dump -b".

johannes

Re: [PATCH] cfg80211: update information elements in cached BSS struct

[Sven Neumann]

Hi,

On Thu, 2010-12-09 at 13:47 +0100, Johannes Berg wrote:
> On Thu, 2010-12-09 at 11:48 +0100, Sven Neumann wrote:
> > When a cached BSS struct is updated because a new beacon was received,
> > the code replaces the cached information elements by the IEs from the
> > new beacon. However it did not update the pub.information_elements
> > and pub.len_information_elements fields leaving them either pointing
> > to the old beacon IEs or in an inconsistent state where the data is
> > replaced by the new beacon IEs but len_information_elements still has
> > its value from the first beacon.
> > 
> > Fix this by updating the information elements fields if they are
> > pointing to beacon IEs.
> 
> Err, no, this is intentional -- we want data from probe responses to
> "win" and be displayed preferentially. If you look at the code that
> dumps them out, that's what will happen.

As far as I can see my patch does not break this behavior. The
information elements fields are only updated if they are currently
pointing to beacon IEs. If there's ever a probe response frame, then it
will win and the information elements fields won't be changed for
beacons arriving later.

> If you need the most up-to-date beacon IEs, use (the equivalent of) "iw
> wlan0 scan dump -b".

What my patch is fixing are the following situations (and yes, this
happened in real life, it is reproducable and it is reproducably fixed
by the patch that I've submitted):

 AP sends a beacon without RSN IE. This is the first beacon received
 and the station with IEs is added to the BSS cache.
 pub.information_elements now points to pub.beacon_ies and
 pub.len_information_elements has the value of pub.len_beacon_ies.

 AP sends another beacon, this time it includes RSN IE. As there's
 not enough place for the larger IEs, memory is allocated for it
 and the pub.beacon_ies pointer is changed to point to this newly
 allocated memory. Now pub.information_elements still points to
 the old value.

 User-space asks for scan results and will receive the IEs from the
 first beacon because even though the data from subsequent beacons
 has been added to the cache, pub.information_elements still points
 to the old data.

 As a result user-space looking for a WPA enabled access-point will
 skip the AP and never associate with it. This situation will only
 ever fix itself if you are lucky enough that so few beacons are
 arriving that the cached RSS expires at some point.

And there are even worse scenarios:

 If the second beacon is smaller than the first or only slightly larger
 (so that it fits into the allocated memory which has some padding),
 then no new memory will be allocated for the IEs. Instead the data
 in pub.beacon_ies will be changed and pub.len_beacon_ies will be
 updated. Now pub.information_elements points to the new data
 but pub.len_information_elements still holds the old value. Data
 and length have become inconsistent. As a result invalid IEs are
 returned to user-space when it asks for scan results.

As you can see this is all without any probe response frames. Our driver
(libertas) doesn't do any active scanning and thus there are no probe
response frames received. I still tried to write the patch in a way that
should not break things if probe response frames are received.

Please review this patch again. I am pretty sure that there's something
broken. Perhaps my fix is not totally correct, but it fixes a
reproducable association problem for us.


Regards,
Sven


-- 
Sven Neumann
Head of Software Development

RAUMFELD GmbH | Reichenberger Str. 124 | 10999 Berlin | Germany
Tel: +49.30.340.60.98.0 | Fax: +49.30.340.60.98.99 | s.neumann@raumfeld.com

Re: [PATCH] cfg80211: update information elements in cached BSS struct

[Johannes Berg]

On Thu, 2010-12-09 at 14:17 +0100, Sven Neumann wrote:
[snip

> Please review this patch again. I am pretty sure that there's something
> broken. Perhaps my fix is not totally correct, but it fixes a
> reproducable association problem for us.

You're right, the if catches that.

Can you please make the variable a bool, remove the blank line before
it? And your line length was way above 80. A comment would also be good
about why this is done, maybe a one-liner like the probe resp one, just
saying "Override IEs if they were from a beacon before".

Thanks!

johannes

[PATCH] cfg80211: update information elements in cached BSS struct

[Sven Neumann]

When a cached BSS struct is updated because a new beacon was received,
the code replaces the cached information elements by the IEs from the
new beacon. However it did not update the pub.information_elements
and pub.len_information_elements fields leaving them either pointing
to the old beacon IEs or in an inconsistent state where the data is
replaced by the new beacon IEs but len_information_elements still has
its value from the first beacon.

Fix this by updating the information elements fields if they are
pointing to beacon IEs.

Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>
---
 net/wireless/scan.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 503ebb8..ea427f4 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -464,6 +464,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 		if (res->pub.beacon_ies) {
 			size_t used = dev->wiphy.bss_priv_size + sizeof(*res);
 			size_t ielen = res->pub.len_beacon_ies;
+			bool information_elements_is_beacon_ies =
+				(found->pub.information_elements ==
+				 found->pub.beacon_ies);
 
 			if (found->pub.beacon_ies &&
 			    !found->beacon_ies_allocated &&
@@ -487,6 +490,14 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 					found->pub.len_beacon_ies = ielen;
 				}
 			}
+
+			/* Override IEs if they were from a beacon before */
+			if (information_elements_is_beacon_ies) {
+				found->pub.information_elements =
+					found->pub.beacon_ies;
+				found->pub.len_information_elements =
+					found->pub.len_beacon_ies;
+			}
 		}
 
 		kref_put(&res->ref, bss_release);
-- 
1.7.1

Re: [PATCH] cfg80211: update information elements in cached BSS struct

[Johannes Berg]

On Thu, 2010-12-09 at 15:05 +0100, Sven Neumann wrote:
> When a cached BSS struct is updated because a new beacon was received,
> the code replaces the cached information elements by the IEs from the
> new beacon. However it did not update the pub.information_elements
> and pub.len_information_elements fields leaving them either pointing
> to the old beacon IEs or in an inconsistent state where the data is
> replaced by the new beacon IEs but len_information_elements still has
> its value from the first beacon.
> 
> Fix this by updating the information elements fields if they are
> pointing to beacon IEs.
> 
> Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

> ---
>  net/wireless/scan.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 503ebb8..ea427f4 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -464,6 +464,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
>  		if (res->pub.beacon_ies) {
>  			size_t used = dev->wiphy.bss_priv_size + sizeof(*res);
>  			size_t ielen = res->pub.len_beacon_ies;
> +			bool information_elements_is_beacon_ies =
> +				(found->pub.information_elements ==
> +				 found->pub.beacon_ies);
>  
>  			if (found->pub.beacon_ies &&
>  			    !found->beacon_ies_allocated &&
> @@ -487,6 +490,14 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
>  					found->pub.len_beacon_ies = ielen;
>  				}
>  			}
> +
> +			/* Override IEs if they were from a beacon before */
> +			if (information_elements_is_beacon_ies) {
> +				found->pub.information_elements =
> +					found->pub.beacon_ies;
> +				found->pub.len_information_elements =
> +					found->pub.len_beacon_ies;
> +			}
>  		}
>  
>  		kref_put(&res->ref, bss_release);