Hello Emmanuel Grumbach,
The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12,
2021, leads to the following Smatch static checker warning:
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:2640 iwl_mvm_start_ap_ibss()
error: NULL dereference inside function '__iwl_mvm_mac_set_key()'
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
2559 static int iwl_mvm_start_ap_ibss(struct ieee80211_hw *hw,
2560 struct ieee80211_vif *vif)
2561 {
2562 struct iwl_mvm *mvm = IWL_MAC80211_GET_MVM(hw);
2563 struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
2564 int ret, i;
2565
2566 mutex_lock(&mvm->mutex);
2567
2568 /* Send the beacon template */
2569 ret = iwl_mvm_mac_ctxt_beacon_changed(mvm, vif);
2570 if (ret)
2571 goto out_unlock;
2572
2573 /*
2574 * Re-calculate the tsf id, as the leader-follower relations depend on
2575 * the beacon interval, which was not known when the AP interface
2576 * was added.
2577 */
2578 if (vif->type == NL80211_IFTYPE_AP)
2579 iwl_mvm_mac_ctxt_recalc_tsf_id(mvm, vif);
2580
2581 mvmvif->ap_assoc_sta_count = 0;
2582
2583 /* Add the mac context */
2584 ret = iwl_mvm_mac_ctxt_add(mvm, vif);
2585 if (ret)
2586 goto out_unlock;
2587
2588 /* Perform the binding */
2589 ret = iwl_mvm_binding_add_vif(mvm, vif);
2590 if (ret)
2591 goto out_remove;
2592
2593 /*
2594 * This is not very nice, but the simplest:
2595 * For older FWs adding the mcast sta before the bcast station may
2596 * cause assert 0x2b00.
2597 * This is fixed in later FW so make the order of removal depend on
2598 * the TLV
2599 */
2600 if (fw_has_api(&mvm->fw->ucode_capa, IWL_UCODE_TLV_API_STA_TYPE)) {
2601 ret = iwl_mvm_add_mcast_sta(mvm, vif);
2602 if (ret)
2603 goto out_unbind;
2604 /*
2605 * Send the bcast station. At this stage the TBTT and DTIM time
2606 * events are added and applied to the scheduler
2607 */
2608 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
2609 if (ret) {
2610 iwl_mvm_rm_mcast_sta(mvm, vif);
2611 goto out_unbind;
2612 }
2613 } else {
2614 /*
2615 * Send the bcast station. At this stage the TBTT and DTIM time
2616 * events are added and applied to the scheduler
2617 */
2618 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
2619 if (ret)
2620 goto out_unbind;
2621 ret = iwl_mvm_add_mcast_sta(mvm, vif);
2622 if (ret) {
2623 iwl_mvm_send_rm_bcast_sta(mvm, vif);
2624 goto out_unbind;
2625 }
2626 }
2627
2628 /* must be set before quota calculations */
2629 mvmvif->ap_ibss_active = true;
2630
2631 /* send all the early keys to the device now */
2632 for (i = 0; i < ARRAY_SIZE(mvmvif->ap_early_keys); i++) {
2633 struct ieee80211_key_conf *key = mvmvif->ap_early_keys[i];
2634
2635 if (!key)
2636 continue;
2637
2638 mvmvif->ap_early_keys[i] = NULL;
2639
--> 2640 ret = __iwl_mvm_mac_set_key(hw, SET_KEY, vif, NULL, key);
^^^^
This passes a NULL "sta" and now it will always crash. (Possibly it
used to sometimes crash before your patch but the static checker does
not mind about that. :P).
2641 if (ret)
2642 goto out_quota_failed;
2643 }
2644
2645 if (vif->type == NL80211_IFTYPE_AP && !vif->p2p) {
2646 iwl_mvm_vif_set_low_latency(mvmvif, true,
2647 LOW_LATENCY_VIF_TYPE);
2648 iwl_mvm_send_low_latency_cmd(mvm, true, mvmvif->id);
2649 }
2650
2651 /* power updated needs to be done before quotas */
2652 iwl_mvm_power_update_mac(mvm);
2653
2654 ret = iwl_mvm_update_quotas(mvm, false, NULL);
2655 if (ret)
2656 goto out_quota_failed;
2657
2658 /* Need to update the P2P Device MAC (only GO, IBSS is single vif) */
2659 if (vif->p2p && mvm->p2p_device_vif)
2660 iwl_mvm_mac_ctxt_changed(mvm, mvm->p2p_device_vif, false, NULL);
2661
2662 iwl_mvm_bt_coex_vif_change(mvm);
2663
2664 /* we don't support TDLS during DCM */
2665 if (iwl_mvm_phy_ctx_count(mvm) > 1)
2666 iwl_mvm_teardown_tdls_peers(mvm);
2667
2668 iwl_mvm_ftm_restart_responder(mvm, vif);
2669
2670 goto out_unlock;
2671
2672 out_quota_failed:
2673 iwl_mvm_power_update_mac(mvm);
2674 mvmvif->ap_ibss_active = false;
2675 iwl_mvm_send_rm_bcast_sta(mvm, vif);
2676 iwl_mvm_rm_mcast_sta(mvm, vif);
2677 out_unbind:
2678 iwl_mvm_binding_remove_vif(mvm, vif);
2679 out_remove:
2680 iwl_mvm_mac_ctxt_remove(mvm, vif);
2681 out_unlock:
2682 mutex_unlock(&mvm->mutex);
2683 return ret;
2684 }
regards,
dan carpenter
Hi Dan,
> Hello Emmanuel Grumbach,
>
> The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12, 2021,
> leads to the following Smatch static checker warning:
>
> drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:2640
> iwl_mvm_start_ap_ibss()
> error: NULL dereference inside function
> '__iwl_mvm_mac_set_key()'
Where in __iwl_mvm_mac_set_key() ?
This function should be able to cope with with a NULL sta I think.
I don't really see how this could be related to my patch since iwlmei is not related to AP mode at all.
I also moved to that commit, but the line numbers don't match, so I am a bit confused.
>
> drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
> 2559 static int iwl_mvm_start_ap_ibss(struct ieee80211_hw *hw,
> 2560 struct ieee80211_vif *vif)
> 2561 {
> 2562 struct iwl_mvm *mvm = IWL_MAC80211_GET_MVM(hw);
> 2563 struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
> 2564 int ret, i;
> 2565
> 2566 mutex_lock(&mvm->mutex);
> 2567
> 2568 /* Send the beacon template */
> 2569 ret = iwl_mvm_mac_ctxt_beacon_changed(mvm, vif);
> 2570 if (ret)
> 2571 goto out_unlock;
> 2572
> 2573 /*
> 2574 * Re-calculate the tsf id, as the leader-follower relations depend
> on
> 2575 * the beacon interval, which was not known when the AP
> interface
> 2576 * was added.
> 2577 */
> 2578 if (vif->type == NL80211_IFTYPE_AP)
> 2579 iwl_mvm_mac_ctxt_recalc_tsf_id(mvm, vif);
> 2580
> 2581 mvmvif->ap_assoc_sta_count = 0;
> 2582
> 2583 /* Add the mac context */
> 2584 ret = iwl_mvm_mac_ctxt_add(mvm, vif);
> 2585 if (ret)
> 2586 goto out_unlock;
> 2587
> 2588 /* Perform the binding */
> 2589 ret = iwl_mvm_binding_add_vif(mvm, vif);
> 2590 if (ret)
> 2591 goto out_remove;
> 2592
> 2593 /*
> 2594 * This is not very nice, but the simplest:
> 2595 * For older FWs adding the mcast sta before the bcast station
> may
> 2596 * cause assert 0x2b00.
> 2597 * This is fixed in later FW so make the order of removal depend
> on
> 2598 * the TLV
> 2599 */
> 2600 if (fw_has_api(&mvm->fw->ucode_capa,
> IWL_UCODE_TLV_API_STA_TYPE)) {
> 2601 ret = iwl_mvm_add_mcast_sta(mvm, vif);
> 2602 if (ret)
> 2603 goto out_unbind;
> 2604 /*
> 2605 * Send the bcast station. At this stage the TBTT and DTIM
> time
> 2606 * events are added and applied to the scheduler
> 2607 */
> 2608 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
> 2609 if (ret) {
> 2610 iwl_mvm_rm_mcast_sta(mvm, vif);
> 2611 goto out_unbind;
> 2612 }
> 2613 } else {
> 2614 /*
> 2615 * Send the bcast station. At this stage the TBTT and DTIM
> time
> 2616 * events are added and applied to the scheduler
> 2617 */
> 2618 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
> 2619 if (ret)
> 2620 goto out_unbind;
> 2621 ret = iwl_mvm_add_mcast_sta(mvm, vif);
> 2622 if (ret) {
> 2623 iwl_mvm_send_rm_bcast_sta(mvm, vif);
> 2624 goto out_unbind;
> 2625 }
> 2626 }
> 2627
> 2628 /* must be set before quota calculations */
> 2629 mvmvif->ap_ibss_active = true;
> 2630
> 2631 /* send all the early keys to the device now */
> 2632 for (i = 0; i < ARRAY_SIZE(mvmvif->ap_early_keys); i++) {
> 2633 struct ieee80211_key_conf *key = mvmvif->ap_early_keys[i];
> 2634
> 2635 if (!key)
> 2636 continue;
> 2637
> 2638 mvmvif->ap_early_keys[i] = NULL;
> 2639
> --> 2640 ret = __iwl_mvm_mac_set_key(hw, SET_KEY, vif, NULL,
> key);
> ^^^^ This passes a NULL "sta" and now it
> will always crash. (Possibly it used to sometimes crash before your patch but
> the static checker does not mind about that. :P).
This existed long before my patch
>
> 2641 if (ret)
> 2642 goto out_quota_failed;
> 2643 }
> 2644
> 2645 if (vif->type == NL80211_IFTYPE_AP && !vif->p2p) {
> 2646 iwl_mvm_vif_set_low_latency(mvmvif, true,
> 2647 LOW_LATENCY_VIF_TYPE);
> 2648 iwl_mvm_send_low_latency_cmd(mvm, true, mvmvif->id);
> 2649 }
> 2650
> 2651 /* power updated needs to be done before quotas */
> 2652 iwl_mvm_power_update_mac(mvm);
> 2653
> 2654 ret = iwl_mvm_update_quotas(mvm, false, NULL);
> 2655 if (ret)
> 2656 goto out_quota_failed;
> 2657
> 2658 /* Need to update the P2P Device MAC (only GO, IBSS is single
> vif) */
> 2659 if (vif->p2p && mvm->p2p_device_vif)
> 2660 iwl_mvm_mac_ctxt_changed(mvm, mvm->p2p_device_vif,
> false, NULL);
> 2661
> 2662 iwl_mvm_bt_coex_vif_change(mvm);
> 2663
> 2664 /* we don't support TDLS during DCM */
> 2665 if (iwl_mvm_phy_ctx_count(mvm) > 1)
> 2666 iwl_mvm_teardown_tdls_peers(mvm);
> 2667
> 2668 iwl_mvm_ftm_restart_responder(mvm, vif);
> 2669
> 2670 goto out_unlock;
> 2671
> 2672 out_quota_failed:
> 2673 iwl_mvm_power_update_mac(mvm);
> 2674 mvmvif->ap_ibss_active = false;
> 2675 iwl_mvm_send_rm_bcast_sta(mvm, vif);
> 2676 iwl_mvm_rm_mcast_sta(mvm, vif);
> 2677 out_unbind:
> 2678 iwl_mvm_binding_remove_vif(mvm, vif);
> 2679 out_remove:
> 2680 iwl_mvm_mac_ctxt_remove(mvm, vif);
> 2681 out_unlock:
> 2682 mutex_unlock(&mvm->mutex);
> 2683 return ret;
> 2684 }
>
> regards,
> dan carpenter
On Tue, Nov 30, 2021 at 08:09:48AM +0000, Grumbach, Emmanuel wrote:
> Hi Dan,
>
> > Hello Emmanuel Grumbach,
> >
> > The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12, 2021,
> > leads to the following Smatch static checker warning:
> >
> > drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:2640
> > iwl_mvm_start_ap_ibss()
> > error: NULL dereference inside function
> > '__iwl_mvm_mac_set_key()'
>
> Where in __iwl_mvm_mac_set_key() ?
> This function should be able to cope with with a NULL sta I think.
> I don't really see how this could be related to my patch since iwlmei is not related to AP mode at all.
>
> I also moved to that commit, but the line numbers don't match, so I am a bit confused.
>
I'm on yesterday's linux-next. The patch moves the mvmsta assignment to
the start of the function.
mvmsta = iwl_mvm_sta_from_mac80211(sta);
^^^^
It introduces a couple other Smatch warnings as well.
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:3643 __iwl_mvm_mac_set_key() warn: variable dereferenced before check 'sta' (see line 3594)
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:3773 __iwl_mvm_mac_set_key() warn: variable dereferenced before check 'sta' (see line 3594)
regards,
dan carpenter