| Message ID | 20220121111418.9144-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | iwlwifi: mvm: Fix potential NULL dereference for sta | expand |
On Fri, 2022-01-21 at 12:14 +0100, Takashi Iwai wrote: > The recent fix for NULL sta in iwl_mvm_get_tx_rate() may still hit a > potential NULL dereference, as iwl_mvm_sta_from_mac80211() is called > unconditionally (although this doesn't seem happening, practically > seen, thanks to the compiler optimization). > No objection to the patch, but I think the description isn't quite right? static inline struct iwl_mvm_sta * iwl_mvm_sta_from_mac80211(struct ieee80211_sta *sta) { return (void *)sta->drv_priv; } looks like a dereference, but I _think_ struct ieee80211_sta { [...] /* must be last */ u8 drv_priv[] __aligned(sizeof(void *)); }; means it's just an address calculation, i.e. the same as if we had return (void *)((u8 *)sta + offsetof(typeof(*sta), drv_priv)); no? I guess technically it's still UB doing calculations on a NULL pointer, but practically that's going to work. Anyway, no objections :) johannes
On Fri, 21 Jan 2022 12:22:05 +0100, Johannes Berg wrote: > > On Fri, 2022-01-21 at 12:14 +0100, Takashi Iwai wrote: > > The recent fix for NULL sta in iwl_mvm_get_tx_rate() may still hit a > > potential NULL dereference, as iwl_mvm_sta_from_mac80211() is called > > unconditionally (although this doesn't seem happening, practically > > seen, thanks to the compiler optimization). > > > > No objection to the patch, but I think the description isn't quite > right? > > static inline struct iwl_mvm_sta * > iwl_mvm_sta_from_mac80211(struct ieee80211_sta *sta) > { > return (void *)sta->drv_priv; > } > > looks like a dereference, but I _think_ > > struct ieee80211_sta { > [...] > > /* must be last */ > u8 drv_priv[] __aligned(sizeof(void *)); > }; > > > means it's just an address calculation, i.e. the same as if we had > > return (void *)((u8 *)sta + offsetof(typeof(*sta), drv_priv)); > > no? Yeah, indeed, that won't access the member. > I guess technically it's still UB doing calculations on a NULL pointer, > but practically that's going to work. > > Anyway, no objections :) OK, I'll submit v2 with rephrasing for avoid confusion. Takashi
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c index 6fa2c12f7955..4d1ddca73fb0 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c @@ -318,15 +318,14 @@ static u32 iwl_mvm_get_tx_rate(struct iwl_mvm *mvm, /* info->control is only relevant for non HW rate control */ if (!ieee80211_hw_check(mvm->hw, HAS_RATE_CONTROL)) { - struct iwl_mvm_sta *mvmsta = iwl_mvm_sta_from_mac80211(sta); - /* HT rate doesn't make sense for a non data frame */ WARN_ONCE(info->control.rates[0].flags & IEEE80211_TX_RC_MCS && !ieee80211_is_data(fc), "Got a HT rate (flags:0x%x/mcs:%d/fc:0x%x/state:%d) for a non data frame\n", info->control.rates[0].flags, info->control.rates[0].idx, - le16_to_cpu(fc), sta ? mvmsta->sta_state : -1); + le16_to_cpu(fc), + sta ? iwl_mvm_sta_from_mac80211(sta)->sta_state : -1); rate_idx = info->control.rates[0].idx; }
The recent fix for NULL sta in iwl_mvm_get_tx_rate() may still hit a potential NULL dereference, as iwl_mvm_sta_from_mac80211() is called unconditionally (although this doesn't seem happening, practically seen, thanks to the compiler optimization). This patch addresses it by dropping the temporary variable. Fixes: d599f714b73e ("iwlwifi: mvm: don't crash on invalid rate w/o STA") Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)