mac80211 is lenient with respect to reception of corrupted beacons.
Even if the frame is corrupted as a whole, the available IE elements
are still passed back and accepted, sometimes replacing legitimate
data. It is unknown to what extent this "feature" is made use of,
but it is clear that in some cases, this is detrimental. One such
case is reported in http://crosbug.com/26832 where an AP corrupts
its beacons but not its probe responses.
One approach would be to completely reject frames with invaid data
(for example, if the last tag extends beyond the end of the enclosing
PDU). The enclosed approach is much more conservative: we simply
prevent later IEs from overwriting the state from previous ones.
This approach hopes that there might be some salient data in the
IE stream before the corruption, and seeks to at least prevent that
data from being overwritten. This approach will fix the case above.
Short of any statistics gathering in the various forms of AP breakage,
it's not possible to ascertain the side effects of more stringent
discarding of data.
Signed-off-by: Paul Stewart <[email protected]>
---
net/mac80211/util.c | 36 ++++++++++++++++++++++++++++++++----
1 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 264397a..661047c 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -572,6 +572,7 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
size_t left = len;
u8 *pos = start;
bool calc_crc = filter != 0;
+ u64 seen_elems;
memset(elems, 0, sizeof(*elems));
elems->ie_start = start;
@@ -579,6 +580,7 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
while (left >= 2) {
u8 id, elen;
+ bool parse_failed = false;
id = *pos++;
elen = *pos++;
@@ -587,8 +589,21 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
if (elen > left)
break;
- if (calc_crc && id < 64 && (filter & (1ULL << id)))
- crc = crc32_be(crc, pos - 2, elen + 2);
+ if (id < 64) {
+ if (seen_elems & (1ULL << id)) {
+ /*
+ * Advance past duplicate entries with
+ * id < 64. Notably this excludes vendor
+ * specific codes which may appear repeatedly.
+ */
+ left -= elen;
+ pos += elen;
+ continue;
+ }
+
+ if (calc_crc && (filter & (1ULL << id)))
+ crc = crc32_be(crc, pos - 2, elen + 2);
+ }
switch (id) {
case WLAN_EID_SSID:
@@ -615,7 +630,8 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
if (elen >= sizeof(struct ieee80211_tim_ie)) {
elems->tim = (void *)pos;
elems->tim_len = elen;
- }
+ } else
+ parse_failed = true;
break;
case WLAN_EID_IBSS_PARAMS:
elems->ibss_params = pos;
@@ -664,10 +680,14 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
case WLAN_EID_HT_CAPABILITY:
if (elen >= sizeof(struct ieee80211_ht_cap))
elems->ht_cap_elem = (void *)pos;
+ else
+ parse_failed = true;
break;
case WLAN_EID_HT_INFORMATION:
if (elen >= sizeof(struct ieee80211_ht_info))
elems->ht_info_elem = (void *)pos;
+ else
+ parse_failed = true;
break;
case WLAN_EID_MESH_ID:
elems->mesh_id = pos;
@@ -676,6 +696,8 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
case WLAN_EID_MESH_CONFIG:
if (elen >= sizeof(struct ieee80211_meshconf_ie))
elems->mesh_config = (void *)pos;
+ else
+ parse_failed = true;
break;
case WLAN_EID_PEER_MGMT:
elems->peering = pos;
@@ -696,6 +718,8 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
case WLAN_EID_RANN:
if (elen >= sizeof(struct ieee80211_rann_ie))
elems->rann = (void *)pos;
+ else
+ parse_failed = true;
break;
case WLAN_EID_CHANNEL_SWITCH:
elems->ch_switch_elem = pos;
@@ -705,7 +729,8 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
if (!elems->quiet_elem) {
elems->quiet_elem = pos;
elems->quiet_elem_len = elen;
- }
+ } else
+ parse_failed = true;
elems->num_of_quiet_elem++;
break;
case WLAN_EID_COUNTRY:
@@ -724,6 +749,9 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
break;
}
+ if (!parse_failed && id < 64)
+ seen_elems |= (1ULL << id);
+
left -= elen;
pos += elen;
}
--
1.7.7.3
On Fri, 2012-02-24 at 07:25 -0800, Paul Stewart wrote:
> >> > I suppose better to accept such service degradation than not connect,
> >> > but I wonder if we should log a warning when we actually try to use such
> >> > an AP?
> >>
> >> Let me know where you'd like it.
> >
> > How about having a flag in the BSS struct (ieee80211_bss) and printing a
> > message when we use that BSS struct for assoc? E.g. in
> > ieee80211_mgd_assoc()? I'm not sure I fully understand where this
> > actually has any effect.
>
> How do we clear this flag? Let's say we got one corrupted beacon and
> from then on, everything was ducky. We set the flag flag on the
> corrupted beacon. Do we clear it on an unblemished probe response?
> Are these really two flags, since beacons and probe responses
> sometimes have different subsets of info, or does a valid probe
> response clear both flags?
Huh, good questions. Which info do we end up using? Just the one that
was not corrupted? I think we always use the last IEs, no?
Hmm. You said in the bug that supported rates were really only affected,
and this caused an issue with the supported rates that we send, because
we restrict ourselves to the rates the AP advertises (due to other buggy
APs ...)
Maybe then it doesn't matter all that much.
OTOH, why should we ever reset the flag? Eventually the BSS entry will
be deleted anyway? And if we keep reconnecting to it, maybe there's some
other bug?
I'm not really sure -- open to suggestions. I just think that in order
to actually notice such things in the future we should print something
so we don't get confused.
johannes
On Fri, Feb 24, 2012 at 6:29 AM, Johannes Berg
<[email protected]> wrote:
> On Fri, 2012-02-24 at 06:24 -0800, Paul Stewart wrote:
>> On Thu, Feb 23, 2012 at 11:34 PM, Johannes Berg
>> <[email protected]> wrote:
>> > On Thu, 2012-02-23 at 17:59 -0800, Paul Stewart wrote:
>> >> mac80211 is lenient with respect to reception of corrupted beacons.
>> >> Even if the frame is corrupted as a whole, the available IE elements
>> >> are still passed back and accepted, sometimes replacing legitimate
>> >> data. ?It is unknown to what extent this "feature" is made use of,
>> >> but it is clear that in some cases, this is detrimental. ?One such
>> >> case is reported in http://crosbug.com/26832 where an AP corrupts
>> >> its beacons but not its probe responses.
>> >>
>> >> One approach would be to completely reject frames with invaid data
>> >> (for example, if the last tag extends beyond the end of the enclosing
>> >> PDU). ?The enclosed approach is much more conservative: we simply
>> >> prevent later IEs from overwriting the state from previous ones.
>> >> This approach hopes that there might be some salient data in the
>> >> IE stream before the corruption, and seeks to at least prevent that
>> >> data from being overwritten. ?This approach will fix the case above.
>> >>
>> >> Short of any statistics gathering in the various forms of AP breakage,
>> >> it's not possible to ascertain the side effects of more stringent
>> >> discarding of data.
>> >
>> > Hmmm. This seems reasonable, but in the given example (in the bug
>> > report) it will at least lose the WMM info (since it's corrupt) and the
>> > HT info, as well as WPS (which is probably not relevant.)
>>
>> As you can see from the association request, the STA appears to hold
>> an amalgamation of the broken rate information in the beacon and the
>> correct HT/WMM information from the probe response. ?In the
>> association request there's both HT and WMM info, as it was able to
>> get it from the probe response.
>
> Ah, indeed, I hadn't even looked at that.
>
>> > I suppose better to accept such service degradation than not connect,
>> > but I wonder if we should log a warning when we actually try to use such
>> > an AP?
>>
>> Let me know where you'd like it.
>
> How about having a flag in the BSS struct (ieee80211_bss) and printing a
> message when we use that BSS struct for assoc? E.g. in
> ieee80211_mgd_assoc()? I'm not sure I fully understand where this
> actually has any effect.
How do we clear this flag? Let's say we got one corrupted beacon and
from then on, everything was ducky. We set the flag flag on the
corrupted beacon. Do we clear it on an unblemished probe response?
Are these really two flags, since beacons and probe responses
sometimes have different subsets of info, or does a valid probe
response clear both flags?
>
> johannes
>
On Fri, 2012-02-24 at 06:24 -0800, Paul Stewart wrote:
> On Thu, Feb 23, 2012 at 11:34 PM, Johannes Berg
> <[email protected]> wrote:
> > On Thu, 2012-02-23 at 17:59 -0800, Paul Stewart wrote:
> >> mac80211 is lenient with respect to reception of corrupted beacons.
> >> Even if the frame is corrupted as a whole, the available IE elements
> >> are still passed back and accepted, sometimes replacing legitimate
> >> data. It is unknown to what extent this "feature" is made use of,
> >> but it is clear that in some cases, this is detrimental. One such
> >> case is reported in http://crosbug.com/26832 where an AP corrupts
> >> its beacons but not its probe responses.
> >>
> >> One approach would be to completely reject frames with invaid data
> >> (for example, if the last tag extends beyond the end of the enclosing
> >> PDU). The enclosed approach is much more conservative: we simply
> >> prevent later IEs from overwriting the state from previous ones.
> >> This approach hopes that there might be some salient data in the
> >> IE stream before the corruption, and seeks to at least prevent that
> >> data from being overwritten. This approach will fix the case above.
> >>
> >> Short of any statistics gathering in the various forms of AP breakage,
> >> it's not possible to ascertain the side effects of more stringent
> >> discarding of data.
> >
> > Hmmm. This seems reasonable, but in the given example (in the bug
> > report) it will at least lose the WMM info (since it's corrupt) and the
> > HT info, as well as WPS (which is probably not relevant.)
>
> As you can see from the association request, the STA appears to hold
> an amalgamation of the broken rate information in the beacon and the
> correct HT/WMM information from the probe response. In the
> association request there's both HT and WMM info, as it was able to
> get it from the probe response.
Ah, indeed, I hadn't even looked at that.
> > I suppose better to accept such service degradation than not connect,
> > but I wonder if we should log a warning when we actually try to use such
> > an AP?
>
> Let me know where you'd like it.
How about having a flag in the BSS struct (ieee80211_bss) and printing a
message when we use that BSS struct for assoc? E.g. in
ieee80211_mgd_assoc()? I'm not sure I fully understand where this
actually has any effect.
johannes
On Thu, 2012-02-23 at 17:59 -0800, Paul Stewart wrote:
> mac80211 is lenient with respect to reception of corrupted beacons.
> Even if the frame is corrupted as a whole, the available IE elements
> are still passed back and accepted, sometimes replacing legitimate
> data. It is unknown to what extent this "feature" is made use of,
> but it is clear that in some cases, this is detrimental. One such
> case is reported in http://crosbug.com/26832 where an AP corrupts
> its beacons but not its probe responses.
>
> One approach would be to completely reject frames with invaid data
> (for example, if the last tag extends beyond the end of the enclosing
> PDU). The enclosed approach is much more conservative: we simply
> prevent later IEs from overwriting the state from previous ones.
> This approach hopes that there might be some salient data in the
> IE stream before the corruption, and seeks to at least prevent that
> data from being overwritten. This approach will fix the case above.
>
> Short of any statistics gathering in the various forms of AP breakage,
> it's not possible to ascertain the side effects of more stringent
> discarding of data.
Hmmm. This seems reasonable, but in the given example (in the bug
report) it will at least lose the WMM info (since it's corrupt) and the
HT info, as well as WPS (which is probably not relevant.)
I suppose better to accept such service degradation than not connect,
but I wonder if we should log a warning when we actually try to use such
an AP?
johannes
On Fri, 2012-02-24 at 07:43 -0800, Paul Stewart wrote:
> > Huh, good questions. Which info do we end up using? Just the one that
> > was not corrupted? I think we always use the last IEs, no?
> >
> > Hmm. You said in the bug that supported rates were really only affected,
> > and this caused an issue with the supported rates that we send, because
> > we restrict ourselves to the rates the AP advertises (due to other buggy
> > APs ...)
> >
> > Maybe then it doesn't matter all that much.
> >
> > OTOH, why should we ever reset the flag?
>
> If my over-the-air captures are any indication, with some probability
> you will receive a corrupted beacon from time to time -- perhaps not
> due to a broken AP, but just from it being corrupted over the air.
> FCS could catch that most times, but perhaps sometimes not. If you
> carry that flag along with you as long as the BSS exists (as long as
> the AP is in view) despite tens or hundreds of intact receptions
> afterwards, the importance of the error message diminishes.
Yes, good point. Actually we (currently) only care about (extended)
supported rates, so maybe we should reset whenever we take those from an
intact frame?
johannes
On Fri, Feb 24, 2012 at 7:36 AM, Johannes Berg
<[email protected]> wrote:
> On Fri, 2012-02-24 at 07:25 -0800, Paul Stewart wrote:
>
>> >> > I suppose better to accept such service degradation than not connect,
>> >> > but I wonder if we should log a warning when we actually try to use such
>> >> > an AP?
>> >>
>> >> Let me know where you'd like it.
>> >
>> > How about having a flag in the BSS struct (ieee80211_bss) and printing a
>> > message when we use that BSS struct for assoc? E.g. in
>> > ieee80211_mgd_assoc()? I'm not sure I fully understand where this
>> > actually has any effect.
>>
>> How do we clear this flag? ?Let's say we got one corrupted beacon and
>> from then on, everything was ducky. ?We set the flag flag on the
>> corrupted beacon. ?Do we clear it on an unblemished probe response?
>> Are these really two flags, since beacons and probe responses
>> sometimes have different subsets of info, or does a valid probe
>> response clear both flags?
>
> Huh, good questions. Which info do we end up using? Just the one that
> was not corrupted? I think we always use the last IEs, no?
>
> Hmm. You said in the bug that supported rates were really only affected,
> and this caused an issue with the supported rates that we send, because
> we restrict ourselves to the rates the AP advertises (due to other buggy
> APs ...)
>
> Maybe then it doesn't matter all that much.
>
> OTOH, why should we ever reset the flag?
If my over-the-air captures are any indication, with some probability
you will receive a corrupted beacon from time to time -- perhaps not
due to a broken AP, but just from it being corrupted over the air.
FCS could catch that most times, but perhaps sometimes not. If you
carry that flag along with you as long as the BSS exists (as long as
the AP is in view) despite tens or hundreds of intact receptions
afterwards, the importance of the error message diminishes.
> Eventually the BSS entry will
> be deleted anyway? And if we keep reconnecting to it, maybe there's some
> other bug?
>
> I'm not really sure -- open to suggestions. I just think that in order
> to actually notice such things in the future we should print something
> so we don't get confused.
Let me cook something up, see what you think.
>
> johannes
>
On Thu, Feb 23, 2012 at 11:34 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2012-02-23 at 17:59 -0800, Paul Stewart wrote:
>> mac80211 is lenient with respect to reception of corrupted beacons.
>> Even if the frame is corrupted as a whole, the available IE elements
>> are still passed back and accepted, sometimes replacing legitimate
>> data. ?It is unknown to what extent this "feature" is made use of,
>> but it is clear that in some cases, this is detrimental. ?One such
>> case is reported in http://crosbug.com/26832 where an AP corrupts
>> its beacons but not its probe responses.
>>
>> One approach would be to completely reject frames with invaid data
>> (for example, if the last tag extends beyond the end of the enclosing
>> PDU). ?The enclosed approach is much more conservative: we simply
>> prevent later IEs from overwriting the state from previous ones.
>> This approach hopes that there might be some salient data in the
>> IE stream before the corruption, and seeks to at least prevent that
>> data from being overwritten. ?This approach will fix the case above.
>>
>> Short of any statistics gathering in the various forms of AP breakage,
>> it's not possible to ascertain the side effects of more stringent
>> discarding of data.
>
> Hmmm. This seems reasonable, but in the given example (in the bug
> report) it will at least lose the WMM info (since it's corrupt) and the
> HT info, as well as WPS (which is probably not relevant.)
As you can see from the association request, the STA appears to hold
an amalgamation of the broken rate information in the beacon and the
correct HT/WMM information from the probe response. In the
association request there's both HT and WMM info, as it was able to
get it from the probe response.
> I suppose better to accept such service degradation than not connect,
> but I wonder if we should log a warning when we actually try to use such
> an AP?
Let me know where you'd like it.
> johannes
>
>