| Message ID | 20221115014519.2718154-1-gilad.itzkovitch@morsemicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] wifi: ieee80211: Fix for Rx fragmented action frames | expand |
Hi Gilad,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 9ea570a33ee525751e3117e266626dd705adc39e]
url: https://github.com/intel-lab-lkp/linux/commits/Gilad-Itzkovitch/wifi-ieee80211-Fix-for-Rx-fragmented-action-frames/20221115-094743
base: 9ea570a33ee525751e3117e266626dd705adc39e
patch link: https://lore.kernel.org/r/20221115014519.2718154-1-gilad.itzkovitch%40morsemicro.com
patch subject: [PATCH v2] wifi: ieee80211: Fix for Rx fragmented action frames
config: arc-randconfig-s033-20221120
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/46b4ef1bfa7b17e256b07d4a5e78c1e4f85fc617
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Gilad-Itzkovitch/wifi-ieee80211-Fix-for-Rx-fragmented-action-frames/20221115-094743
git checkout 46b4ef1bfa7b17e256b07d4a5e78c1e4f85fc617
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arc SHELL=/bin/bash net/mac80211/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> net/mac80211/rx.c:4218:45: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected restricted __le16 [usertype] seq_ctrl @@ got unsigned short [usertype] @@
net/mac80211/rx.c:4218:45: sparse: expected restricted __le16 [usertype] seq_ctrl
net/mac80211/rx.c:4218:45: sparse: got unsigned short [usertype]
vim +4218 net/mac80211/rx.c
4203
4204 static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
4205 {
4206 struct ieee80211_sub_if_data *sdata = rx->sdata;
4207 struct sk_buff *skb = rx->skb;
4208 struct ieee80211_hdr *hdr = (void *)skb->data;
4209 struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
4210 u8 *bssid = ieee80211_get_bssid(hdr, skb->len, sdata->vif.type);
4211 bool multicast = is_multicast_ether_addr(hdr->addr1) ||
4212 ieee80211_is_s1g_beacon(hdr->frame_control);
4213
4214 switch (sdata->vif.type) {
4215 case NL80211_IFTYPE_STATION:
4216 if (!bssid && !sdata->u.mgd.use_4addr)
4217 return false;
> 4218 if (ieee80211_is_first_frag(le16_to_cpu(hdr->seq_ctrl)) &&
4219 ieee80211_is_robust_mgmt_frame(skb) && !rx->sta)
4220 return false;
4221 if (multicast)
4222 return true;
4223 return ieee80211_is_our_addr(sdata, hdr->addr1, &rx->link_id);
4224 case NL80211_IFTYPE_ADHOC:
4225 if (!bssid)
4226 return false;
4227 if (ether_addr_equal(sdata->vif.addr, hdr->addr2) ||
4228 ether_addr_equal(sdata->u.ibss.bssid, hdr->addr2) ||
4229 !is_valid_ether_addr(hdr->addr2))
4230 return false;
4231 if (ieee80211_is_beacon(hdr->frame_control))
4232 return true;
4233 if (!ieee80211_bssid_match(bssid, sdata->u.ibss.bssid))
4234 return false;
4235 if (!multicast &&
4236 !ether_addr_equal(sdata->vif.addr, hdr->addr1))
4237 return false;
4238 if (!rx->sta) {
4239 int rate_idx;
4240 if (status->encoding != RX_ENC_LEGACY)
4241 rate_idx = 0; /* TODO: HT/VHT rates */
4242 else
4243 rate_idx = status->rate_idx;
4244 ieee80211_ibss_rx_no_sta(sdata, bssid, hdr->addr2,
4245 BIT(rate_idx));
4246 }
4247 return true;
4248 case NL80211_IFTYPE_OCB:
4249 if (!bssid)
4250 return false;
4251 if (!ieee80211_is_data_present(hdr->frame_control))
4252 return false;
4253 if (!is_broadcast_ether_addr(bssid))
4254 return false;
4255 if (!multicast &&
4256 !ether_addr_equal(sdata->dev->dev_addr, hdr->addr1))
4257 return false;
4258 if (!rx->sta) {
4259 int rate_idx;
4260 if (status->encoding != RX_ENC_LEGACY)
4261 rate_idx = 0; /* TODO: HT rates */
4262 else
4263 rate_idx = status->rate_idx;
4264 ieee80211_ocb_rx_no_sta(sdata, bssid, hdr->addr2,
4265 BIT(rate_idx));
4266 }
4267 return true;
4268 case NL80211_IFTYPE_MESH_POINT:
4269 if (ether_addr_equal(sdata->vif.addr, hdr->addr2))
4270 return false;
4271 if (multicast)
4272 return true;
4273 return ether_addr_equal(sdata->vif.addr, hdr->addr1);
4274 case NL80211_IFTYPE_AP_VLAN:
4275 case NL80211_IFTYPE_AP:
4276 if (!bssid)
4277 return ieee80211_is_our_addr(sdata, hdr->addr1,
4278 &rx->link_id);
4279
4280 if (!is_broadcast_ether_addr(bssid) &&
4281 !ieee80211_is_our_addr(sdata, bssid, NULL)) {
4282 /*
4283 * Accept public action frames even when the
4284 * BSSID doesn't match, this is used for P2P
4285 * and location updates. Note that mac80211
4286 * itself never looks at these frames.
4287 */
4288 if (!multicast &&
4289 !ieee80211_is_our_addr(sdata, hdr->addr1,
4290 &rx->link_id))
4291 return false;
4292 if (ieee80211_is_public_action(hdr, skb->len))
4293 return true;
4294 return ieee80211_is_beacon(hdr->frame_control);
4295 }
4296
4297 if (!ieee80211_has_tods(hdr->frame_control)) {
4298 /* ignore data frames to TDLS-peers */
4299 if (ieee80211_is_data(hdr->frame_control))
4300 return false;
4301 /* ignore action frames to TDLS-peers */
4302 if (ieee80211_is_action(hdr->frame_control) &&
4303 !is_broadcast_ether_addr(bssid) &&
4304 !ether_addr_equal(bssid, hdr->addr1))
4305 return false;
4306 }
4307
4308 /*
4309 * 802.11-2016 Table 9-26 says that for data frames, A1 must be
4310 * the BSSID - we've checked that already but may have accepted
4311 * the wildcard (ff:ff:ff:ff:ff:ff).
4312 *
4313 * It also says:
4314 * The BSSID of the Data frame is determined as follows:
4315 * a) If the STA is contained within an AP or is associated
4316 * with an AP, the BSSID is the address currently in use
4317 * by the STA contained in the AP.
4318 *
4319 * So we should not accept data frames with an address that's
4320 * multicast.
4321 *
4322 * Accepting it also opens a security problem because stations
4323 * could encrypt it with the GTK and inject traffic that way.
4324 */
4325 if (ieee80211_is_data(hdr->frame_control) && multicast)
4326 return false;
4327
4328 return true;
4329 case NL80211_IFTYPE_P2P_DEVICE:
4330 return ieee80211_is_public_action(hdr, skb->len) ||
4331 ieee80211_is_probe_req(hdr->frame_control) ||
4332 ieee80211_is_probe_resp(hdr->frame_control) ||
4333 ieee80211_is_beacon(hdr->frame_control);
4334 case NL80211_IFTYPE_NAN:
4335 /* Currently no frames on NAN interface are allowed */
4336 return false;
4337 default:
4338 break;
4339 }
4340
4341 WARN_ON_ONCE(1);
4342 return false;
4343 }
4344
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index c28c6fbf786e..94d2b8e90732 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -4215,7 +4215,8 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) case NL80211_IFTYPE_STATION: if (!bssid && !sdata->u.mgd.use_4addr) return false; - if (ieee80211_is_robust_mgmt_frame(skb) && !rx->sta) + if (ieee80211_is_first_frag(le16_to_cpu(hdr->seq_ctrl)) && + ieee80211_is_robust_mgmt_frame(skb) && !rx->sta) return false; if (multicast) return true;
The ieee80211_accept_frame() function performs a number of early checks to decide whether or not further processing needs to be done on a frame. One of those checks is the ieee80211_is_robust_mgmt_frame() function. It requires to peek into the frame payload, but because defragmentation does not occur until later on in the receive path, this peek is invalid for any fragment other than the first one. Also, in this scenario there is no STA and so the fragmented frame will be dropped later on in the process and will not reach the upper stack. This can happen with large action frames at low rates, for example, we see issues with DPP on S1G. This change will only check if the frame is robust if it's the first fragment. Invalid fragmented packets will be discarded later after defragmentation is completed. Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com> --- net/mac80211/rx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) base-commit: 9ea570a33ee525751e3117e266626dd705adc39e