This patch fixes the following NULL pointer dereference:
> I rerun the test on a kernel with some more lock checking and got lucky.
>
> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
> with 98:fc:11:8e:94:57
> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
> 98:fc:11:8e:94:57 (try 1/3)
> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
> kernel NULL pointer dereference at 00000004
> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
> minstrel_tx_status+0x48/0xe0 [mac80211]
static inline int
rix_to_ndx(struct minstrel_sta_info *mi, int rix)
{
int i = rix;
for (i = rix; i >= 0; i--)
if (mi->r[i].rix == rix)
48: 3b 58 04 cmp 0x4(%eax),%ebx
4b: 74 4b je 98 <minstrel_tx_status+0x98>
4d: 83 c2 01 add $0x1,%edx
It fails because of mi->r[i] being a NULL pointer. It is allocated in
minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
struct minstrel_sta_info. During authenticate the bool is_ht is false so
minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
is not set until rate_init which is after assoc.
Cc: Johannes Berg <[email protected]>
Cc: Felix Fietkau <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
John,
This one with tab indent instead of spaces. Noticed just after sending
the first patch.
Gr. AvS
---
net/mac80211/rc80211_minstrel_ht.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index ff5f7b8..a58c348 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -813,6 +813,9 @@ minstrel_ht_alloc_sta(void *priv, struct ieee80211_sta *sta, gfp_t gfp)
if (!msp->sample_table)
goto error1;
+ msp->legacy.r = msp->ratelist;
+ msp->legacy.sample_table = msp->sample_table;
+
return msp;
error1:
--
1.7.5.4
On 02/08/2012 05:08 PM, Felix Fietkau wrote:
> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>> This patch fixes the following NULL pointer dereference:
>>
>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>
>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>> with 98:fc:11:8e:94:57
>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>> 98:fc:11:8e:94:57 (try 1/3)
>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>> kernel NULL pointer dereference at 00000004
>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>
>> static inline int
>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>> {
>> int i = rix;
>> for (i = rix; i >= 0; i--)
>> if (mi->r[i].rix == rix)
>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
>> 4d: 83 c2 01 add $0x1,%edx
>>
>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>
>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>> is not set until rate_init which is after assoc.
> I'm not sure this is the right fix. If I understand the issue correctly,
> mi->r is only NULL because .rate_init has not been called yet, which is
> a bug that other rate control modules might trip over as well, maybe in
> more subtle ways. If this is the case, then I believe this bug should be
> fixed in mac80211 instead.
>
> Maybe it would even be a good idea to set a flag after calling
> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
> before that.
>
> - Felix
>
Hi Felix,
I am not sure I can agree. minstrel_ht allocates the struct
minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
default and zero'ing the legacy field. When setting is_ht to false
consequently the contents of the legacy field should be properly set as
is done in minstrel_ht_update_caps (at use_legacy label).
Gr. AvS
On 02/08/2012 01:44 PM, Arend van Spriel wrote:
> This patch fixes the following NULL pointer dereference:
>
>> I rerun the test on a kernel with some more lock checking and got lucky.
>>
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>> with 98:fc:11:8e:94:57
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>> 98:fc:11:8e:94:57 (try 1/3)
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>> kernel NULL pointer dereference at 00000004
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>> minstrel_tx_status+0x48/0xe0 [mac80211]
>
> static inline int
> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
> {
> int i = rix;
> for (i = rix; i >= 0; i--)
> if (mi->r[i].rix == rix)
> 48: 3b 58 04 cmp 0x4(%eax),%ebx
> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
> 4d: 83 c2 01 add $0x1,%edx
>
> It fails because of mi->r[i] being a NULL pointer. It is allocated in
> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>
> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
> struct minstrel_sta_info. During authenticate the bool is_ht is false so
> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
> is not set until rate_init which is after assoc.
>
> Cc: Johannes Berg <[email protected]>
> Cc: Felix Fietkau <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
> ---
> John,
>
> This one with tab indent instead of spaces. Noticed just after sending
> the first patch.
>
> Gr. AvS
> ---
> net/mac80211/rc80211_minstrel_ht.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
> index ff5f7b8..a58c348 100644
> --- a/net/mac80211/rc80211_minstrel_ht.c
> +++ b/net/mac80211/rc80211_minstrel_ht.c
> @@ -813,6 +813,9 @@ minstrel_ht_alloc_sta(void *priv, struct ieee80211_sta *sta, gfp_t gfp)
> if (!msp->sample_table)
> goto error1;
>
> + msp->legacy.r = msp->ratelist;
> + msp->legacy.sample_table = msp->sample_table;
> +
> return msp;
>
> error1:
Hi John,
As Felix submitted a replacement patch, please drop this one.
Gr. AvS
On 02/08/2012 06:29 PM, Felix Fietkau wrote:
> On 2012-02-08 6:06 PM, Felix Fietkau wrote:
>> On 2012-02-08 5:27 PM, Arend van Spriel wrote:
>>> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>>>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>>>> This patch fixes the following NULL pointer dereference:
>>>>>
>>>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>>>
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>>>> with 98:fc:11:8e:94:57
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>>>> kernel NULL pointer dereference at 00000004
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>>>
>>>>> static inline int
>>>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>>>> {
>>>>> int i = rix;
>>>>> for (i = rix; i >= 0; i--)
>>>>> if (mi->r[i].rix == rix)
>>>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>>>> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
>>>>> 4d: 83 c2 01 add $0x1,%edx
>>>>>
>>>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>>>
>>>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>>>> is not set until rate_init which is after assoc.
>>>> I'm not sure this is the right fix. If I understand the issue correctly,
>>>> mi->r is only NULL because .rate_init has not been called yet, which is
>>>> a bug that other rate control modules might trip over as well, maybe in
>>>> more subtle ways. If this is the case, then I believe this bug should be
>>>> fixed in mac80211 instead.
>>>>
>>>> Maybe it would even be a good idea to set a flag after calling
>>>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>>>> before that.
>>>>
>>>> - Felix
>>>>
>>>
>>> Hi Felix,
>>>
>>> I am not sure I can agree. minstrel_ht allocates the struct
>>> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
>>> default and zero'ing the legacy field. When setting is_ht to false
>>> consequently the contents of the legacy field should be properly set as
>>> is done in minstrel_ht_update_caps (at use_legacy label).
>> Right, minstrel_ht_update_caps initializes that field, and it's always
>> called from .rate_init.
>>
>> While the non-HT minstrel initializes that field earlier (because it is
>> not stored anywhere else), it still also assumes that it isn't asked to
>> do tx until .rate_init is called.
>>
>> It doesn't matter whether it crashes in non-HT minstrel or not, the main
>> issue is that assumptions of both minstrel and minstrel_ht (and possibly
>> other rate control implementations) are being violated by mac80211.
>>
>> Before we can address that problem, we need to decide whether we want to
>> change the code to no longer require .rate_init before tx (which I think
>> is a bad idea, since there's no list of supported rates available yet at
>> that point in time), or if we change mac80211 to no longer violate those
>> assumptions. In either case, this patch is insufficient.
> Actually, I took a quick look at the code and it seems to be easier
> than I thought. mac80211 sets a flag after rate_control_rate_init
> already, only the tx status path is lacking a check for it. This simple
> patch should take care of it, please test:
>
> ---
> --- a/net/mac80211/rate.h
> +++ b/net/mac80211/rate.h
> @@ -37,7 +37,7 @@ static inline void rate_control_tx_statu
> struct ieee80211_sta *ista = &sta->sta;
> void *priv_sta = sta->rate_ctrl_priv;
>
> - if (!ref)
> + if (!ref || !test_sta_flag(sta, WLAN_STA_RATE_CONTROL))
> return;
>
> ref->ops->tx_status(ref->priv, sband, ista, priv_sta, skb);
>
>
Works for me so you can sent this patch with
Tested-by: Arend van Spriel <[email protected]>
Gr. AvS
On 2012-02-08 1:44 PM, Arend van Spriel wrote:
> This patch fixes the following NULL pointer dereference:
>
>> I rerun the test on a kernel with some more lock checking and got lucky.
>>
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>> with 98:fc:11:8e:94:57
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>> 98:fc:11:8e:94:57 (try 1/3)
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>> kernel NULL pointer dereference at 00000004
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>> minstrel_tx_status+0x48/0xe0 [mac80211]
>
> static inline int
> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
> {
> int i = rix;
> for (i = rix; i >= 0; i--)
> if (mi->r[i].rix == rix)
> 48: 3b 58 04 cmp 0x4(%eax),%ebx
> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
> 4d: 83 c2 01 add $0x1,%edx
>
> It fails because of mi->r[i] being a NULL pointer. It is allocated in
> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>
> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
> struct minstrel_sta_info. During authenticate the bool is_ht is false so
> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
> is not set until rate_init which is after assoc.
I'm not sure this is the right fix. If I understand the issue correctly,
mi->r is only NULL because .rate_init has not been called yet, which is
a bug that other rate control modules might trip over as well, maybe in
more subtle ways. If this is the case, then I believe this bug should be
fixed in mac80211 instead.
Maybe it would even be a good idea to set a flag after calling
rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
before that.
- Felix
On 02/08/2012 06:14 PM, Daniel Halperin wrote:
> On Wed, Feb 8, 2012 at 9:06 AM, Felix Fietkau <[email protected]<mailto:[email protected]>> wrote:
> On 2012-02-08 5:27 PM, Arend van Spriel wrote:
>> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>>> This patch fixes the following NULL pointer dereference:
>>>>
>>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>>
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>>> with 98:fc:11:8e:94:57
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>>> kernel NULL pointer dereference at 00000004
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>>
>>>> static inline int
>>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>>> {
>>>> int i = rix;
>>>> for (i = rix; i >= 0; i--)
>>>> if (mi->r[i].rix == rix)
>>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>>> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
>>>> 4d: 83 c2 01 add $0x1,%edx
>>>>
>>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>>
>>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>>> is not set until rate_init which is after assoc.
>>> I'm not sure this is the right fix. If I understand the issue correctly,
>>> mi->r is only NULL because .rate_init has not been called yet, which is
>>> a bug that other rate control modules might trip over as well, maybe in
>>> more subtle ways. If this is the case, then I believe this bug should be
>>> fixed in mac80211 instead.
>>>
>>> Maybe it would even be a good idea to set a flag after calling
>>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>>> before that.
>>>
>>> - Felix
>>>
>>
>> Hi Felix,
>>
>> I am not sure I can agree. minstrel_ht allocates the struct
>> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
>> default and zero'ing the legacy field. When setting is_ht to false
>> consequently the contents of the legacy field should be properly set as
>> is done in minstrel_ht_update_caps (at use_legacy label).
> Right, minstrel_ht_update_caps initializes that field, and it's always
> called from .rate_init.
>
> While the non-HT minstrel initializes that field earlier (because it is
> not stored anywhere else), it still also assumes that it isn't asked to
> do tx until .rate_init is called.
>
> It doesn't matter whether it crashes in non-HT minstrel or not, the main
> issue is that assumptions of both minstrel and minstrel_ht (and possibly
> other rate control implementations) are being violated by mac80211.
>
> Before we can address that problem, we need to decide whether we want to
> change the code to no longer require .rate_init before tx (which I think
> is a bad idea, since there's no list of supported rates available yet at
> that point in time), or if we change mac80211 to no longer violate those
> assumptions. In either case, this patch is insufficient.
>
> My two cents: Felix is right---this whole process is a bit confusing and hard to work around. I know that iwlwifi has had to implement a bunch of weird things because of the strange order in which mac80211 tells rate modules about clients. Taking the time work out a sane order, that's clearly specified, would help a lot in this space. We can patch minstrel_ht but it doesn't make it easier for other modules.
>
> Dan
>
Ok,
I think we are discussing the rate_control interface here and what ops
is called when.
The .alloc_sta is being called when a sta is created. The .rate_init
callback is called at different points in mac80211 depending on the mode
(STA, P2P, TDLS, etc.). Commonality seems to be that .rate_init is being
called when the connection has been established, ie. when the actual
rates to be used over that connection are known/negotiated. Before this
point the supported rates available are basically the ones supported by
the local device and these are known upon calling .alloc_sta. The
negotiated rates are always the supported rates or a subset.
Gr. AvS
On 2012-02-08 6:35 PM, Arend van Spriel wrote:
> On 02/08/2012 06:29 PM, Felix Fietkau wrote:
>> On 2012-02-08 6:06 PM, Felix Fietkau wrote:
>>> On 2012-02-08 5:27 PM, Arend van Spriel wrote:
>>>> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>>>>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>>>>> This patch fixes the following NULL pointer dereference:
>>>>>>
>>>>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>>>>
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>>>>> with 98:fc:11:8e:94:57
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>>>>> kernel NULL pointer dereference at 00000004
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>>>>
>>>>>> static inline int
>>>>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>>>>> {
>>>>>> int i = rix;
>>>>>> for (i = rix; i >= 0; i--)
>>>>>> if (mi->r[i].rix == rix)
>>>>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>>>>> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
>>>>>> 4d: 83 c2 01 add $0x1,%edx
>>>>>>
>>>>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>>>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>>>>
>>>>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>>>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>>>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>>>>> is not set until rate_init which is after assoc.
>>>>> I'm not sure this is the right fix. If I understand the issue correctly,
>>>>> mi->r is only NULL because .rate_init has not been called yet, which is
>>>>> a bug that other rate control modules might trip over as well, maybe in
>>>>> more subtle ways. If this is the case, then I believe this bug should be
>>>>> fixed in mac80211 instead.
>>>>>
>>>>> Maybe it would even be a good idea to set a flag after calling
>>>>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>>>>> before that.
>>>>>
>>>>> - Felix
>>>>>
>>>>
>>>> Hi Felix,
>>>>
>>>> I am not sure I can agree. minstrel_ht allocates the struct
>>>> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
>>>> default and zero'ing the legacy field. When setting is_ht to false
>>>> consequently the contents of the legacy field should be properly set as
>>>> is done in minstrel_ht_update_caps (at use_legacy label).
>>> Right, minstrel_ht_update_caps initializes that field, and it's always
>>> called from .rate_init.
>>>
>>> While the non-HT minstrel initializes that field earlier (because it is
>>> not stored anywhere else), it still also assumes that it isn't asked to
>>> do tx until .rate_init is called.
>>>
>>> It doesn't matter whether it crashes in non-HT minstrel or not, the main
>>> issue is that assumptions of both minstrel and minstrel_ht (and possibly
>>> other rate control implementations) are being violated by mac80211.
>>>
>>> Before we can address that problem, we need to decide whether we want to
>>> change the code to no longer require .rate_init before tx (which I think
>>> is a bad idea, since there's no list of supported rates available yet at
>>> that point in time), or if we change mac80211 to no longer violate those
>>> assumptions. In either case, this patch is insufficient.
>> Actually, I took a quick look at the code and it seems to be easier
>> than I thought. mac80211 sets a flag after rate_control_rate_init
>> already, only the tx status path is lacking a check for it. This simple
>> patch should take care of it, please test:
>>
>
> I will test it. So basically we don't want to do rate control for
> management frames like AUTH and ASSOC?
Right. That's already handled like this by the current code, only the tx
status path was left out (probably accidentally).
- Felix
On 02/08/2012 12:04 PM, Arend van Spriel wrote:
> On 02/08/2012 06:29 PM, Felix Fietkau wrote:
>> On 2012-02-08 6:06 PM, Felix Fietkau wrote:
>>> On 2012-02-08 5:27 PM, Arend van Spriel wrote:
>>>> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>>>>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>>>>> This patch fixes the following NULL pointer dereference:
>>>>>>
>>>>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>>>>
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>>>>> with 98:fc:11:8e:94:57
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>>>>> kernel NULL pointer dereference at 00000004
>>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>>>>
>>>>>> static inline int
>>>>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>>>>> {
>>>>>> int i = rix;
>>>>>> for (i = rix; i>= 0; i--)
>>>>>> if (mi->r[i].rix == rix)
>>>>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>>>>> 4b: 74 4b je 98<minstrel_tx_status+0x98>
>>>>>> 4d: 83 c2 01 add $0x1,%edx
>>>>>>
>>>>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>>>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>>>>
>>>>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>>>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>>>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>>>>> is not set until rate_init which is after assoc.
>>>>> I'm not sure this is the right fix. If I understand the issue correctly,
>>>>> mi->r is only NULL because .rate_init has not been called yet, which is
>>>>> a bug that other rate control modules might trip over as well, maybe in
>>>>> more subtle ways. If this is the case, then I believe this bug should be
>>>>> fixed in mac80211 instead.
>>>>>
>>>>> Maybe it would even be a good idea to set a flag after calling
>>>>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>>>>> before that.
>>>>>
>>>>> - Felix
>>>>>
>>>>
>>>> Hi Felix,
>>>>
>>>> I am not sure I can agree. minstrel_ht allocates the struct
>>>> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
>>>> default and zero'ing the legacy field. When setting is_ht to false
>>>> consequently the contents of the legacy field should be properly set as
>>>> is done in minstrel_ht_update_caps (at use_legacy label).
>>> Right, minstrel_ht_update_caps initializes that field, and it's always
>>> called from .rate_init.
>>>
>>> While the non-HT minstrel initializes that field earlier (because it is
>>> not stored anywhere else), it still also assumes that it isn't asked to
>>> do tx until .rate_init is called.
>>>
>>> It doesn't matter whether it crashes in non-HT minstrel or not, the main
>>> issue is that assumptions of both minstrel and minstrel_ht (and possibly
>>> other rate control implementations) are being violated by mac80211.
>>>
>>> Before we can address that problem, we need to decide whether we want to
>>> change the code to no longer require .rate_init before tx (which I think
>>> is a bad idea, since there's no list of supported rates available yet at
>>> that point in time), or if we change mac80211 to no longer violate those
>>> assumptions. In either case, this patch is insufficient.
>> Actually, I took a quick look at the code and it seems to be easier
>> than I thought. mac80211 sets a flag after rate_control_rate_init
>> already, only the tx status path is lacking a check for it. This simple
>> patch should take care of it, please test:
>>
>> ---
>> --- a/net/mac80211/rate.h
>> +++ b/net/mac80211/rate.h
>> @@ -37,7 +37,7 @@ static inline void rate_control_tx_statu
>> struct ieee80211_sta *ista =&sta->sta;
>> void *priv_sta = sta->rate_ctrl_priv;
>>
>> - if (!ref)
>> + if (!ref || !test_sta_flag(sta, WLAN_STA_RATE_CONTROL))
>> return;
>>
>> ref->ops->tx_status(ref->priv, sband, ista, priv_sta, skb);
>>
>>
>
> Works for me so you can sent this patch with
> Tested-by: Arend van Spriel<[email protected]>
It works for me as well.
Larry
On 02/08/2012 06:44 AM, Arend van Spriel wrote:
> This patch fixes the following NULL pointer dereference:
>
>> I rerun the test on a kernel with some more lock checking and got lucky.
>>
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>> with 98:fc:11:8e:94:57
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>> 98:fc:11:8e:94:57 (try 1/3)
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>> kernel NULL pointer dereference at 00000004
>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>> minstrel_tx_status+0x48/0xe0 [mac80211]
>
> static inline int
> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
> {
> int i = rix;
> for (i = rix; i>= 0; i--)
> if (mi->r[i].rix == rix)
> 48: 3b 58 04 cmp 0x4(%eax),%ebx
> 4b: 74 4b je 98<minstrel_tx_status+0x98>
> 4d: 83 c2 01 add $0x1,%edx
>
> It fails because of mi->r[i] being a NULL pointer. It is allocated in
> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>
> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
> struct minstrel_sta_info. During authenticate the bool is_ht is false so
> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
> is not set until rate_init which is after assoc.
>
> Cc: Johannes Berg<[email protected]>
> Cc: Felix Fietkau<[email protected]>
> Signed-off-by: Arend van Spriel<[email protected]>
> ---
> John,
>
> This one with tab indent instead of spaces. Noticed just after sending
> the first patch.
>
> Gr. AvS
> ---
> net/mac80211/rc80211_minstrel_ht.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
> index ff5f7b8..a58c348 100644
> --- a/net/mac80211/rc80211_minstrel_ht.c
> +++ b/net/mac80211/rc80211_minstrel_ht.c
> @@ -813,6 +813,9 @@ minstrel_ht_alloc_sta(void *priv, struct ieee80211_sta *sta, gfp_t gfp)
> if (!msp->sample_table)
> goto error1;
>
> + msp->legacy.r = msp->ratelist;
> + msp->legacy.sample_table = msp->sample_table;
> +
> return msp;
>
> error1:
This patch fixes the problem I was having with minstrel_tx_status with p54usb.
John: If you want, you may add a
Tested-by: Larry Finger <[email protected]>
Arend: Thanks for finding this problem and fixing it.
Larry
On 02/08/2012 06:29 PM, Felix Fietkau wrote:
> On 2012-02-08 6:06 PM, Felix Fietkau wrote:
>> On 2012-02-08 5:27 PM, Arend van Spriel wrote:
>>> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>>>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>>>> This patch fixes the following NULL pointer dereference:
>>>>>
>>>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>>>
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>>>> with 98:fc:11:8e:94:57
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>>>> kernel NULL pointer dereference at 00000004
>>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>>>
>>>>> static inline int
>>>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>>>> {
>>>>> int i = rix;
>>>>> for (i = rix; i >= 0; i--)
>>>>> if (mi->r[i].rix == rix)
>>>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>>>> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
>>>>> 4d: 83 c2 01 add $0x1,%edx
>>>>>
>>>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>>>
>>>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>>>> is not set until rate_init which is after assoc.
>>>> I'm not sure this is the right fix. If I understand the issue correctly,
>>>> mi->r is only NULL because .rate_init has not been called yet, which is
>>>> a bug that other rate control modules might trip over as well, maybe in
>>>> more subtle ways. If this is the case, then I believe this bug should be
>>>> fixed in mac80211 instead.
>>>>
>>>> Maybe it would even be a good idea to set a flag after calling
>>>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>>>> before that.
>>>>
>>>> - Felix
>>>>
>>>
>>> Hi Felix,
>>>
>>> I am not sure I can agree. minstrel_ht allocates the struct
>>> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
>>> default and zero'ing the legacy field. When setting is_ht to false
>>> consequently the contents of the legacy field should be properly set as
>>> is done in minstrel_ht_update_caps (at use_legacy label).
>> Right, minstrel_ht_update_caps initializes that field, and it's always
>> called from .rate_init.
>>
>> While the non-HT minstrel initializes that field earlier (because it is
>> not stored anywhere else), it still also assumes that it isn't asked to
>> do tx until .rate_init is called.
>>
>> It doesn't matter whether it crashes in non-HT minstrel or not, the main
>> issue is that assumptions of both minstrel and minstrel_ht (and possibly
>> other rate control implementations) are being violated by mac80211.
>>
>> Before we can address that problem, we need to decide whether we want to
>> change the code to no longer require .rate_init before tx (which I think
>> is a bad idea, since there's no list of supported rates available yet at
>> that point in time), or if we change mac80211 to no longer violate those
>> assumptions. In either case, this patch is insufficient.
> Actually, I took a quick look at the code and it seems to be easier
> than I thought. mac80211 sets a flag after rate_control_rate_init
> already, only the tx status path is lacking a check for it. This simple
> patch should take care of it, please test:
>
I will test it. So basically we don't want to do rate control for
management frames like AUTH and ASSOC?
Gr. AvS
On 02/08/2012 10:27 AM, Arend van Spriel wrote:
> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>> This patch fixes the following NULL pointer dereference:
>>>
>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>> with 98:fc:11:8e:94:57
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>> kernel NULL pointer dereference at 00000004
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>
>>> static inline int
>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>> {
>>> int i = rix;
>>> for (i = rix; i>= 0; i--)
>>> if (mi->r[i].rix == rix)
>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>> 4b: 74 4b je 98<minstrel_tx_status+0x98>
>>> 4d: 83 c2 01 add $0x1,%edx
>>>
>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>
>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>> is not set until rate_init which is after assoc.
>> I'm not sure this is the right fix. If I understand the issue correctly,
>> mi->r is only NULL because .rate_init has not been called yet, which is
>> a bug that other rate control modules might trip over as well, maybe in
>> more subtle ways. If this is the case, then I believe this bug should be
>> fixed in mac80211 instead.
>>
>> Maybe it would even be a good idea to set a flag after calling
>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>> before that.
>>
>> - Felix
>>
>
> Hi Felix,
>
> I am not sure I can agree. minstrel_ht allocates the struct
> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
> default and zero'ing the legacy field. When setting is_ht to false
> consequently the contents of the legacy field should be properly set as
> is done in minstrel_ht_update_caps (at use_legacy label).
Once you settle on a "final" patch, I can test it as I can force the crash
nearly instantly.
Larry
On 2012-02-08 5:27 PM, Arend van Spriel wrote:
> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>> This patch fixes the following NULL pointer dereference:
>>>
>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>> with 98:fc:11:8e:94:57
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>> kernel NULL pointer dereference at 00000004
>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>
>>> static inline int
>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>> {
>>> int i = rix;
>>> for (i = rix; i >= 0; i--)
>>> if (mi->r[i].rix == rix)
>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
>>> 4d: 83 c2 01 add $0x1,%edx
>>>
>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>
>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>> is not set until rate_init which is after assoc.
>> I'm not sure this is the right fix. If I understand the issue correctly,
>> mi->r is only NULL because .rate_init has not been called yet, which is
>> a bug that other rate control modules might trip over as well, maybe in
>> more subtle ways. If this is the case, then I believe this bug should be
>> fixed in mac80211 instead.
>>
>> Maybe it would even be a good idea to set a flag after calling
>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>> before that.
>>
>> - Felix
>>
>
> Hi Felix,
>
> I am not sure I can agree. minstrel_ht allocates the struct
> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
> default and zero'ing the legacy field. When setting is_ht to false
> consequently the contents of the legacy field should be properly set as
> is done in minstrel_ht_update_caps (at use_legacy label).
Right, minstrel_ht_update_caps initializes that field, and it's always
called from .rate_init.
While the non-HT minstrel initializes that field earlier (because it is
not stored anywhere else), it still also assumes that it isn't asked to
do tx until .rate_init is called.
It doesn't matter whether it crashes in non-HT minstrel or not, the main
issue is that assumptions of both minstrel and minstrel_ht (and possibly
other rate control implementations) are being violated by mac80211.
Before we can address that problem, we need to decide whether we want to
change the code to no longer require .rate_init before tx (which I think
is a bad idea, since there's no list of supported rates available yet at
that point in time), or if we change mac80211 to no longer violate those
assumptions. In either case, this patch is insufficient.
- Felix
On 2/8/2012 6:40 PM, Felix Fietkau wrote:
>> I will test it. So basically we don't want to do rate control for
>> management frames like AUTH and ASSOC?
> Right. That's already handled like this by the current code, only the tx
> status path was left out (probably accidentally).
Yes, my mistake, I forgot about TX status path. Thanks everyone for
taking care of this!
johannes
On 2012-02-08 6:06 PM, Felix Fietkau wrote:
> On 2012-02-08 5:27 PM, Arend van Spriel wrote:
>> On 02/08/2012 05:08 PM, Felix Fietkau wrote:
>>> On 2012-02-08 1:44 PM, Arend van Spriel wrote:
>>>> This patch fixes the following NULL pointer dereference:
>>>>
>>>>> I rerun the test on a kernel with some more lock checking and got lucky.
>>>>>
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512283] wlan0: authenticate
>>>>> with 98:fc:11:8e:94:57
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.512515] wlan0: send auth to
>>>>> 98:fc:11:8e:94:57 (try 1/3)
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514184] BUG: unable to handle
>>>>> kernel NULL pointer dereference at 00000004
>>>>> Feb 8 08:40:17 lb-bun-10 kernel: [ 514.514233] IP: [<f8648f08>]
>>>>> minstrel_tx_status+0x48/0xe0 [mac80211]
>>>>
>>>> static inline int
>>>> rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>>>> {
>>>> int i = rix;
>>>> for (i = rix; i >= 0; i--)
>>>> if (mi->r[i].rix == rix)
>>>> 48: 3b 58 04 cmp 0x4(%eax),%ebx
>>>> 4b: 74 4b je 98 <minstrel_tx_status+0x98>
>>>> 4d: 83 c2 01 add $0x1,%edx
>>>>
>>>> It fails because of mi->r[i] being a NULL pointer. It is allocated in
>>>> minstel_alloc_sta, but minstrel_ht_alloc_sta does not call that function.
>>>>
>>>> struct minstrel_ht_sta_priv holds a union with struct minstrel_ht_sta and
>>>> struct minstrel_sta_info. During authenticate the bool is_ht is false so
>>>> minstrel_ht_tx_status calls minstrel_tx_status, but minstrel_sta_info::r
>>>> is not set until rate_init which is after assoc.
>>> I'm not sure this is the right fix. If I understand the issue correctly,
>>> mi->r is only NULL because .rate_init has not been called yet, which is
>>> a bug that other rate control modules might trip over as well, maybe in
>>> more subtle ways. If this is the case, then I believe this bug should be
>>> fixed in mac80211 instead.
>>>
>>> Maybe it would even be a good idea to set a flag after calling
>>> rate_control_rate_init, and calling WARN_ON or even BUG_ON when doing tx
>>> before that.
>>>
>>> - Felix
>>>
>>
>> Hi Felix,
>>
>> I am not sure I can agree. minstrel_ht allocates the struct
>> minstrel_ht_sta_priv initializing is_ht to false thereby choosing a
>> default and zero'ing the legacy field. When setting is_ht to false
>> consequently the contents of the legacy field should be properly set as
>> is done in minstrel_ht_update_caps (at use_legacy label).
> Right, minstrel_ht_update_caps initializes that field, and it's always
> called from .rate_init.
>
> While the non-HT minstrel initializes that field earlier (because it is
> not stored anywhere else), it still also assumes that it isn't asked to
> do tx until .rate_init is called.
>
> It doesn't matter whether it crashes in non-HT minstrel or not, the main
> issue is that assumptions of both minstrel and minstrel_ht (and possibly
> other rate control implementations) are being violated by mac80211.
>
> Before we can address that problem, we need to decide whether we want to
> change the code to no longer require .rate_init before tx (which I think
> is a bad idea, since there's no list of supported rates available yet at
> that point in time), or if we change mac80211 to no longer violate those
> assumptions. In either case, this patch is insufficient.
Actually, I took a quick look at the code and it seems to be easier
than I thought. mac80211 sets a flag after rate_control_rate_init
already, only the tx status path is lacking a check for it. This simple
patch should take care of it, please test:
---
--- a/net/mac80211/rate.h
+++ b/net/mac80211/rate.h
@@ -37,7 +37,7 @@ static inline void rate_control_tx_statu
struct ieee80211_sta *ista = &sta->sta;
void *priv_sta = sta->rate_ctrl_priv;
- if (!ref)
+ if (!ref || !test_sta_flag(sta, WLAN_STA_RATE_CONTROL))
return;
ref->ops->tx_status(ref->priv, sband, ista, priv_sta, skb);