* [bug report] wifi: mt76: Check link_conf pointer in mt76_connac_mcu_sta_basic_tlv()
@ 2025-03-21 14:33 Dan Carpenter
2025-03-21 16:29 ` Lorenzo Bianconi
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-03-21 14:33 UTC (permalink / raw)
To: Shayne Chen; +Cc: linux-wireless, linux-mediatek
Hello Shayne Chen,
This is a semi-automatic email about new static checker warnings.
Commit 9890624c1b39 ("wifi: mt76: Check link_conf pointer in
mt76_connac_mcu_sta_basic_tlv()") from Mar 11, 2025, leads to the
following Smatch complaint:
drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c:394 mt76_connac_mcu_sta_basic_tlv()
warn: variable dereferenced before check 'link_conf' (see line 376)
drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c
375 {
376 struct ieee80211_vif *vif = link_conf->vif;
^^^^^^^^^^^^^^
Dereferenced.
377 struct sta_rec_basic *basic;
378 struct tlv *tlv;
379 int conn_type;
380
381 tlv = mt76_connac_mcu_add_tlv(skb, STA_REC_BASIC, sizeof(*basic));
382
383 basic = (struct sta_rec_basic *)tlv;
384 basic->extra_info = cpu_to_le16(EXTRA_INFO_VER);
385
386 if (newly && conn_state != CONN_STATE_DISCONNECT)
387 basic->extra_info |= cpu_to_le16(EXTRA_INFO_NEW);
388 basic->conn_state = conn_state;
389
390 if (!link_sta) {
391 basic->conn_type = cpu_to_le32(CONNECTION_INFRA_BC);
392
393 if (vif->type == NL80211_IFTYPE_STATION &&
394 link_conf && !is_zero_ether_addr(link_conf->bssid)) {
^^^^^^^^^
The patch adds a NULL dereference but it's too late.
395 memcpy(basic->peer_addr, link_conf->bssid, ETH_ALEN);
396 basic->aid = cpu_to_le16(vif->cfg.aid);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] wifi: mt76: Check link_conf pointer in mt76_connac_mcu_sta_basic_tlv() 2025-03-21 14:33 [bug report] wifi: mt76: Check link_conf pointer in mt76_connac_mcu_sta_basic_tlv() Dan Carpenter @ 2025-03-21 16:29 ` Lorenzo Bianconi 2025-03-24 2:07 ` Shayne Chen 0 siblings, 1 reply; 4+ messages in thread From: Lorenzo Bianconi @ 2025-03-21 16:29 UTC (permalink / raw) To: Dan Carpenter; +Cc: Shayne Chen, linux-wireless, linux-mediatek [-- Attachment #1: Type: text/plain, Size: 1823 bytes --] > Hello Shayne Chen, > > This is a semi-automatic email about new static checker warnings. > > Commit 9890624c1b39 ("wifi: mt76: Check link_conf pointer in > mt76_connac_mcu_sta_basic_tlv()") from Mar 11, 2025, leads to the > following Smatch complaint: > > drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c:394 mt76_connac_mcu_sta_basic_tlv() > warn: variable dereferenced before check 'link_conf' (see line 376) > > drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c > 375 { > 376 struct ieee80211_vif *vif = link_conf->vif; > ^^^^^^^^^^^^^^ Reviewing the codebase, it seems to me it is safe to revert 9890624c1b39 since link_conf is always not NULL running mt76_connac_mcu_sta_basic_tlv(). @Shayne Chen: agree? Regards, Lorenzo > Dereferenced. > > 377 struct sta_rec_basic *basic; > 378 struct tlv *tlv; > 379 int conn_type; > 380 > 381 tlv = mt76_connac_mcu_add_tlv(skb, STA_REC_BASIC, sizeof(*basic)); > 382 > 383 basic = (struct sta_rec_basic *)tlv; > 384 basic->extra_info = cpu_to_le16(EXTRA_INFO_VER); > 385 > 386 if (newly && conn_state != CONN_STATE_DISCONNECT) > 387 basic->extra_info |= cpu_to_le16(EXTRA_INFO_NEW); > 388 basic->conn_state = conn_state; > 389 > 390 if (!link_sta) { > 391 basic->conn_type = cpu_to_le32(CONNECTION_INFRA_BC); > 392 > 393 if (vif->type == NL80211_IFTYPE_STATION && > 394 link_conf && !is_zero_ether_addr(link_conf->bssid)) { > ^^^^^^^^^ > The patch adds a NULL dereference but it's too late. > > 395 memcpy(basic->peer_addr, link_conf->bssid, ETH_ALEN); > 396 basic->aid = cpu_to_le16(vif->cfg.aid); > > regards, > dan carpenter > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] wifi: mt76: Check link_conf pointer in mt76_connac_mcu_sta_basic_tlv() 2025-03-21 16:29 ` Lorenzo Bianconi @ 2025-03-24 2:07 ` Shayne Chen 2025-03-25 13:44 ` Lorenzo Bianconi 0 siblings, 1 reply; 4+ messages in thread From: Shayne Chen @ 2025-03-24 2:07 UTC (permalink / raw) To: Lorenzo Bianconi, Dan Carpenter; +Cc: linux-wireless, linux-mediatek On Fri, 2025-03-21 at 17:29 +0100, Lorenzo Bianconi wrote: > > Hello Shayne Chen, > > > > This is a semi-automatic email about new static checker warnings. > > > > Commit 9890624c1b39 ("wifi: mt76: Check link_conf pointer in > > mt76_connac_mcu_sta_basic_tlv()") from Mar 11, 2025, leads to the > > following Smatch complaint: > > > > drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c:394 > > mt76_connac_mcu_sta_basic_tlv() > > warn: variable dereferenced before check 'link_conf' (see line > > 376) > > > > drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c > > 375 { > > 376 struct ieee80211_vif *vif = link_conf->vif; > > ^^^^^^^^^^^^^^ > > Reviewing the codebase, it seems to me it is safe to revert > 9890624c1b39 since > link_conf is always not NULL running mt76_connac_mcu_sta_basic_tlv(). > @Shayne Chen: agree? > link_conf won't be NULL in this function at the moment, but it could be NULL after adding "MLO reconfiguration" support. So in our internal tree, we directly pass struct ieee80211_vif to this function. Both methods are fine to me, what do you think? Regards, Shayne > Regards, > Lorenzo > > > Dereferenced. > > > > 377 struct sta_rec_basic *basic; > > 378 struct tlv *tlv; > > 379 int conn_type; > > 380 > > 381 tlv = mt76_connac_mcu_add_tlv(skb, STA_REC_BASIC, > > sizeof(*basic)); > > 382 > > 383 basic = (struct sta_rec_basic *)tlv; > > 384 basic->extra_info = cpu_to_le16(EXTRA_INFO_VER); > > 385 > > 386 if (newly && conn_state != CONN_STATE_DISCONNECT) > > 387 basic->extra_info |= > > cpu_to_le16(EXTRA_INFO_NEW); > > 388 basic->conn_state = conn_state; > > 389 > > 390 if (!link_sta) { > > 391 basic->conn_type = > > cpu_to_le32(CONNECTION_INFRA_BC); > > 392 > > 393 if (vif->type == NL80211_IFTYPE_STATION && > > 394 link_conf && > > !is_zero_ether_addr(link_conf->bssid)) { > > ^^^^^^^^^ > > The patch adds a NULL dereference but it's too late. > > > > 395 memcpy(basic->peer_addr, > > link_conf->bssid, ETH_ALEN); > > 396 basic->aid = cpu_to_le16(vif- > > >cfg.aid); > > > > regards, > > dan carpenter > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] wifi: mt76: Check link_conf pointer in mt76_connac_mcu_sta_basic_tlv() 2025-03-24 2:07 ` Shayne Chen @ 2025-03-25 13:44 ` Lorenzo Bianconi 0 siblings, 0 replies; 4+ messages in thread From: Lorenzo Bianconi @ 2025-03-25 13:44 UTC (permalink / raw) To: Shayne Chen; +Cc: Dan Carpenter, linux-wireless, linux-mediatek [-- Attachment #1: Type: text/plain, Size: 2739 bytes --] On Mar 24, Shayne Chen wrote: > On Fri, 2025-03-21 at 17:29 +0100, Lorenzo Bianconi wrote: > > > Hello Shayne Chen, > > > > > > This is a semi-automatic email about new static checker warnings. > > > > > > Commit 9890624c1b39 ("wifi: mt76: Check link_conf pointer in > > > mt76_connac_mcu_sta_basic_tlv()") from Mar 11, 2025, leads to the > > > following Smatch complaint: > > > > > > drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c:394 > > > mt76_connac_mcu_sta_basic_tlv() > > > warn: variable dereferenced before check 'link_conf' (see line > > > 376) > > > > > > drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c > > > 375 { > > > 376 struct ieee80211_vif *vif = link_conf->vif; > > > ^^^^^^^^^^^^^^ > > > > Reviewing the codebase, it seems to me it is safe to revert > > 9890624c1b39 since > > link_conf is always not NULL running mt76_connac_mcu_sta_basic_tlv(). > > @Shayne Chen: agree? > > > link_conf won't be NULL in this function at the moment, but it could be > NULL after adding "MLO reconfiguration" support. So in our internal > tree, we directly pass struct ieee80211_vif to this function. ack, but at the moment in mt76_connac_mcu_sta_basic_tlv() assumes link_conf is not NULL since we dereference it to get vif pointer. > > Both methods are fine to me, what do you think? I would prefer the revert for the moment and modify the signature when it is necessary. Regards, Lorenzo > > Regards, > Shayne > > > Regards, > > Lorenzo > > > > > Dereferenced. > > > > > > 377 struct sta_rec_basic *basic; > > > 378 struct tlv *tlv; > > > 379 int conn_type; > > > 380 > > > 381 tlv = mt76_connac_mcu_add_tlv(skb, STA_REC_BASIC, > > > sizeof(*basic)); > > > 382 > > > 383 basic = (struct sta_rec_basic *)tlv; > > > 384 basic->extra_info = cpu_to_le16(EXTRA_INFO_VER); > > > 385 > > > 386 if (newly && conn_state != CONN_STATE_DISCONNECT) > > > 387 basic->extra_info |= > > > cpu_to_le16(EXTRA_INFO_NEW); > > > 388 basic->conn_state = conn_state; > > > 389 > > > 390 if (!link_sta) { > > > 391 basic->conn_type = > > > cpu_to_le32(CONNECTION_INFRA_BC); > > > 392 > > > 393 if (vif->type == NL80211_IFTYPE_STATION && > > > 394 link_conf && > > > !is_zero_ether_addr(link_conf->bssid)) { > > > ^^^^^^^^^ > > > The patch adds a NULL dereference but it's too late. > > > > > > 395 memcpy(basic->peer_addr, > > > link_conf->bssid, ETH_ALEN); > > > 396 basic->aid = cpu_to_le16(vif- > > > >cfg.aid); > > > > > > regards, > > > dan carpenter > > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-03-25 13:54 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-21 14:33 [bug report] wifi: mt76: Check link_conf pointer in mt76_connac_mcu_sta_basic_tlv() Dan Carpenter 2025-03-21 16:29 ` Lorenzo Bianconi 2025-03-24 2:07 ` Shayne Chen 2025-03-25 13:44 ` Lorenzo Bianconi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox