Fix a possible NULL pointer dereference in ieee80211_compatible_rates
introduced in the patch "mac80211: fix association with some APs". If no bss
is available just use all supported rates in the association request.
Signed-off-by: Helmut Schaa <[email protected]>
---
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 76ad4ed..3f7f92a 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device *dev,
capab |= WLAN_CAPABILITY_PRIVACY;
if (bss->wmm_ie)
wmm = 1;
+
+ /* get all rates supported by the device and the AP as
+ * some APs don't like getting a superset of their rates
+ * in the association request (e.g. D-Link DAP 1353 in
+ * b-only mode) */
+ rates_len = ieee80211_compatible_rates(bss, sband, &rates);
+
ieee80211_rx_bss_put(dev, bss);
+ } else {
+ rates = ~0;
+ rates_len = sband->n_bitrates;
}
mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
@@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device *dev,
*pos++ = ifsta->ssid_len;
memcpy(pos, ifsta->ssid, ifsta->ssid_len);
- /* all supported rates should be added here but some APs
- * (e.g. D-Link DAP 1353 in b-only mode) don't like that
- * Therefore only add rates the AP supports */
- rates_len = ieee80211_compatible_rates(bss, sband, &rates);
+ /* add all rates which were marked to be used above */
supp_rates_len = rates_len;
if (supp_rates_len > 8)
supp_rates_len = 8;
On Wed, May 21, 2008 at 01:47:04PM +0300, Tomas Winkler wrote:
> On Tue, May 20, 2008 at 3:54 PM, Tomas Winkler <[email protected]> wrote:
> > I found one ieee80211_rx_bss_{get,put} imbalance in
> > ieee80211_sta_join_ibss function
> > That may cause this problem yet it doesn't look like this is the case.
> > ieee80211_sta_join_ibss
> > calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
>
> The patch below introduced _get/_put imbalance. ieee80211_rx_bss_info
> _put bss back at the end. Other callers of the ieee80211_sta_join_ibss
> function don't use put.
> I will post a patch that takes out the _put out of
> ieee80211_rx_bss_info, I think it's more readable.
Since you are doing _get and _add in ieee80211_rx_bss_info, it makes
sense to me to do _put at the end of it. Perhaps we should remove
the _put from the end of ieee80211_sta_join_ibss and change it's
callers instead?
John
--
John W. Linville
[email protected]
On Tue, May 20, 2008 at 3:57 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-05-20 at 15:54 +0300, Tomas Winkler wrote:
>> On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <[email protected]> wrote:
>> > Fix a possible NULL pointer dereference in ieee80211_compatible_rates
>> > introduced in the patch "mac80211: fix association with some APs". If no bss
>> > is available just use all supported rates in the association request.
>> >
>> > Signed-off-by: Helmut Schaa <[email protected]>
>> > ---
>> >
>> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
>> > index 76ad4ed..3f7f92a 100644
>> > --- a/net/mac80211/mlme.c
>> > +++ b/net/mac80211/mlme.c
>> > @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
>> > *dev,
>> > capab |= WLAN_CAPABILITY_PRIVACY;
>> > if (bss->wmm_ie)
>> > wmm = 1;
>> > +
>> > + /* get all rates supported by the device and the AP as
>> > + * some APs don't like getting a superset of their rates
>> > + * in the association request (e.g. D-Link DAP 1353 in
>> > + * b-only mode) */
>> > + rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> > +
>> > ieee80211_rx_bss_put(dev, bss);
>> > + } else {
>> > + rates = ~0;
>> > + rates_len = sband->n_bitrates;
>> > }
>> >
>> > mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
>> > @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
>> > *dev,
>> > *pos++ = ifsta->ssid_len;
>> > memcpy(pos, ifsta->ssid, ifsta->ssid_len);
>> >
>> > - /* all supported rates should be added here but some APs
>> > - * (e.g. D-Link DAP 1353 in b-only mode) don't like that
>> > - * Therefore only add rates the AP supports */
>> > - rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> > + /* add all rates which were marked to be used above */
>> > supp_rates_len = rates_len;
>> > if (supp_rates_len > 8)
>> > supp_rates_len = 8;
>> >
>> >
>>
>> I found one ieee80211_rx_bss_{get,put} imbalance in
>> ieee80211_sta_join_ibss function
>> That may cause this problem yet it doesn't look like this is the case.
>> ieee80211_sta_join_ibss
>> calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
>> Keep searching.
>
> Hm. Send a patch? :)
>
Searching for the real problem ...
>> I suggest to insert at least some WARN_ON(1) for the else case.
>
> Disagree, not until somebody audits the code. We already know it can
> happen and a WARN() won't help us track it down because it provides no
> additional information (stack trace is useless)
What about printk(KERN_WARN ), The else statement actually means that
something wrong happened.
Tomas
On Wed, May 21, 2008 at 4:54 PM, John W. Linville
<[email protected]> wrote:
> On Wed, May 21, 2008 at 01:47:04PM +0300, Tomas Winkler wrote:
>> On Tue, May 20, 2008 at 3:54 PM, Tomas Winkler <[email protected]> wrote:
>
>> > I found one ieee80211_rx_bss_{get,put} imbalance in
>> > ieee80211_sta_join_ibss function
>> > That may cause this problem yet it doesn't look like this is the case.
>> > ieee80211_sta_join_ibss
>> > calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
>>
>> The patch below introduced _get/_put imbalance. ieee80211_rx_bss_info
>> _put bss back at the end. Other callers of the ieee80211_sta_join_ibss
>> function don't use put.
>> I will post a patch that takes out the _put out of
>> ieee80211_rx_bss_info, I think it's more readable.
>
> Since you are doing _get and _add in ieee80211_rx_bss_info, it makes
> sense to me to do _put at the end of it. Perhaps we should remove
> the _put from the end of ieee80211_sta_join_ibss and change it's
> callers instead?
That what I meant I've just pasted wrong function name into the mail
(was lazy typing)
Maybe someone can answer this
static void ieee80211_rx_bss_put(struct net_device *dev,
struct ieee80211_sta_bss *bss)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
local_bh_disable();
if (!atomic_dec_and_lock(&bss->users, &local->sta_bss_lock)) {
local_bh_enable();
return;
}
---- don't we miss local_bh_enable(); here or spin_unlock_bh takes
care of this ---
__ieee80211_rx_bss_hash_del(dev, bss);
list_del(&bss->list);
spin_unlock_bh(&local->sta_bss_lock);
ieee80211_rx_bss_free(bss);
}
Thanks
Tomas
> John
> --
> John W. Linville
> [email protected]
>
> static void ieee80211_rx_bss_put(struct net_device *dev,
> struct ieee80211_sta_bss *bss)
> {
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
>
> local_bh_disable();
> if (!atomic_dec_and_lock(&bss->users, &local->sta_bss_lock)) {
> local_bh_enable();
> return;
> }
>
> ---- don't we miss local_bh_enable(); here or spin_unlock_bh takes
> care of this ---
>
>
> __ieee80211_rx_bss_hash_del(dev, bss);
> list_del(&bss->list);
> spin_unlock_bh(&local->sta_bss_lock);
spin_unlock_bh takes care of it. The local_bh_disable() +
atomic_dec_and_lock() is like spin_lock_bh() + atomic_dec() just with
different atomicity guarantees.
johannes
> >> I suggest to insert at least some WARN_ON(1) for the else case.
> >
> > Disagree, not until somebody audits the code. We already know it can
> > happen and a WARN() won't help us track it down because it provides no
> > additional information (stack trace is useless)
>
> What about printk(KERN_WARN ), The else statement actually means that
> something wrong happened.
Thing is, I'm not totally convinced it is wrong to the code while it may
or may not be wrong... I think this patch should go in first as it
actually fixes the oops, and then we can discuss the merits of adding a
warning there separately. Maybe after we look a bit at the code and try
to figure out whether it can still happen after that patch from
Abhijeet.
johannes
Johannes Berg wrote:
>>> Thing is, I'm not totally convinced it is wrong to the code while it may
>>> or may not be wrong...
>> Doesn't should be bss pinned int he bss list if you are associating to
>> it. If it's not there you don't have access to it's info It looks very
>> wrong to me.
>
> Well, yes, it is a bit odd.
>
>>> I think this patch should go in first as it
>>> actually fixes the oops, and then we can discuss the merits of adding a
>>> warning there separately. Maybe after we look a bit at the code and try
>>> to figure out whether it can still happen after that patch from
>>> Abhijeet.
>> I'm not sure if this patch is complete without this warning. What is
>> in the else statement is a hack and it should be obvious.
>
> Considering that the message won't help us at all, why bother? We know
> it's triggering, we know this might be a problem, and we know we can
> only solve it by auditing the code. So why add a message that will get
> us countless emails/complaints from people we cannot do anything about
> anyway without doing the audit?
This argument could go on endlessly; however, it is clear that we need to
settle on a patch and get it upstream ASAP! Now that mainline is broken, the
urgency is _MUCH_ greater.
Larry
On Wed, May 21, 2008 at 6:08 PM, Johannes Berg
<[email protected]> wrote:
>
>> static void ieee80211_rx_bss_put(struct net_device *dev,
>> struct ieee80211_sta_bss *bss)
>> {
>> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
>>
>> local_bh_disable();
>> if (!atomic_dec_and_lock(&bss->users, &local->sta_bss_lock)) {
>> local_bh_enable();
>> return;
>> }
>>
>> ---- don't we miss local_bh_enable(); here or spin_unlock_bh takes
>> care of this ---
>>
>>
>> __ieee80211_rx_bss_hash_del(dev, bss);
>> list_del(&bss->list);
>> spin_unlock_bh(&local->sta_bss_lock);
>
> spin_unlock_bh takes care of it. The local_bh_disable() +
> atomic_dec_and_lock() is like spin_lock_bh() + atomic_dec() just with
> different atomicity guarantees.
>
> johannes
>
OK, thanks for explanation
Tomas
On Tue, May 20, 2008 at 4:22 PM, Johannes Berg
<[email protected]> wrote:
>
>> >> I suggest to insert at least some WARN_ON(1) for the else case.
>> >
>> > Disagree, not until somebody audits the code. We already know it can
>> > happen and a WARN() won't help us track it down because it provides no
>> > additional information (stack trace is useless)
>>
>> What about printk(KERN_WARN ), The else statement actually means that
>> something wrong happened.
>
> Thing is, I'm not totally convinced it is wrong to the code while it may
> or may not be wrong...
Doesn't should be bss pinned int he bss list if you are associating to
it. If it's not there you don't have access to it's info It looks very
wrong to me.
I think this patch should go in first as it
> actually fixes the oops, and then we can discuss the merits of adding a
> warning there separately. Maybe after we look a bit at the code and try
> to figure out whether it can still happen after that patch from
> Abhijeet.
I'm not sure if this patch is complete without this warning. What is
in the else statement is a hack and it should be obvious.
Tomas
> > Thing is, I'm not totally convinced it is wrong to the code while it may
> > or may not be wrong...
>
> Doesn't should be bss pinned int he bss list if you are associating to
> it. If it's not there you don't have access to it's info It looks very
> wrong to me.
Well, yes, it is a bit odd.
> > I think this patch should go in first as it
> > actually fixes the oops, and then we can discuss the merits of adding a
> > warning there separately. Maybe after we look a bit at the code and try
> > to figure out whether it can still happen after that patch from
> > Abhijeet.
>
> I'm not sure if this patch is complete without this warning. What is
> in the else statement is a hack and it should be obvious.
Considering that the message won't help us at all, why bother? We know
it's triggering, we know this might be a problem, and we know we can
only solve it by auditing the code. So why add a message that will get
us countless emails/complaints from people we cannot do anything about
anyway without doing the audit?
johannes
On Tue, May 20, 2008 at 4:38 PM, Johannes Berg
<[email protected]> wrote:
>
>> > Thing is, I'm not totally convinced it is wrong to the code while it may
>> > or may not be wrong...
>>
>> Doesn't should be bss pinned int he bss list if you are associating to
>> it. If it's not there you don't have access to it's info It looks very
>> wrong to me.
>
> Well, yes, it is a bit odd.
>
>> > I think this patch should go in first as it
>> > actually fixes the oops, and then we can discuss the merits of adding a
>> > warning there separately. Maybe after we look a bit at the code and try
>> > to figure out whether it can still happen after that patch from
>> > Abhijeet.
>>
>> I'm not sure if this patch is complete without this warning. What is
>> in the else statement is a hack and it should be obvious.
>
> Considering that the message won't help us at all, why bother? We know
> it's triggering, we know this might be a problem, and we know we can
> only solve it by auditing the code. So why add a message that will get
> us countless emails/complaints from people we cannot do anything about
> anyway without doing the audit?
As I understand it's not easily reproducible so you need reference
point in the trace
when it happens. I'm not sure what your debug techniques are, though.
Tomas
> johannes
>
On Tue, May 20, 2008 at 3:54 PM, Tomas Winkler <[email protected]> wrote:
> On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <[email protected]> wrote:
>> Fix a possible NULL pointer dereference in ieee80211_compatible_rates
>> introduced in the patch "mac80211: fix association with some APs". If no bss
>> is available just use all supported rates in the association request.
>>
>> Signed-off-by: Helmut Schaa <[email protected]>
>> ---
>>
>> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
>> index 76ad4ed..3f7f92a 100644
>> --- a/net/mac80211/mlme.c
>> +++ b/net/mac80211/mlme.c
>> @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
>> *dev,
>> capab |= WLAN_CAPABILITY_PRIVACY;
>> if (bss->wmm_ie)
>> wmm = 1;
>> +
>> + /* get all rates supported by the device and the AP as
>> + * some APs don't like getting a superset of their rates
>> + * in the association request (e.g. D-Link DAP 1353 in
>> + * b-only mode) */
>> + rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> +
>> ieee80211_rx_bss_put(dev, bss);
>> + } else {
>> + rates = ~0;
>> + rates_len = sband->n_bitrates;
>> }
>>
>> mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
>> @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
>> *dev,
>> *pos++ = ifsta->ssid_len;
>> memcpy(pos, ifsta->ssid, ifsta->ssid_len);
>>
>> - /* all supported rates should be added here but some APs
>> - * (e.g. D-Link DAP 1353 in b-only mode) don't like that
>> - * Therefore only add rates the AP supports */
>> - rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> + /* add all rates which were marked to be used above */
>> supp_rates_len = rates_len;
>> if (supp_rates_len > 8)
>> supp_rates_len = 8;
>>
>>
>
> I found one ieee80211_rx_bss_{get,put} imbalance in
> ieee80211_sta_join_ibss function
> That may cause this problem yet it doesn't look like this is the case.
> ieee80211_sta_join_ibss
> calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
The patch below introduced _get/_put imbalance. ieee80211_rx_bss_info
_put bss back at the end. Other callers of the ieee80211_sta_join_ibss
function don't use put.
I will post a patch that takes out the _put out of
ieee80211_rx_bss_info, I think it's more readable.
commit 9d9bf77d16ba527f6f63846ca18cf20ae6e8d697
Author: Bruno Randolf <[email protected]>
Date: Mon Feb 18 11:21:36 2008 +0900
mac80211: enable IBSS merging
Tomas
On Tue, 2008-05-20 at 15:54 +0300, Tomas Winkler wrote:
> On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <[email protected]> wrote:
> > Fix a possible NULL pointer dereference in ieee80211_compatible_rates
> > introduced in the patch "mac80211: fix association with some APs". If no bss
> > is available just use all supported rates in the association request.
> >
> > Signed-off-by: Helmut Schaa <[email protected]>
> > ---
> >
> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> > index 76ad4ed..3f7f92a 100644
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
> > *dev,
> > capab |= WLAN_CAPABILITY_PRIVACY;
> > if (bss->wmm_ie)
> > wmm = 1;
> > +
> > + /* get all rates supported by the device and the AP as
> > + * some APs don't like getting a superset of their rates
> > + * in the association request (e.g. D-Link DAP 1353 in
> > + * b-only mode) */
> > + rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> > +
> > ieee80211_rx_bss_put(dev, bss);
> > + } else {
> > + rates = ~0;
> > + rates_len = sband->n_bitrates;
> > }
> >
> > mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
> > @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
> > *dev,
> > *pos++ = ifsta->ssid_len;
> > memcpy(pos, ifsta->ssid, ifsta->ssid_len);
> >
> > - /* all supported rates should be added here but some APs
> > - * (e.g. D-Link DAP 1353 in b-only mode) don't like that
> > - * Therefore only add rates the AP supports */
> > - rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> > + /* add all rates which were marked to be used above */
> > supp_rates_len = rates_len;
> > if (supp_rates_len > 8)
> > supp_rates_len = 8;
> >
> >
>
> I found one ieee80211_rx_bss_{get,put} imbalance in
> ieee80211_sta_join_ibss function
> That may cause this problem yet it doesn't look like this is the case.
> ieee80211_sta_join_ibss
> calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
> Keep searching.
Hm. Send a patch? :)
> I suggest to insert at least some WARN_ON(1) for the else case.
Disagree, not until somebody audits the code. We already know it can
happen and a WARN() won't help us track it down because it provides no
additional information (stack trace is useless)
johannes
On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <[email protected]> wrote:
> Fix a possible NULL pointer dereference in ieee80211_compatible_rates
> introduced in the patch "mac80211: fix association with some APs". If no bss
> is available just use all supported rates in the association request.
>
> Signed-off-by: Helmut Schaa <[email protected]>
> ---
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 76ad4ed..3f7f92a 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
> *dev,
> capab |= WLAN_CAPABILITY_PRIVACY;
> if (bss->wmm_ie)
> wmm = 1;
> +
> + /* get all rates supported by the device and the AP as
> + * some APs don't like getting a superset of their rates
> + * in the association request (e.g. D-Link DAP 1353 in
> + * b-only mode) */
> + rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> +
> ieee80211_rx_bss_put(dev, bss);
> + } else {
> + rates = ~0;
> + rates_len = sband->n_bitrates;
> }
>
> mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
> @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
> *dev,
> *pos++ = ifsta->ssid_len;
> memcpy(pos, ifsta->ssid, ifsta->ssid_len);
>
> - /* all supported rates should be added here but some APs
> - * (e.g. D-Link DAP 1353 in b-only mode) don't like that
> - * Therefore only add rates the AP supports */
> - rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> + /* add all rates which were marked to be used above */
> supp_rates_len = rates_len;
> if (supp_rates_len > 8)
> supp_rates_len = 8;
>
>
I found one ieee80211_rx_bss_{get,put} imbalance in
ieee80211_sta_join_ibss function
That may cause this problem yet it doesn't look like this is the case.
ieee80211_sta_join_ibss
calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
Keep searching.
I suggest to insert at least some WARN_ON(1) for the else case.
Tomas
On Tue, May 20, 2008 at 4:47 PM, Larry Finger <[email protected]> wrote:
> Johannes Berg wrote:
>>>>
>>>> Thing is, I'm not totally convinced it is wrong to the code while it may
>>>> or may not be wrong...
>>>
>>> Doesn't should be bss pinned int he bss list if you are associating to
>>> it. If it's not there you don't have access to it's info It looks very
>>> wrong to me.
>>
>> Well, yes, it is a bit odd.
>>
>>>> I think this patch should go in first as it
>>>> actually fixes the oops, and then we can discuss the merits of adding a
>>>> warning there separately. Maybe after we look a bit at the code and try
>>>> to figure out whether it can still happen after that patch from
>>>> Abhijeet.
>>>
>>> I'm not sure if this patch is complete without this warning. What is
>>> in the else statement is a hack and it should be obvious.
>>
>> Considering that the message won't help us at all, why bother? We know
>> it's triggering, we know this might be a problem, and we know we can
>> only solve it by auditing the code. So why add a message that will get
>> us countless emails/complaints from people we cannot do anything about
>> anyway without doing the audit?
>
> This argument could go on endlessly; however, it is clear that we need to
> settle on a patch and get it upstream ASAP! Now that mainline is broken, the
> urgency is _MUCH_ greater.
>
We've just arguing about warning message so I won't consider it blocking.
Tomas
On Tue, May 20, 2008 at 08:47:13AM -0500, Larry Finger wrote:
> Johannes Berg wrote:
>>>> Thing is, I'm not totally convinced it is wrong to the code while it may
>>>> or may not be wrong...
>>> Doesn't should be bss pinned int he bss list if you are associating to
>>> it. If it's not there you don't have access to it's info It looks very
>>> wrong to me.
>>
>> Well, yes, it is a bit odd.
>>
>>>> I think this patch should go in first as it
>>>> actually fixes the oops, and then we can discuss the merits of adding a
>>>> warning there separately. Maybe after we look a bit at the code and try
>>>> to figure out whether it can still happen after that patch from
>>>> Abhijeet.
>>> I'm not sure if this patch is complete without this warning. What is
>>> in the else statement is a hack and it should be obvious.
>>
>> Considering that the message won't help us at all, why bother? We know
>> it's triggering, we know this might be a problem, and we know we can
>> only solve it by auditing the code. So why add a message that will get
>> us countless emails/complaints from people we cannot do anything about
>> anyway without doing the audit?
>
> This argument could go on endlessly; however, it is clear that we need to
> settle on a patch and get it upstream ASAP! Now that mainline is broken,
> the urgency is _MUCH_ greater.
I agree. I'll take this one unless someone finds a problem with it.
John
--
John W. Linville
[email protected]