| Message ID | 20250129103120.1985802-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [6.1] wifi: iwlwifi: assume known PNVM power table size | expand |
On Wed, 2025-01-29 at 13:31 +0300, Dmitry Antipov wrote: > In 'iwl_pnvm_load()', assume that the power table size is always > equal to IWL_HARDCODED_REDUCE_POWER_SIZE. Compile tested only. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > I would gently ask Johannes to review this before taking any other > actions. This is intended for stable linux-6.1.y only in attempt to > avoid possible use of an uninitialized 'len' without backporting I don't see that there's uninitialized use of 'len'. Maybe some static checkers aren't seeing through this, but the code is fine: If iwl_pnvm_get_from_fs() is successful, then 'len' is initialized. If it fails, we goto skip_parse. There, if trans->reduce_power_loaded is false, 'len' again is either initialized or we go to skip_reduce_power and never use 'len'. If trans->reduce_power_loaded is true, then we get to iwl_trans_pcie_ctx_info_gen3_set_reduce_power() which doesn't use 'len' in this case. I'd rather not fix this non-problem in a very confusing way. johannes
On 1/29/25 1:48 PM, Johannes Berg wrote: > I don't see that there's uninitialized use of 'len'. Maybe some static > checkers aren't seeing through this, but the code is fine: > > If iwl_pnvm_get_from_fs() is successful, then 'len' is initialized. If > it fails, we goto skip_parse. > > There, if trans->reduce_power_loaded is false, 'len' again is either > initialized or we go to skip_reduce_power and never use 'len'. > > If trans->reduce_power_loaded is true, then we get to > iwl_trans_pcie_ctx_info_gen3_set_reduce_power() which doesn't use 'len' > in this case. Hm. As of 6.1.127, what I'm seeing is ('cat -n' annotated by me): 258 int iwl_pnvm_load(struct iwl_trans *trans, 259 struct iwl_notif_wait_data *notif_wait) 260 { 261 u8 *data; 262 size_t len; /* uninitialized */ 263 struct pnvm_sku_package *package; 264 struct iwl_notification_wait pnvm_wait; 265 static const u16 ntf_cmds[] = { WIDE_ID(REGULATORY_AND_NVM_GROUP, 266 PNVM_INIT_COMPLETE_NTFY) }; 267 int ret; 268 269 /* if the SKU_ID is empty, there's nothing to do */ 270 if (!trans->sku_id[0] && !trans->sku_id[1] && !trans->sku_id[2]) 271 return 0; 272 273 /* 274 * If we already loaded (or tried to load) it before, we just 275 * need to set it again. 276 */ 277 if (trans->pnvm_loaded) { 278 ret = iwl_trans_set_pnvm(trans, NULL, 0); 279 if (ret) 280 return ret; 281 goto skip_parse; /* taking goto */ 282 } 283 284 /* First attempt to get the PNVM from BIOS */ 285 package = iwl_uefi_get_pnvm(trans, &len); 286 if (!IS_ERR_OR_NULL(package)) { 287 if (len >= sizeof(*package)) { 288 /* we need only the data */ 289 len -= sizeof(*package); 290 data = kmemdup(package->data, len, GFP_KERNEL); 291 } else { 292 data = NULL; 293 } 294 295 /* free package regardless of whether kmemdup succeeded */ 296 kfree(package); 297 298 if (data) 299 goto parse; 300 } 301 302 /* If it's not available, try from the filesystem */ 303 ret = iwl_pnvm_get_from_fs(trans, &data, &len); 304 if (ret) { 305 /* 306 * Pretend we've loaded it - at least we've tried and 307 * couldn't load it at all, so there's no point in 308 * trying again over and over. 309 */ 310 trans->pnvm_loaded = true; 311 312 goto skip_parse; 313 } 314 315 parse: 316 iwl_pnvm_parse(trans, data, len); 317 318 kfree(data); 319 320 skip_parse: 321 data = NULL; 322 /* now try to get the reduce power table, if not loaded yet */ 323 if (!trans->reduce_power_loaded) { /* the condition is false */ 324 data = iwl_uefi_get_reduced_power(trans, &len); 325 if (IS_ERR_OR_NULL(data)) { 326 /* 327 * Pretend we've loaded it - at least we've tried and 328 * couldn't load it at all, so there's no point in 329 * trying again over and over. 330 */ 331 trans->reduce_power_loaded = true; 332 333 goto skip_reduce_power; 334 } 335 } 336 337 ret = iwl_trans_set_reduce_power(trans, data, len); /* len - ? */ 338 if (ret) 339 IWL_DEBUG_FW(trans, 340 "Failed to set reduce power table %d\n", 341 ret); 342 kfree(data); Am I missing something? Dmitry
On Wed, 2025-01-29 at 14:50 +0300, Dmitry Antipov wrote: > On 1/29/25 1:48 PM, Johannes Berg wrote: > > > I don't see that there's uninitialized use of 'len'. Maybe some static > > checkers aren't seeing through this, but the code is fine: > > > > If iwl_pnvm_get_from_fs() is successful, then 'len' is initialized. If > > it fails, we goto skip_parse. > > [... reordering too ...] > Am I missing something? You missed this: > > There, if trans->reduce_power_loaded is false, 'len' again is either > > initialized or we go to skip_reduce_power and never use 'len'. > > > > If trans->reduce_power_loaded is true, then we get to > > iwl_trans_pcie_ctx_info_gen3_set_reduce_power() which doesn't use 'len' > > in this case. johannes
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c b/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c index b6d3ac6ed440..ddf7acd67e94 100644 --- a/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c +++ b/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c @@ -332,6 +332,9 @@ int iwl_pnvm_load(struct iwl_trans *trans, goto skip_reduce_power; } + } else { + /* see iwl_uefi_get_reduced_power() to check why */ + len = IWL_HARDCODED_REDUCE_POWER_SIZE; } ret = iwl_trans_set_reduce_power(trans, data, len);
In 'iwl_pnvm_load()', assume that the power table size is always equal to IWL_HARDCODED_REDUCE_POWER_SIZE. Compile tested only. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- I would gently ask Johannes to review this before taking any other actions. This is intended for stable linux-6.1.y only in attempt to avoid possible use of an uninitialized 'len' without backporting https://lore.kernel.org/linux-wireless/20230606074310.889520-1-gregory.greenman@intel.com. --- drivers/net/wireless/intel/iwlwifi/fw/pnvm.c | 3 +++ 1 file changed, 3 insertions(+)