diff mbox

mac80211: fix possible NULL dereference

Message ID 1394176178-8504-1-git-send-email-michal.kazior@tieto.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Michal Kazior March 7, 2014, 7:09 a.m. UTC
If chanctx is missing on a given vif then the band
is assumed to be 2GHz. However if hw doesn't
support 2GHz band then mac80211 ended up with a
NULL dereference.

This fixes a splat:

[ 4605.207223] BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
[ 4605.210789] IP: [<ffffffffa07b5635>] ieee80211_parse_bitrates+0x65/0x110 [mac80211]

The splat was preceeded by WARN_ON(!chanctx_conf)
in ieee80211_get_sdata_band().

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Michal Kazior March 11, 2014, 11:30 a.m. UTC | #1
On 7 March 2014 08:09, Michal Kazior <michal.kazior@tieto.com> wrote:
> If chanctx is missing on a given vif then the band
> is assumed to be 2GHz. However if hw doesn't
> support 2GHz band then mac80211 ended up with a
> NULL dereference.
>
> This fixes a splat:
>
> [ 4605.207223] BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
> [ 4605.210789] IP: [<ffffffffa07b5635>] ieee80211_parse_bitrates+0x65/0x110 [mac80211]
>
> The splat was preceeded by WARN_ON(!chanctx_conf)
> in ieee80211_get_sdata_band().
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

Drop this, please. There are more places that need sband to be
NULL-checked. I'll send out a more thorough patch later.


Micha?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg March 11, 2014, 1:14 p.m. UTC | #2
On Tue, 2014-03-11 at 12:30 +0100, Michal Kazior wrote:
> On 7 March 2014 08:09, Michal Kazior <michal.kazior@tieto.com> wrote:
> > If chanctx is missing on a given vif then the band
> > is assumed to be 2GHz. However if hw doesn't
> > support 2GHz band then mac80211 ended up with a
> > NULL dereference.

> Drop this, please. There are more places that need sband to be
> NULL-checked. I'll send out a more thorough patch later.

Ok. However, I'm not sure that we should ever really run into this? At
least with Luca's patches we want to not go through NULL state to start
with.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Kazior March 11, 2014, 1:25 p.m. UTC | #3
On 11 March 2014 14:14, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2014-03-11 at 12:30 +0100, Michal Kazior wrote:
>> On 7 March 2014 08:09, Michal Kazior <michal.kazior@tieto.com> wrote:
>> > If chanctx is missing on a given vif then the band
>> > is assumed to be 2GHz. However if hw doesn't
>> > support 2GHz band then mac80211 ended up with a
>> > NULL dereference.
>
>> Drop this, please. There are more places that need sband to be
>> NULL-checked. I'll send out a more thorough patch later.
>
> Ok. However, I'm not sure that we should ever really run into this? At
> least with Luca's patches we want to not go through NULL state to start
> with.

Current channel reservation patches do a sequence of
unassign_vif_chanctx() followed by assign_vif_chanctx(). This implies
you have no chanctx for a split second. All places that aren't
protected by chanctx_mtx (i.e. RCU) can get NULL chanctx during the
reassignment.

One way to trigger that would be to spam-call ieee80211_get_station().
If you get a NULL chanctx and you have 5GHz only hardware you get NULL
dereference kernel splat.

With multi-vif CSA the vulnerability timeframe will increase.


Micha?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg March 11, 2014, 4:29 p.m. UTC | #4
On Tue, 2014-03-11 at 14:25 +0100, Michal Kazior wrote:

> > Ok. However, I'm not sure that we should ever really run into this? At
> > least with Luca's patches we want to not go through NULL state to start
> > with.
> 
> Current channel reservation patches do a sequence of
> unassign_vif_chanctx() followed by assign_vif_chanctx(). This implies
> you have no chanctx for a split second. All places that aren't
> protected by chanctx_mtx (i.e. RCU) can get NULL chanctx during the
> reassignment.
> 
> One way to trigger that would be to spam-call ieee80211_get_station().
> If you get a NULL chanctx and you have 5GHz only hardware you get NULL
> dereference kernel splat.
> 
> With multi-vif CSA the vulnerability timeframe will increase.

Luca is just fixing his patches to not go through the NULL state and
directly go from the old to the new context, so that will no longer be a
concern.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aaa59d7..5513bec 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2002,6 +2002,9 @@  static int ieee80211_change_bss(struct wiphy *wiphy,
 
 	band = ieee80211_get_sdata_band(sdata);
 
+	if (WARN_ON(!wiphy->bands[band]))
+		return -EINVAL;
+
 	if (params->use_cts_prot >= 0) {
 		sdata->vif.bss_conf.use_cts_prot = params->use_cts_prot;
 		changed |= BSS_CHANGED_ERP_CTS_PROT;