| Message ID | 23f0ec986ef1529055f4f93dcb3940a6cf8d9a94.1690143750.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [wireless] wifi: iwlwifi: mvm: Fix a memory corruption issue | expand |
On Sun, 2023-07-23 at 22:24 +0200, Christophe JAILLET wrote: > A few lines above, space is kzalloc()'ed for: > sizeof(struct iwl_nvm_data) + > sizeof(struct ieee80211_channel) + > sizeof(struct ieee80211_rate) > > 'mvm->nvm_data' is a 'struct iwl_nvm_data', so it is fine. > > At the end of this structure, there is the 'channels' flex array. > Each element is of type 'struct ieee80211_channel'. > So only 1 element is allocated in this array. > > When doing: > mvm->nvm_data->bands[0].channels = mvm->nvm_data->channels; > We point at the first element of the 'channels' flex array. > So this is fine. > > However, when doing: > mvm->nvm_data->bands[0].bitrates = > (void *)((u8 *)mvm->nvm_data->channels + 1); > because of the "(u8 *)" cast, we add only 1 to the address of the beginning > of the flex array. > > It is likely that we want point at the 'struct ieee80211_rate' allocated > just after. > > Remove the spurious casting so that the pointer arithmetic works as > expected. > > Fixes: 8ca151b568b6 ("iwlwifi: add the MVM driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > I've checked in the .s files, and : > > Before > ====== > # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:801: mvm->nvm_data->bands[0].channels = mvm->nvm_data->channels; > leaq 1448(%r13), %rax #, tmp248 > > # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:805: (void *)((u8 *)mvm->nvm_data->channels + 1); > leaq 1449(%r13), %rax #, tmp252 > > > After: > ===== > # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:801: mvm->nvm_data->bands[0].channels = mvm->nvm_data->channels; > leaq 1448(%r13), %rax #, tmp248 > > # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:805: (void *)(mvm->nvm_data->channels + 1); > leaq 1512(%r13), %rax #, tmp252 > > And on my system sizeof(struct ieee80211_channel) = 64 > > /!\ This patch is only speculative and untested. /!\ > > It is strange that a memory corruption issue has been un-noticed for more > than 10 years. > > So review with care. > --- > drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c > index 1f5db65a088d..1d5ee4330f29 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c > @@ -802,7 +802,7 @@ int iwl_run_init_mvm_ucode(struct iwl_mvm *mvm) > mvm->nvm_data->bands[0].n_channels = 1; > mvm->nvm_data->bands[0].n_bitrates = 1; > mvm->nvm_data->bands[0].bitrates = > - (void *)((u8 *)mvm->nvm_data->channels + 1); > + (void *)(mvm->nvm_data->channels + 1); > mvm->nvm_data->bands[0].bitrates->hw_value = 10; > } > Acked-by: Gregory Greenman <gregory.greenman@intel.com>
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c index 1f5db65a088d..1d5ee4330f29 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c @@ -802,7 +802,7 @@ int iwl_run_init_mvm_ucode(struct iwl_mvm *mvm) mvm->nvm_data->bands[0].n_channels = 1; mvm->nvm_data->bands[0].n_bitrates = 1; mvm->nvm_data->bands[0].bitrates = - (void *)((u8 *)mvm->nvm_data->channels + 1); + (void *)(mvm->nvm_data->channels + 1); mvm->nvm_data->bands[0].bitrates->hw_value = 10; }
A few lines above, space is kzalloc()'ed for: sizeof(struct iwl_nvm_data) + sizeof(struct ieee80211_channel) + sizeof(struct ieee80211_rate) 'mvm->nvm_data' is a 'struct iwl_nvm_data', so it is fine. At the end of this structure, there is the 'channels' flex array. Each element is of type 'struct ieee80211_channel'. So only 1 element is allocated in this array. When doing: mvm->nvm_data->bands[0].channels = mvm->nvm_data->channels; We point at the first element of the 'channels' flex array. So this is fine. However, when doing: mvm->nvm_data->bands[0].bitrates = (void *)((u8 *)mvm->nvm_data->channels + 1); because of the "(u8 *)" cast, we add only 1 to the address of the beginning of the flex array. It is likely that we want point at the 'struct ieee80211_rate' allocated just after. Remove the spurious casting so that the pointer arithmetic works as expected. Fixes: 8ca151b568b6 ("iwlwifi: add the MVM driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- I've checked in the .s files, and : Before ====== # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:801: mvm->nvm_data->bands[0].channels = mvm->nvm_data->channels; leaq 1448(%r13), %rax #, tmp248 # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:805: (void *)((u8 *)mvm->nvm_data->channels + 1); leaq 1449(%r13), %rax #, tmp252 After: ===== # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:801: mvm->nvm_data->bands[0].channels = mvm->nvm_data->channels; leaq 1448(%r13), %rax #, tmp248 # drivers/net/wireless/intel/iwlwifi/mvm/fw.c:805: (void *)(mvm->nvm_data->channels + 1); leaq 1512(%r13), %rax #, tmp252 And on my system sizeof(struct ieee80211_channel) = 64 /!\ This patch is only speculative and untested. /!\ It is strange that a memory corruption issue has been un-noticed for more than 10 years. So review with care. --- drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)