[bug report] nl80211/cfg80211: add radar detection command/event

[bug report] nl80211/cfg80211: add radar detection command/event

[Dan Carpenter]

Hi wireless devs,

The patch 04f39047af2a: "nl80211/cfg80211: add radar detection
command/event" from Feb 8, 2013, leads to the following static
checker warning:

	net/wireless/chan.c:250 cfg80211_set_chans_dfs_state()
	warn: 'center_freq + bandwidth / 2 - 10' negative user limit promoted to high

net/wireless/chan.c
   242  static void cfg80211_set_chans_dfs_state(struct wiphy *wiphy, u32 center_freq,
   243                                           u32 bandwidth,
   244                                           enum nl80211_dfs_state dfs_state)
   245  {
   246          struct ieee80211_channel *c;
   247          u32 freq;
   248  
   249          for (freq = center_freq - bandwidth/2 + 10;
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   250               freq <= center_freq + bandwidth/2 - 10;
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This isn't really a big issue but center_freq comes from
nla_get_u32(attrs[NL80211_ATTR_WIPHY_FREQ]) in nl80211_parse_chandef().
Smatch is complaining that there is an issue with the math
over/underflowing.  It just means that we loop for a long time.  It's
not a security problem.  Even without the overflow, we could end up
looping for a long time.

Is center_freq capped somewhere that I haven't seen?

   251               freq += 20) {
   252                  c = ieee80211_get_channel(wiphy, freq);
   253                  if (!c || !(c->flags & IEEE80211_CHAN_RADAR))
   254                          continue;
   255  
   256                  c->dfs_state = dfs_state;
   257                  c->dfs_state_entered = jiffies;
   258          }
   259  }

regards,
dan carpenter

Re: [bug report] nl80211/cfg80211: add radar detection command/event

[Johannes Berg]

Hi Dan,

> The patch 04f39047af2a: "nl80211/cfg80211: add radar detection
> command/event" from Feb 8, 2013, leads to the following static
> checker warning:

Huh, that's kinda old :-)

I was wondering if it was surfaced by a recent station-side patch, but I
guess not.

> net/wireless/chan.c
>    242  static void cfg80211_set_chans_dfs_state(struct wiphy *wiphy, u32 center_freq,
>    243                                           u32 bandwidth,
>    244                                           enum nl80211_dfs_state dfs_state)
>    245  {
>    246          struct ieee80211_channel *c;
>    247          u32 freq;
>    248  
>    249          for (freq = center_freq - bandwidth/2 + 10;
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    250               freq <= center_freq + bandwidth/2 - 10;
>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[snip]

> This isn't really a big issue but center_freq comes from
> nla_get_u32(attrs[NL80211_ATTR_WIPHY_FREQ]) in nl80211_parse_chandef().
> Smatch is complaining that there is an issue with the math
> over/underflowing.  It just means that we loop for a long time.  It's
> not a security problem.  Even without the overflow, we could end up
> looping for a long time.
> 
> Is center_freq capped somewhere that I haven't seen?

I think so, but I'm not sure how you got here.

So we can only get to the code quoted above from
cfg80211_set_dfs_state(), which passes

        cfg80211_set_chans_dfs_state(wiphy, chandef->center_freq1,
                                     width, dfs_state);

or

        cfg80211_set_chans_dfs_state(wiphy, chandef->center_freq2,
                                     width, dfs_state);

It also checks cfg80211_chandef_valid() which limits the center_freq1
and 2 against each other, but that code might also have over- or under-
flows.

cfg80211_set_dfs_state() is called from

 * cfg80211_radar_event() - that should be OK, coming from driver code
 * regulatory_propagate_dfs_state() - also seems OK with kernel source
   of the data
 * nl80211_notify_radar_detection() which calls
   cfg80211_chandef_dfs_required() - that checks that the channel is
   actually advertised by a driver in some way, so you can't really get
   here with an invalid frequency afaict.

However, the way that cfg80211_chandef_dfs_required() checks is very
similar:

cfg80211_chandef_dfs_required()
-> cfg80211_get_chans_dfs_required(center_freq)
-> start_freq = center_freq - bandwidth/2 + 10;
   end_freq = center_freq + bandwidth/2 - 10;
   for (freq = start_freq; freq <= end_freq; freq += 20)



However, as you say we get center_freq from
nla_get_u32(attrs[NL80211_ATTR_WIPHY_FREQ]) in nl80211_parse_chandef().

In nl80211_parse_chandef() we have this code:

        control_freq = nla_get_u32(attrs[NL80211_ATTR_WIPHY_FREQ]);

        chandef->chan = ieee80211_get_channel(&rdev->wiphy, control_freq);


which requires that the control_freq (or center_freq later) is actually
a valid frequency that's advertised by the driver as a valid channel.
This of course will not be the case for things like 0 or 0xffffffff or
close to them.

However (again), we use center_freq1/center_freq2 as well - center_freq1
is validated against control_freq by cfg80211_chandef_valid(), but
center_freq2 isn't and cannot. For center_freq2, we just get to
cfg80211_secondary_chans_ok() eventually, which also contains:

        start_freq = cfg80211_get_start_freq(center_freq, bandwidth);
        end_freq = cfg80211_get_end_freq(center_freq, bandwidth);

        for (freq = start_freq; freq <= end_freq; freq += 20) {

and here it actually looks like we can get into overflow/underflow
issues.

I believe no driver actually supports 80+80MHz today so even this is
theoretical, but it makes sense to some semi-sane range, like

        [NL80211_ATTR_CENTER_FREQ2] = NLA_POLICY_RANGE(NLA_U32, 2000, 7000),

That will avoid the over/underflow issues and limit the iteration there
to the normal bandwidth-dependent, which is only 4 steps.

Need to check though if this actually ends up getting used for 60GHz
band, in which case we couldn't use NLA_POLICY_RANGE (it has s16 for
min/max for policy size reasons), but we could assign a validation
function instead and reject anything outside the range of e.g.
200..100000, as long as we avoid the over/underflow we don't really need
to worry about the exact range, right?

johannes