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;
MIME-Version: 1.0
References: <X8irC3FP0QDE9QFe@mwanda>
In-Reply-To: <X8irC3FP0QDE9QFe@mwanda>
From:   Brian Norris <briannorris@chromium.org>
Date:   Wed, 2 Jun 2021 14:12:31 -0700
Message-ID: <CA+ASDXMS-qR2tgFzE7_cqtxLj9-TavbmghqGo0jDtHkbUB59Ow@mail.gmail.com>
Subject: Re: [bug report] iwlwifi: support REDUCE_TX_POWER_CMD version 6
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        "Coelho, Luciano" <luciano.coelho@intel.com>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>,
        Johannes Berg <johannes@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

On Thu, Dec 3, 2020 at 1:10 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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)

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.

>    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?

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

>    463                          IWL_DEBUG_RADIO(fwrt, "    Band[%d] = %d * .125dBm\n",
>    464                                          j, prof->table[idx]);
>                                                    ^^^^^^^^^^^^^^^^
>    465                  }
>    466          }
>    468          return 0;
>    469  }
>
> regards,
> dan carpenter