* Re: [bug report] nl80211/cfg80211: add radar detection command/event
2018-12-20 11:17 [bug report] nl80211/cfg80211: add radar detection command/event Dan Carpenter
@ 2018-12-20 14:16 ` Johannes Berg
0 siblings, 0 replies; 2+ messages in thread
From: Johannes Berg @ 2018-12-20 14:16 UTC (permalink / raw)
To: Dan Carpenter, linux-wireless
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
^ permalink raw reply [flat|nested] 2+ messages in thread