Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 3 Dec 2020 12:08:27 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     luciano.coelho@intel.com
Cc:     linux-wireless@vger.kernel.org
Subject: [bug report] iwlwifi: support REDUCE_TX_POWER_CMD version 6
Message-ID: <X8irC3FP0QDE9QFe@mwanda>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Precedence: bulk

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