iwlwifi: support REDUCE_TX_POWER_CMD version 6
Hello Luca Coelho,
The patch fbb7957d28ac: "iwlwifi: support REDUCE_TX_POWER_CMD version 6" from Sep 28, 2020, leads to the following static checker warning:
drivers/net/wireless/intel/iwlwifi/fw/acpi.c:462 iwl_sar_fill_table() error: buffer overflow 'prof->table' 10 <= 15
drivers/net/wireless/intel/iwlwifi/fw/acpi.c
422 static int iwl_sar_fill_table(struct iwl_fw_runtime *fwrt,
423 __le16 *per_chain, u32 n_subbands,
424 int prof_a, int prof_b)
Original n_subbands was ACPI_SAR_NUM_SUB_BANDS (5) but now it can be IWL_NUM_SUB_BANDS_V2 (11) as well.
425 {
426 int profs[ACPI_SAR_NUM_CHAIN_LIMITS] = { prof_a, prof_b };
427 int i, j, idx;
428
429 for (i = 0; i < ACPI_SAR_NUM_CHAIN_LIMITS; i++) {
430 struct iwl_sar_profile *prof;
431
432 /* don't allow SAR to be disabled (profile 0 means disable) */
433 if (profs[i] == 0)
434 return -EPERM;
435
436 /* we are off by one, so allow up to ACPI_SAR_PROFILE_NUM */
437 if (profs[i] > ACPI_SAR_PROFILE_NUM)
438 return -EINVAL;
439
440 /* profiles go from 1 to 4, so decrement to access the array */
441 prof = &fwrt->sar_profiles[profs[i] - 1];
442
443 /* if the profile is disabled, do nothing */
444 if (!prof->enabled) {
445 IWL_DEBUG_RADIO(fwrt, "SAR profile %d is disabled.\n",
446 profs[i]);
447 /*
448 * if one of the profiles is disabled, we
449 * ignore all of them and return 1 to
450 * differentiate disabled from other failures.
451 */
452 return 1;
453 }
454
455 IWL_DEBUG_INFO(fwrt,
456 "SAR EWRD: chain %d profile index %d\n",
457 i, profs[i]);
458 IWL_DEBUG_RADIO(fwrt, " Chain[%d]:\n", i);
459 for (j = 0; j < n_subbands; j++) {
460 idx = i * ACPI_SAR_NUM_SUB_BANDS + j;
461 per_chain[i * n_subbands + j] =
462 cpu_to_le16(prof->table[idx]);
^^^^^^^^^^^^^^^^But this table size wasn't increased so potentially we're reading beyond the end of the array?
463 IWL_DEBUG_RADIO(fwrt, " Band[%d] = %d * .125dBm\n",
464 j, prof->table[idx]);
^^^^^^^^^^^^^^^^ 465 }
466 }
468 return 0;
469 }
regards,
dan carpenter
Signal boost:
I've seen issues in this code in the past, as this is a custom format, the docs are non-public, it interacts with ACPI tables that Intel doesn't always get to review, and the parsing is all written in C... ...I also think Dan's static checker warning below is correct.
Luca, has this been addressed yet?
Quoted 17 lines · Dan Carpenter
Hello Luca Coelho,
The patch
fbb7957d28ac: "iwlwifi: supportREDUCE_TX_POWER_CMDversion 6" from Sep 28, 2020, leads to the following static checker warning:drivers/net/wireless/intel/iwlwifi/fw/acpi.c:462 iwl_sar_fill_table() error: buffer overflow 'prof->table' 10 <= 15
drivers/net/wireless/intel/iwlwifi/fw/acpi.c
422 static int iwl_sar_fill_table(structiwl_fw_runtime*fwrt,
423__le16*per_chain, u32n_subbands,
424 intprof_a, intprof_b)Original
n_subbandswasACPI_SAR_NUM_SUB_BANDS(5) but now it can beIWL_NUM_SUB_BANDS_V2(11) as well.
Quoted 13 lines
425 {
426 int profs[ACPI_SAR_NUM_CHAIN_LIMITS] = { prof_a, prof_b };
427 int i, j, idx;
428
429 for (i = 0; i < ACPI_SAR_NUM_CHAIN_LIMITS; i++) {
430 struct iwl_sar_profile *prof;
431
432 /* don't allow SAR to be disabled (profile 0 means disable) */
433 if (profs[i] == 0)
434 return -EPERM;
435
436 /* we are off by one, so allow up to ACPI_SAR_PROFILE_NUM */
437 if (profs[i] > ACPI_SAR_PROFILE_NUM)Side note: stuff like this would be more readable and hopefully less error prone if we used ARRAY_SIZE() instead of memorizing the array's size here. Similar up at line 429, but at least that array is defined ~3 lines away.
Quoted 26 lines
438 return -EINVAL;
439
440 /* profiles go from 1 to 4, so decrement to access the array */
441 prof = &fwrt->sar_profiles[profs[i] - 1];
442
443 /* if the profile is disabled, do nothing */
444 if (!prof->enabled) {
445 IWL_DEBUG_RADIO(fwrt, "SAR profile %d is disabled.\n",
446 profs[i]);
447 /*
448 * if one of the profiles is disabled, we
449 * ignore all of them and return 1 to
450 * differentiate disabled from other failures.
451 */
452 return 1;
453 }
454
455 IWL_DEBUG_INFO(fwrt,
456 "SAR EWRD: chain %d profile index %d\n",
457 i, profs[i]);
458 IWL_DEBUG_RADIO(fwrt, " Chain[%d]:\n", i);
459 for (j = 0; j < n_subbands; j++) {
460 idx = i * ACPI_SAR_NUM_SUB_BANDS + j;
461 per_chain[i * n_subbands + j] =
462 cpu_to_le16(prof->table[idx]);
^^^^^^^^^^^^^^^^Quoted 2 lines
But this table size wasn't increased so potentially we're reading beyond the end of the array?
I believe your warning is pointing out a real issue, and I think (?) it's safe to just bump the table size, but given all the not-quite-explicit relationships between various bits of the ACPI table, the IWL command definition, and the various macros, it's not obvious that this is the only necessary change.
Brian
Quoted 10 lines
463 IWL_DEBUG_RADIO(fwrt, " Band[%d] = %d * .125dBm\n", 464 j, prof->table[idx]); ^^^^^^^^^^^^^^^^ 465 } 466 } 468 return 0; 469 }regards,
dan carpenter
Quoted 7 lines · Brian Norris
Signal boost:
I've seen issues in this code in the past, as this is a custom format, the docs are non-public, it interacts with ACPI tables that Intel doesn't always get to review, and the parsing is all written in C... ...I also think Dan's static checker warning below is correct.Luca, has this been addressed yet?
You're right, the issue that Dan pointed out is indeed an issue. I'll send a fix as explained in my previous email.
--
Cheers,
Luca.
Quoted 43 lines · Dan Carpenter
Hello Luca Coelho,
The patch
fbb7957d28ac: "iwlwifi: supportREDUCE_TX_POWER_CMDversion 6" from Sep 28, 2020, leads to the following static checker warning:drivers/net/wireless/intel/iwlwifi/fw/acpi.c:462 iwl_sar_fill_table() error: buffer overflow 'prof->table' 10 <= 15
drivers/net/wireless/intel/iwlwifi/fw/acpi.c
422 static int iwl_sar_fill_table(structiwl_fw_runtime*fwrt,
423__le16*per_chain, u32n_subbands,
424 intprof_a, intprof_b)Original
n_subbandswasACPI_SAR_NUM_SUB_BANDS(5) but now it can beIWL_NUM_SUB_BANDS_V2(11) as well.425 {
426 int profs[ACPI_SAR_NUM_CHAIN_LIMITS] = {prof_a,prof_b};
427 int i, j, idx;
428
429 for (i = 0; i <ACPI_SAR_NUM_CHAIN_LIMITS; i++) {
430 structiwl_sar_profile*prof;
431
432 /* don't allow SAR to be disabled (profile 0 means disable) */
433 if (profs[i] == 0)
434 return-EPERM;
435
436 /* we are off by one, so allow up toACPI_SAR_PROFILE_NUM*/
437 if (profs[i] >ACPI_SAR_PROFILE_NUM)
438 return-EINVAL;
439
440 /* profiles go from 1 to 4, so decrement to access the array */
441 prof = &fwrt->sar_profiles[profs[i] - 1];
442
443 /* if the profile is disabled, do nothing */
444 if (!prof->enabled) {
445 IWL_DEBUG_RADIO(fwrt, "SAR profile %d is disabled.\n",
446 profs[i]);
447 /*448 * if one of the profiles is disabled, we 449 * ignore all of them and return 1 to 450 * differentiate disabled from other failures. 451 */
Quoted 14 lines
452 return 1;
453 }
454
455 IWL_DEBUG_INFO(fwrt,
456 "SAR EWRD: chain %d profile index %d\n",
457 i, profs[i]);
458IWL_DEBUG_RADIO(fwrt, " Chain[%d]:\n", i);
459 for (j = 0; j <n_subbands; j++) {
460 idx = i *ACPI_SAR_NUM_SUB_BANDS+ j;
461 per_chain[i *n_subbands+ j] =
462cpu_to_le16(prof->table[idx]);
^^^^^^^^^^^^^^^^ But this table size wasn't increased so potentially we're reading beyond the end of the array?
This is a good catch! The intention of this patch is to support the larger structures in the FW, but not from ACPI. So the solution for this here, would be to check if j is larger than ACPI_SAR_NUM_SUB_BANDS (which is the max ACPI table we support) and just fill in with zeros in that case.
This code has already been refactored in our internal tree (and will go upstream soon) to support the full size table from ACPI as well. But since the changes are not only fixing this bug, I'll send a separate bugfix for the mainline for now.
Thanks, Dan!
--
Cheers,
Luca.