Re: [bug report] wifi: mt76: mt7996: Set proper link destination address in mt7996_tx()

Lorenzo Bianconi
2025-09-23T21:17:18Z

[-- Attachment #1: Type: text/plain, Size: 5313 bytes --]

> Hello Lorenzo Bianconi,

Hi Dan,

>
> Commit f940c9b7aef6 ("wifi: mt76: mt7996: Set proper link destination
> address in mt7996_tx()") from Jul 31, 2025 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/net/wireless/mediatek/mt76/mt7996/main.c:1344 mt7996_tx()
> error: testing array offset 'link_id' after use.
>
> drivers/net/wireless/mediatek/mt76/mt7996/main.c
> 1288 static void mt7996_tx(struct ieee80211_hw *hw,
> 1289 struct ieee80211_tx_control *control,
> 1290 struct sk_buff *skb)
> 1291 {
> 1292 struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> 1293 struct mt7996_dev *dev = mt7996_hw_dev(hw);
> 1294 struct ieee80211_sta *sta = control->sta;
> 1295 struct mt7996_sta *msta = sta ? (void *)sta->drv_priv : NULL;
> 1296 struct mt76_phy *mphy = hw->priv;
> 1297 struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> 1298 struct ieee80211_vif *vif = info->control.vif;
> 1299 struct mt7996_vif *mvif = vif ? (void *)vif->drv_priv : NULL;
> 1300 struct mt76_wcid *wcid = &dev->mt76.global_wcid;
> 1301 u8 link_id = u32_get_bits(info->control.flags,
> 1302 IEEE80211_TX_CTRL_MLO_LINK);
> 1303
> 1304 rcu_read_lock();
> 1305
> 1306 /* Use primary link_id if the value from mac80211 is set to
> 1307 * IEEE80211_LINK_UNSPECIFIED.
> 1308 */
> 1309 if (link_id == IEEE80211_LINK_UNSPECIFIED) {
> 1310 if (msta)
> 1311 link_id = msta->deflink_id;
> 1312 else if (mvif)
> 1313 link_id = mvif->mt76.deflink_id;
>
> Can link_id be IEEE80211_LINK_UNSPECIFIED after this if statement?
>
> 1314 }
> 1315
> 1316 if (vif && ieee80211_vif_is_mld(vif)) {
> 1317 struct ieee80211_bss_conf *link_conf;
> 1318
> 1319 if (msta) {
> 1320 struct ieee80211_link_sta *link_sta;
> 1321
> 1322 link_sta = rcu_dereference(sta->link[link_id]);
>
> Some unchecked uses. IEEE80211_LINK_UNSPECIFIED would be off-by-one.
>
> 1323 if (!link_sta)
> 1324 link_sta = rcu_dereference(sta->link[msta->deflink_id]);
> 1325
> 1326 if (link_sta) {
> 1327 memcpy(hdr->addr1, link_sta->addr, ETH_ALEN);
> 1328 if (ether_addr_equal(sta->addr, hdr->addr3))
> 1329 memcpy(hdr->addr3, link_sta->addr, ETH_ALEN);
> 1330 }
> 1331 }
> 1332
> 1333 link_conf = rcu_dereference(vif->link_conf[link_id]);
>
> Here too.
>
> 1334 if (link_conf) {
> 1335 memcpy(hdr->addr2, link_conf->addr, ETH_ALEN);
> 1336 if (ether_addr_equal(vif->addr, hdr->addr3))
> 1337 memcpy(hdr->addr3, link_conf->addr, ETH_ALEN);
> 1338 }
> 1339 }
> 1340
> 1341 if (mvif) {
> 1342 struct mt76_vif_link *mlink = &mvif->deflink.mt76;
> 1343
> --> 1344 if (link_id < IEEE80211_LINK_UNSPECIFIED)
>
> Is this checker required?

I agree, we can get rid of this condition since if mvif (or msta) is not NULL,
link_id can't be set to IEEE80211_LINK_UNSPECIFIED. I will post a fix for it.

Regards,
Lorenzo

>
> 1345 mlink = rcu_dereference(mvif->mt76.link[link_id]);
> 1346
> 1347 if (mlink->wcid)
> 1348 wcid = mlink->wcid;
> 1349
> 1350 if (mvif->mt76.roc_phy &&
> 1351 (info->flags & IEEE80211_TX_CTL_TX_OFFCHAN)) {
> 1352 mphy = mvif->mt76.roc_phy;
> 1353 if (mphy->roc_link)
> 1354 wcid = mphy->roc_link->wcid;
> 1355 } else {
> 1356 mphy = mt76_vif_link_phy(mlink);
> 1357 }
> 1358 }
> 1359
> 1360 if (!mphy) {
> 1361 ieee80211_free_txskb(hw, skb);
> 1362 goto unlock;
> 1363 }
> 1364
> 1365 if (msta && link_id < IEEE80211_LINK_UNSPECIFIED) {
>
> And this?
>
> 1366 struct mt7996_sta_link *msta_link;
> 1367
> 1368 msta_link = rcu_dereference(msta->link[link_id]);
> 1369 if (msta_link)
> 1370 wcid = &msta_link->wcid;
> 1371 }
> 1372 mt76_tx(mphy, control->sta, wcid, skb);
> 1373 unlock:
> 1374 rcu_read_unlock();
> 1375 }
>
> regards,
> dan carpenter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]