[PATCH] mac80211: fix possible NULL dereference

[PATCH] mac80211: fix possible NULL dereference

[Michal Kazior]

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(+)

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;
-- 
1.8.5.3

Re: [PATCH] mac80211: fix possible NULL dereference

[Michal Kazior]

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ł

Re: [PATCH] mac80211: fix possible NULL dereference

[Johannes Berg]

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

Re: [PATCH] mac80211: fix possible NULL dereference

[Michal Kazior]

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ł

Re: [PATCH] mac80211: fix possible NULL dereference

[Johannes Berg]

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