When ieee80211_monitor_select_queue encounters data frames, it selects
the WMM AC based on skb->priority and assumes that skb->priority
contains a valid 802.1d tag. However this assumption is incorrect, since
ieee80211_select_queue has not been called at this point.
If skb->priority > 7, an array overrun occurs, which could lead to
invalid values, resulting in crashes in the tx path.
Fix this by setting skb->priority based on the 802.11 header for QoS
frames and using the default AC for all non-QoS frames.
Signed-off-by: Felix Fietkau <[email protected]>
---
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -685,6 +685,7 @@ static u16 ieee80211_monitor_select_queu
struct ieee80211_local *local = sdata->local;
struct ieee80211_hdr *hdr;
struct ieee80211_radiotap_header *rtap = (void *)skb->data;
+ u8 *p;
if (local->hw.queues < 4)
return 0;
@@ -695,11 +696,14 @@ static u16 ieee80211_monitor_select_queu
hdr = (void *)((u8 *)skb->data + le16_to_cpu(rtap->it_len));
- if (!ieee80211_is_data(hdr->frame_control)) {
+ if (!ieee80211_is_data_qos(hdr->frame_control)) {
skb->priority = 7;
return ieee802_1d_to_ac[skb->priority];
}
+ p = ieee80211_get_qos_ctl(hdr);
+ skb->priority = *p & IEEE80211_QOS_CTL_TAG1D_MASK;
+
return ieee80211_downgrade_queue(local, skb);
}
On Sun, Jan 10, 2010 at 9:47 PM, Felix Fietkau <[email protected]> wrote:
> When ieee80211_monitor_select_queue encounters data frames, it selects
> the WMM AC based on skb->priority and assumes that skb->priority
> contains a valid 802.1d tag. However this assumption is incorrect, since
> ieee80211_select_queue has not been called at this point.
> If skb->priority > 7, an array overrun occurs, which could lead to
> invalid values, resulting in crashes in the tx path.
> Fix this by setting skb->priority based on the 802.11 header for QoS
> frames and using the default AC for all non-QoS frames.
>
> Signed-off-by: Felix Fietkau <[email protected]>
Its unclear whether or not this is a stable fix. It fixes a crash but
does this depend on a patch added recently which is not in stable yet?
Luis
On 2010-01-12 10:37 AM, Lennert Buytenhek wrote:
> On Mon, Jan 11, 2010 at 06:47:00AM +0100, Felix Fietkau wrote:
>
>> When ieee80211_monitor_select_queue encounters data frames, it selects
>> the WMM AC based on skb->priority and assumes that skb->priority
>> contains a valid 802.1d tag. However this assumption is incorrect, since
>> ieee80211_select_queue has not been called at this point.
>> If skb->priority > 7, an array overrun occurs, which could lead to
>> invalid values, resulting in crashes in the tx path.
>
> What you describe here was already reported and fixed:
>
> http://marc.info/?l=linux-wireless&m=126287290723244&w=2
> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-2.6.git;a=commit;h=045cfb71a3901005bf6dcedae98cecb3360a0bfc
>
> Your commit message could at least acknowledge this. I.e. write
> that the existing fix doesn't handle QoS data frames in the optimal
> way, and then mention this:
Sorry, when I wrote and posted the patch, I hadn't seen your previous
fix yet, because I was apparently looking at the wrong tree and had not
noticed your submission yet.
It only cleanly applied to a tree without your change, but it seems that
John fixed it up and replaced your fix with it anyway.
- Felix
On Tue, Jan 12, 2010 at 3:51 AM, Felix Fietkau <[email protected]> wrote:
> On 2010-01-12 10:37 AM, Lennert Buytenhek wrote:
>> On Mon, Jan 11, 2010 at 06:47:00AM +0100, Felix Fietkau wrote:
>>
>>> When ieee80211_monitor_select_queue encounters data frames, it selects
>>> the WMM AC based on skb->priority and assumes that skb->priority
>>> contains a valid 802.1d tag. However this assumption is incorrect, since
>>> ieee80211_select_queue has not been called at this point.
>>> If skb->priority > 7, an array overrun occurs, which could lead to
>>> invalid values, resulting in crashes in the tx path.
>>
>> What you describe here was already reported and fixed:
>>
>> http://marc.info/?l=linux-wireless&m=126287290723244&w=2
>> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-2.6.git;a=commit;h=045cfb71a3901005bf6dcedae98cecb3360a0bfc
>>
>> Your commit message could at least acknowledge this. I.e. write
>> that the existing fix doesn't handle QoS data frames in the optimal
>> way, and then mention this:
> Sorry, when I wrote and posted the patch, I hadn't seen your previous
> fix yet, because I was apparently looking at the wrong tree and had not
> noticed your submission yet.
> It only cleanly applied to a tree without your change, but it seems that
> John fixed it up and replaced your fix with it anyway.
John, did the new changes by Felix get marked for stable too? From
what I gather they apply on top of each other and are stable material.
Luis
On Mon, Jan 11, 2010 at 06:47:00AM +0100, Felix Fietkau wrote:
> When ieee80211_monitor_select_queue encounters data frames, it selects
> the WMM AC based on skb->priority and assumes that skb->priority
> contains a valid 802.1d tag. However this assumption is incorrect, since
> ieee80211_select_queue has not been called at this point.
> If skb->priority > 7, an array overrun occurs, which could lead to
> invalid values, resulting in crashes in the tx path.
What you describe here was already reported and fixed:
http://marc.info/?l=linux-wireless&m=126287290723244&w=2
http://git.kernel.org/?p=linux/kernel/git/linville/wireless-2.6.git;a=commit;h=045cfb71a3901005bf6dcedae98cecb3360a0bfc
Your commit message could at least acknowledge this. I.e. write
that the existing fix doesn't handle QoS data frames in the optimal
way, and then mention this:
> Fix this by setting skb->priority based on the 802.11 header for QoS
> frames and using the default AC for all non-QoS frames.
On 2010-01-11 5:03 PM, Luis R. Rodriguez wrote:
> On Sun, Jan 10, 2010 at 9:47 PM, Felix Fietkau <[email protected]> wrote:
>> When ieee80211_monitor_select_queue encounters data frames, it selects
>> the WMM AC based on skb->priority and assumes that skb->priority
>> contains a valid 802.1d tag. However this assumption is incorrect, since
>> ieee80211_select_queue has not been called at this point.
>> If skb->priority > 7, an array overrun occurs, which could lead to
>> invalid values, resulting in crashes in the tx path.
>> Fix this by setting skb->priority based on the 802.11 header for QoS
>> frames and using the default AC for all non-QoS frames.
>>
>> Signed-off-by: Felix Fietkau <[email protected]>
>
> Its unclear whether or not this is a stable fix. It fixes a crash but
> does this depend on a patch added recently which is not in stable yet?
It depends on the pile of tx queue fixes, and the crash doesn't exist
without those.
- Felix