iwlwifi: integrate with iwlmei

bug report Dan Carpenter 2 participants 3 messages Nov 30, 2021

Share & tools

Grumbach, Emmanuel

2021-11-30 08:09 UTC #1 lore raw

Hi Dan,

Quoted 9 lines

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.

Quoted 90 lines

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

Quoted 5 lines

--> 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

Quoted 50 lines

    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

Dan Carpenter

author

2021-11-30 09:08 UTC #2 lore raw

Quoted 12 lines · Emmanuel

Hi Dan,
Quoted 9 lines
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()'

Quoted 6 lines

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