From 18f9bb8e6f6795ba2638c381909ab22c2a4ede6d Mon Sep 17 00:00:00 2001 From: Nevo Hed Date: Thu, 29 Oct 2020 01:16:44 -0400 Subject: [PATCH 1/4] Harden parsing of the txpwrlmt_cfg.conf file The helper function mwl_fwcmd_parse_txpwrlmt_cfg could run into several errors reading input form essentially a text hexdump and converting it to binary data to send to the firmware. Adding some sanity checks to guard against: - Only the first char of a (2 char) hex byte was tested, second char was wrongfully assumed to be hex. - Unrecognized characters were silently skipped. So human error leaving a `O` instead of a `0` would have dire consequences. Prefer to abort the whole file processing. - Being able to convert less than the requested number of bytes should be a total failure. Added aborts in mwl_fwcmd_set_txpwrlmt_cfg_data if any of the invocations of the helper function above failed. Note that this allows some (lower ID) subbands to be sent to firmware while later subbands woul be skipped. --- hif/fwcmd.c | 51 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/hif/fwcmd.c b/hif/fwcmd.c index 62febdc9..0f3be70e 100644 --- a/hif/fwcmd.c +++ b/hif/fwcmd.c @@ -915,16 +915,23 @@ static u16 mwl_fwcmd_parse_txpwrlmt_cfg(const u8 *src, size_t len, continue; } - if (isxdigit(*ptr)) { + if (isxdigit(ptr[0]) && isxdigit(ptr[1])) { byte_str[0] = *ptr++; byte_str[1] = *ptr++; kstrtol(byte_str, 16, &res); *dptr++ = res; } else { - ptr++; + // any failure to parse binary data in the hex + // format is complete failure. + return -1; } } + // Not able to parse the requested bytes - complete failure. + if((dptr - dst) != parse_len){ + return -1; + } + return (ptr - src); } @@ -3699,7 +3706,8 @@ int mwl_fwcmd_set_txpwrlmt_cfg_data(struct ieee80211_hw *hw) struct mwl_priv *priv = hw->priv; struct hostcmd_cmd_txpwrlmt_cfg *pcmd; struct mwl_txpwrlmt_cfg_entry_hdr hdr; - u16 id, parsed_len, size; + u16 id, size; + s16 parsed_len; __le32 txpwr_cfg_sig; u8 version[TXPWRLMT_CFG_VERSION_INFO_LEN]; const u8 *ptr; @@ -3714,10 +3722,9 @@ int mwl_fwcmd_set_txpwrlmt_cfg_data(struct ieee80211_hw *hw) parsed_len = mwl_fwcmd_parse_txpwrlmt_cfg(ptr, size, TXPWRLMT_CFG_SIG_LEN, (u8 *)&txpwr_cfg_sig); - ptr += parsed_len; - size -= parsed_len; - if (le32_to_cpu(txpwr_cfg_sig) != TXPWRLMT_CFG_SIGNATURE) { + if (parsed_len < 0 || + le32_to_cpu(txpwr_cfg_sig) != TXPWRLMT_CFG_SIGNATURE) { wiphy_err(hw->wiphy, "txpwrlmt config signature mismatch\n"); release_firmware(priv->txpwrlmt_file); @@ -3725,10 +3732,22 @@ int mwl_fwcmd_set_txpwrlmt_cfg_data(struct ieee80211_hw *hw) return 0; } + ptr += parsed_len; + size -= parsed_len; + /* Parsing TxPwrLmit Conf file Version */ parsed_len = mwl_fwcmd_parse_txpwrlmt_cfg(ptr, size, TXPWRLMT_CFG_VERSION_INFO_LEN, version); + + if (parsed_len < 0) { + wiphy_err(hw->wiphy, + "txpwrlmt config failed to read version\n"); + release_firmware(priv->txpwrlmt_file); + priv->txpwrlmt_file = NULL; + return 0; + } + ptr += parsed_len; size -= parsed_len; @@ -3740,6 +3759,16 @@ int mwl_fwcmd_set_txpwrlmt_cfg_data(struct ieee80211_hw *hw) parsed_len = mwl_fwcmd_parse_txpwrlmt_cfg(ptr, size, parsed_len, (u8 *)&hdr); + + if (parsed_len < 0) { + wiphy_err(hw->wiphy, + "txpwrlmt config failed to parse " + "subband header\n"); + release_firmware(priv->txpwrlmt_file); + priv->txpwrlmt_file = NULL; + return 0; + } + ptr += parsed_len; size -= parsed_len; data_len = le16_to_cpu(hdr.len) - @@ -3758,6 +3787,16 @@ int mwl_fwcmd_set_txpwrlmt_cfg_data(struct ieee80211_hw *hw) /* Parsing tx pwr cfg subband header info */ parsed_len = mwl_fwcmd_parse_txpwrlmt_cfg(ptr, size, data_len, pcmd->data); + + if (parsed_len < 0) { + wiphy_err(hw->wiphy, + "txpwrlmt config failed to parse subband data\n"); + release_firmware(priv->txpwrlmt_file); + priv->txpwrlmt_file = NULL; + mutex_unlock(&priv->fwcmd_mutex); + return 0; + } + ptr += parsed_len; size -= parsed_len; From b8a37ef32b392cce4a264c4f7c5e074dca49d975 Mon Sep 17 00:00:00 2001 From: Nevo Hed Date: Thu, 29 Oct 2020 01:32:46 -0400 Subject: [PATCH 2/4] Probably should not release mutex till after copy from shared memory --- hif/fwcmd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hif/fwcmd.c b/hif/fwcmd.c index 0f3be70e..d3e7e968 100644 --- a/hif/fwcmd.c +++ b/hif/fwcmd.c @@ -3849,8 +3849,6 @@ int mwl_fwcmd_get_txpwrlmt_cfg_data(struct ieee80211_hw *hw) return -EIO; } - mutex_unlock(&priv->fwcmd_mutex); - subband_len = le16_to_cpu(pcmd->cmd_hdr.len) - sizeof(struct hostcmd_header) - 2; if (total_len <= SYSADPT_TXPWRLMT_CFG_BUF_SIZE) { @@ -3866,6 +3864,8 @@ int mwl_fwcmd_get_txpwrlmt_cfg_data(struct ieee80211_hw *hw) wiphy_err(hw->wiphy, "TxPwrLmt cfg buf size is not enough\n"); } + + mutex_unlock(&priv->fwcmd_mutex); } return 0; From 0b5553ea668a794918f9bffbef718ee35be40971 Mon Sep 17 00:00:00 2001 From: Nevo Hed Date: Thu, 29 Oct 2020 01:58:16 -0400 Subject: [PATCH 3/4] Add /sys/kernel/debug/ieee80211/phyX/mwlwifi/txpwrlmt_file This presents the same data as mwlwifi/txpwrlmt but it is formatted as hex for consumption as the firmware file mwlwifi/txpwrlmt_cfg.conf. With this you can: 1. Save the current eeprom stored values when mwlwifi/txpwrlmt_cfg.conf is not present for manual editing (to lower values). 2. Compare (diff) against mwlwifi/txpwrlmt_cfg.conf (when it is present) to confirm that it is properly processed. --- debugfs.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/debugfs.c b/debugfs.c index 156662d4..c1e6688a 100644 --- a/debugfs.c +++ b/debugfs.c @@ -761,6 +761,76 @@ static ssize_t mwl_debugfs_txpwrlmt_read(struct file *file, priv->txpwrlmt_data.len); } +static ssize_t mwl_debugfs_txpwrlmt_file_read(struct file *file, + char __user *ubuf, + size_t count, loff_t *ppos) +{ + struct mwl_priv *priv = (struct mwl_priv *)file->private_data; + + const u32 txpwr_cfg_sig = cpu_to_le32(TXPWRLMT_CFG_SIGNATURE); + const u32 txpwr_cfg_ver = 0; + // The binary bytes take 2 chars and one space/nl plus some + // for the header. + int size = (SYSADPT_TXPWRLMT_CFG_BUF_SIZE * 3) + 40; + char *buf = kzalloc(size, GFP_KERNEL); + char *p = buf; + int len = 0; + int remain = priv->txpwrlmt_data.len; + const char *tblp = priv->txpwrlmt_data.buf; + + ssize_t ret = 0; + + if (!p) + return -ENOMEM; + + len += scnprintf(p + len, size - len, "%*ph\n", + TXPWRLMT_CFG_SIG_LEN, &txpwr_cfg_sig); + len += scnprintf(p + len, size - len, "%*ph\n", + TXPWRLMT_CFG_VERSION_INFO_LEN, &txpwr_cfg_ver); + + while (remain > sizeof(struct mwl_txpwrlmt_cfg_entry_hdr)) { + struct mwl_txpwrlmt_cfg_entry_hdr* subband_hdr = + (struct mwl_txpwrlmt_cfg_entry_hdr*)tblp; + u16 subband_len = le16_to_cpu(subband_hdr->len); + len += scnprintf(p + len, size - len, "\n%*ph\n", + sizeof(struct mwl_txpwrlmt_cfg_entry_hdr), + tblp); + tblp += sizeof(struct mwl_txpwrlmt_cfg_entry_hdr); + remain -= sizeof(struct mwl_txpwrlmt_cfg_entry_hdr); + + // TODO(nhed): maybe add more sanity checks? + while (subband_len > 0) { + u16 hexd_line_cnt = min(subband_len, (u16)32); + len += scnprintf(p + len, size - len, "%*ph\n", + hexd_line_cnt, tblp); + tblp += hexd_line_cnt; + remain -= hexd_line_cnt; + subband_len -= hexd_line_cnt; + } + + // Not sure why `\n` delimiters are added between subbands + // into the binary data buffer txpwrlmt_data.buf by + // mwl_fwcmd_get_txpwrlmt_cfg_data() but since we need to skip + // them we may as well test for them. + if (*tblp != '\n') { + printk("Unexpected lack of subband delimiter(s)\n"); + ret = -EIO; + goto cleanup; + } + tblp++; + remain--; + } + len += scnprintf(p + len, size - len, "\n"); + ret = simple_read_from_buffer(ubuf, count, ppos, p, len); + +cleanup: + if (buf) { + kfree(buf); + } + + return ret; +} + static ssize_t mwl_debugfs_tx_amsdu_read(struct file *file, char __user *ubuf, size_t count, loff_t *ppos) @@ -2139,6 +2209,7 @@ MWLWIFI_DEBUGFS_FILE_READ_OPS(ampdu); MWLWIFI_DEBUGFS_FILE_READ_OPS(stnid); MWLWIFI_DEBUGFS_FILE_READ_OPS(device_pwrtbl); MWLWIFI_DEBUGFS_FILE_READ_OPS(txpwrlmt); +MWLWIFI_DEBUGFS_FILE_READ_OPS(txpwrlmt_file); MWLWIFI_DEBUGFS_FILE_OPS(tx_amsdu); MWLWIFI_DEBUGFS_FILE_OPS(dump_hostcmd); MWLWIFI_DEBUGFS_FILE_OPS(dump_probe); @@ -2177,6 +2248,7 @@ void mwl_debugfs_init(struct ieee80211_hw *hw) MWLWIFI_DEBUGFS_ADD_FILE(stnid); MWLWIFI_DEBUGFS_ADD_FILE(device_pwrtbl); MWLWIFI_DEBUGFS_ADD_FILE(txpwrlmt); + MWLWIFI_DEBUGFS_ADD_FILE(txpwrlmt_file); MWLWIFI_DEBUGFS_ADD_FILE(tx_amsdu); MWLWIFI_DEBUGFS_ADD_FILE(dump_hostcmd); MWLWIFI_DEBUGFS_ADD_FILE(dump_probe); From e215a73e91e65585d83189a0bbbed87333da6c87 Mon Sep 17 00:00:00 2001 From: Nevo Hed Date: Thu, 29 Oct 2020 02:09:50 -0400 Subject: [PATCH 4/4] Fix undersized txpwrlmt message to firmware The message includes not only the subband data but also the subband header so reducing the message by the header size caused some data read from file to trashed on way to firmware. With this mod can read back same subband data as is present in the txpwrlmt_cfg.conf file. --- hif/fwcmd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hif/fwcmd.c b/hif/fwcmd.c index d3e7e968..4c31155c 100644 --- a/hif/fwcmd.c +++ b/hif/fwcmd.c @@ -3771,8 +3771,7 @@ int mwl_fwcmd_set_txpwrlmt_cfg_data(struct ieee80211_hw *hw) ptr += parsed_len; size -= parsed_len; - data_len = le16_to_cpu(hdr.len) - - sizeof(struct mwl_txpwrlmt_cfg_entry_hdr); + data_len = le16_to_cpu(hdr.len); pcmd = (struct hostcmd_cmd_txpwrlmt_cfg *)&priv->pcmd_buf[0];