| Message ID | 20220922031013.2150682-11-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | slab: Introduce kmalloc_size_roundup() | expand |
Kees Cook <keescook@chromium.org> writes: > In preparation for reducing the use of ksize(), explicitly track the > size of scan_cmd allocations. This also allows for noticing if the scan > size changes unexpectedly. Note that using ksize() was already incorrect > here, in the sense that ksize() would not match the actual allocation > size, which would trigger future run-time allocation bounds checking. > (In other words, memset() may know how large scan_cmd was allocated for, > but ksize() will return the upper bounds of the actually allocated memory, > causing a run-time warning about an overflow.) > > Cc: Gregory Greenman <gregory.greenman@intel.com> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: Johannes Berg <johannes.berg@intel.com> > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Via which tree is this iwlwifi patch going? Normally via wireless-next or something else?
On Thu, Sep 22, 2022 at 07:18:51AM +0300, Kalle Valo wrote: > Kees Cook <keescook@chromium.org> writes: > > > In preparation for reducing the use of ksize(), explicitly track the > > size of scan_cmd allocations. This also allows for noticing if the scan > > size changes unexpectedly. Note that using ksize() was already incorrect > > here, in the sense that ksize() would not match the actual allocation > > size, which would trigger future run-time allocation bounds checking. > > (In other words, memset() may know how large scan_cmd was allocated for, > > but ksize() will return the upper bounds of the actually allocated memory, > > causing a run-time warning about an overflow.) > > > > Cc: Gregory Greenman <gregory.greenman@intel.com> > > Cc: Kalle Valo <kvalo@kernel.org> > > Cc: Johannes Berg <johannes.berg@intel.com> > > Cc: linux-wireless@vger.kernel.org > > Cc: netdev@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Via which tree is this iwlwifi patch going? Normally via wireless-next > or something else? This doesn't depend on the kmalloc_size_roundup() helper at all, so I would be happy for it to go via wireless-next if the patch seems reasonable.
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/dev.h b/drivers/net/wireless/intel/iwlwifi/dvm/dev.h index bbd574091201..1a9eadace188 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/dev.h +++ b/drivers/net/wireless/intel/iwlwifi/dvm/dev.h @@ -696,6 +696,7 @@ struct iwl_priv { /* Scan related variables */ unsigned long scan_start; unsigned long scan_start_tsf; + size_t scan_cmd_size; void *scan_cmd; enum nl80211_band scan_band; struct cfg80211_scan_request *scan_request; diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/scan.c b/drivers/net/wireless/intel/iwlwifi/dvm/scan.c index 2d38227dfdd2..a7e85c5c8c72 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/scan.c +++ b/drivers/net/wireless/intel/iwlwifi/dvm/scan.c @@ -626,7 +626,7 @@ static int iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif) u8 active_chains; u8 scan_tx_antennas = priv->nvm_data->valid_tx_ant; int ret; - int scan_cmd_size = sizeof(struct iwl_scan_cmd) + + size_t scan_cmd_size = sizeof(struct iwl_scan_cmd) + MAX_SCAN_CHANNEL * sizeof(struct iwl_scan_channel) + priv->fw->ucode_capa.max_probe_length; const u8 *ssid = NULL; @@ -649,9 +649,15 @@ static int iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif) "fail to allocate memory for scan\n"); return -ENOMEM; } + priv->scan_cmd_size = scan_cmd_size; + } + if (priv->scan_cmd_size < scan_cmd_size) { + IWL_DEBUG_SCAN(priv, + "memory needed for scan grew unexpectedly\n"); + return -ENOMEM; } scan = priv->scan_cmd; - memset(scan, 0, scan_cmd_size); + memset(scan, 0, priv->scan_cmd_size); scan->quiet_plcp_th = IWL_PLCP_QUIET_THRESH; scan->quiet_time = IWL_ACTIVE_QUIET_TIME; diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h index bf35e130c876..214b8a525cc6 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h @@ -860,6 +860,7 @@ struct iwl_mvm { /* Scan status, cmd (pre-allocated) and auxiliary station */ unsigned int scan_status; + size_t scan_cmd_size; void *scan_cmd; struct iwl_mcast_filter_cmd *mcast_filter_cmd; /* For CDB this is low band scan type, for non-CDB - type. */ @@ -1705,7 +1706,7 @@ int iwl_mvm_update_quotas(struct iwl_mvm *mvm, bool force_upload, int iwl_mvm_reg_scan_start(struct iwl_mvm *mvm, struct ieee80211_vif *vif, struct cfg80211_scan_request *req, struct ieee80211_scan_ies *ies); -int iwl_mvm_scan_size(struct iwl_mvm *mvm); +size_t iwl_mvm_scan_size(struct iwl_mvm *mvm); int iwl_mvm_scan_stop(struct iwl_mvm *mvm, int type, bool notify); int iwl_mvm_max_scan_ie_len(struct iwl_mvm *mvm); void iwl_mvm_report_scan_aborted(struct iwl_mvm *mvm); diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c index db43c8a83a31..b9cbb18b0dcb 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c @@ -1065,7 +1065,7 @@ iwl_op_mode_mvm_start(struct iwl_trans *trans, const struct iwl_cfg *cfg, static const u8 no_reclaim_cmds[] = { TX_CMD, }; - int scan_size; + size_t scan_size; u32 min_backoff; struct iwl_mvm_csme_conn_info *csme_conn_info __maybe_unused; @@ -1299,6 +1299,7 @@ iwl_op_mode_mvm_start(struct iwl_trans *trans, const struct iwl_cfg *cfg, mvm->scan_cmd = kmalloc(scan_size, GFP_KERNEL); if (!mvm->scan_cmd) goto out_free; + mvm->scan_cmd_size = scan_size; /* invalidate ids to prevent accidental removal of sta_id 0 */ mvm->aux_sta.sta_id = IWL_MVM_INVALID_STA; diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/scan.c b/drivers/net/wireless/intel/iwlwifi/mvm/scan.c index 582a95ffc7ab..acd8803dbcdd 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/scan.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/scan.c @@ -2626,7 +2626,7 @@ static int iwl_mvm_build_scan_cmd(struct iwl_mvm *mvm, u8 scan_ver; lockdep_assert_held(&mvm->mutex); - memset(mvm->scan_cmd, 0, ksize(mvm->scan_cmd)); + memset(mvm->scan_cmd, 0, mvm->scan_cmd_size); if (!fw_has_capa(&mvm->fw->ucode_capa, IWL_UCODE_TLV_CAPA_UMAC_SCAN)) { hcmd->id = SCAN_OFFLOAD_REQUEST_CMD; @@ -3091,7 +3091,7 @@ static int iwl_mvm_scan_stop_wait(struct iwl_mvm *mvm, int type) 1 * HZ); } -static int iwl_scan_req_umac_get_size(u8 scan_ver) +static size_t iwl_scan_req_umac_get_size(u8 scan_ver) { switch (scan_ver) { case 12: @@ -3104,7 +3104,7 @@ static int iwl_scan_req_umac_get_size(u8 scan_ver) return 0; } -int iwl_mvm_scan_size(struct iwl_mvm *mvm) +size_t iwl_mvm_scan_size(struct iwl_mvm *mvm) { int base_size, tail_size; u8 scan_ver = iwl_fw_lookup_cmd_ver(mvm->fw, SCAN_REQ_UMAC,
In preparation for reducing the use of ksize(), explicitly track the size of scan_cmd allocations. This also allows for noticing if the scan size changes unexpectedly. Note that using ksize() was already incorrect here, in the sense that ksize() would not match the actual allocation size, which would trigger future run-time allocation bounds checking. (In other words, memset() may know how large scan_cmd was allocated for, but ksize() will return the upper bounds of the actually allocated memory, causing a run-time warning about an overflow.) Cc: Gregory Greenman <gregory.greenman@intel.com> Cc: Kalle Valo <kvalo@kernel.org> Cc: Johannes Berg <johannes.berg@intel.com> Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/net/wireless/intel/iwlwifi/dvm/dev.h | 1 + drivers/net/wireless/intel/iwlwifi/dvm/scan.c | 10 ++++++++-- drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 3 ++- drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 3 ++- drivers/net/wireless/intel/iwlwifi/mvm/scan.c | 6 +++--- 5 files changed, 16 insertions(+), 7 deletions(-)