Re: [PATCH v2] mac80211: minstrel_ht: fix NULL pointer dereference during auth/assoc

[Felix Fietkau <nbd@openwrt.org>]

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