This patch series fixes potential integer underflow and invalid memory access issues reported by Dan Carpenter.
Amitkumar Karwar (5):
mwifiex: replace u16 with __le16 in struct mwifiex_types_power_group
mwifiex: fix invalid memory access in mwifiex_get_power_level()
mwifiex: fix invalid memory access in mwifiex_ret_tx_rate_cfg()
mwifiex: fix invalid memory access in mwifiex_update_autoindex_ies()
mwifiex: improvements in mwifiex_ret_tx_power_cfg()
Dan Carpenter (1):
mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()
drivers/net/wireless/mwifiex/fw.h | 4 +-
drivers/net/wireless/mwifiex/ie.c | 11 ++--
drivers/net/wireless/mwifiex/sta_cmd.c | 4 +-
drivers/net/wireless/mwifiex/sta_cmdresp.c | 81 +++++++++++++++---------------
drivers/net/wireless/mwifiex/sta_ioctl.c | 5 +-
drivers/net/wireless/mwifiex/wmm.c | 3 ++
6 files changed, 58 insertions(+), 50 deletions(-)
--
1.8.2.3
From: Amitkumar Karwar <[email protected]>
As tlv_buf_len is decremented at the end of the loop, we may have
accessed invalid memory in the last iteration.
Modify the while condition and add a break statement at the
begining of the loop to fix the problem.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/sta_cmdresp.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c b/drivers/net/wireless/mwifiex/sta_cmdresp.c
index 5edea4d..2675ca7 100644
--- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
@@ -274,17 +274,20 @@ static int mwifiex_ret_tx_rate_cfg(struct mwifiex_private *priv,
struct host_cmd_ds_tx_rate_cfg *rate_cfg = &resp->params.tx_rate_cfg;
struct mwifiex_rate_scope *rate_scope;
struct mwifiex_ie_types_header *head;
- u16 tlv, tlv_buf_len;
+ u16 tlv, tlv_buf_len, tlv_buf_left;
u8 *tlv_buf;
u32 i;
- tlv_buf = ((u8 *)rate_cfg) +
- sizeof(struct host_cmd_ds_tx_rate_cfg);
- tlv_buf_len = le16_to_cpu(*(__le16 *) (tlv_buf + sizeof(u16)));
+ tlv_buf = ((u8 *)rate_cfg) + sizeof(struct host_cmd_ds_tx_rate_cfg);
+ tlv_buf_left = le16_to_cpu(resp->size) - S_DS_GEN - sizeof(*rate_cfg);
- while (tlv_buf && tlv_buf_len > 0) {
- tlv = (*tlv_buf);
- tlv = tlv | (*(tlv_buf + 1) << 8);
+ while (tlv_buf_left >= sizeof(*head)) {
+ head = (struct mwifiex_ie_types_header *)tlv_buf;
+ tlv = le16_to_cpu(head->type);
+ tlv_buf_len = le16_to_cpu(head->len);
+
+ if (tlv_buf_left < (sizeof(*head) + tlv_buf_len))
+ break;
switch (tlv) {
case TLV_TYPE_RATE_SCOPE:
@@ -304,9 +307,8 @@ static int mwifiex_ret_tx_rate_cfg(struct mwifiex_private *priv,
/* Add RATE_DROP tlv here */
}
- head = (struct mwifiex_ie_types_header *) tlv_buf;
- tlv_buf += le16_to_cpu(head->len) + sizeof(*head);
- tlv_buf_len -= le16_to_cpu(head->len);
+ tlv_buf += (sizeof(*head) + tlv_buf_len);
+ tlv_buf_left -= (sizeof(*head) + tlv_buf_len);
}
priv->is_data_rate_auto = mwifiex_is_rate_auto(priv);
--
1.8.2.3
From: Amitkumar Karwar <[email protected]>
While parsing TLVs, return failure if number of remaining bytes
are less than current tlv length. This avoids invalid memory
access.
Signed-off-by: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/ie.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/ie.c b/drivers/net/wireless/mwifiex/ie.c
index 220af4f..81ac001 100644
--- a/drivers/net/wireless/mwifiex/ie.c
+++ b/drivers/net/wireless/mwifiex/ie.c
@@ -82,7 +82,7 @@ mwifiex_update_autoindex_ies(struct mwifiex_private *priv,
struct mwifiex_ie_list *ie_list)
{
u16 travel_len, index, mask;
- s16 input_len;
+ s16 input_len, tlv_len;
struct mwifiex_ie *ie;
u8 *tmp;
@@ -91,11 +91,13 @@ mwifiex_update_autoindex_ies(struct mwifiex_private *priv,
ie_list->len = 0;
- while (input_len > 0) {
+ while (input_len >= sizeof(struct mwifiex_ie_types_header)) {
ie = (struct mwifiex_ie *)(((u8 *)ie_list) + travel_len);
- input_len -= le16_to_cpu(ie->ie_length) + MWIFIEX_IE_HDR_SIZE;
- travel_len += le16_to_cpu(ie->ie_length) + MWIFIEX_IE_HDR_SIZE;
+ tlv_len = le16_to_cpu(ie->ie_length);
+ travel_len += tlv_len + MWIFIEX_IE_HDR_SIZE;
+ if (input_len < tlv_len + MWIFIEX_IE_HDR_SIZE)
+ return -1;
index = le16_to_cpu(ie->ie_index);
mask = le16_to_cpu(ie->mgmt_subtype_mask);
@@ -132,6 +134,7 @@ mwifiex_update_autoindex_ies(struct mwifiex_private *priv,
le16_add_cpu(&ie_list->len,
le16_to_cpu(priv->mgmt_ie[index].ie_length) +
MWIFIEX_IE_HDR_SIZE);
+ input_len -= tlv_len + MWIFIEX_IE_HDR_SIZE;
}
if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_UAP)
--
1.8.2.3
From: Amitkumar Karwar <[email protected]>
1) Move common code out of switch case handling
2) Return from the function if number of bytes left in response buffer
are less than tlv size
3) Pass pg_tlv_hdr directly instead of txp_cfg to mwifiex_get_power_level()
Signed-off-by: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/sta_cmdresp.c | 35 ++++++++++++++----------------
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c b/drivers/net/wireless/mwifiex/sta_cmdresp.c
index 2675ca7..5511946 100644
--- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
@@ -338,8 +338,7 @@ static int mwifiex_get_power_level(struct mwifiex_private *priv, void *data_buf)
if (!data_buf)
return -1;
- pg_tlv_hdr = (struct mwifiex_types_power_group *)
- ((u8 *) data_buf + sizeof(struct host_cmd_ds_txpwr_cfg));
+ pg_tlv_hdr = (struct mwifiex_types_power_group *)((u8 *)data_buf);
pg = (struct mwifiex_power_group *)
((u8 *) pg_tlv_hdr + sizeof(struct mwifiex_types_power_group));
length = le16_to_cpu(pg_tlv_hdr->length);
@@ -383,19 +382,25 @@ static int mwifiex_ret_tx_power_cfg(struct mwifiex_private *priv,
struct mwifiex_types_power_group *pg_tlv_hdr;
struct mwifiex_power_group *pg;
u16 action = le16_to_cpu(txp_cfg->action);
+ u16 tlv_buf_left;
- switch (action) {
- case HostCmd_ACT_GEN_GET:
- pg_tlv_hdr = (struct mwifiex_types_power_group *)
- ((u8 *) txp_cfg +
- sizeof(struct host_cmd_ds_txpwr_cfg));
+ pg_tlv_hdr = (struct mwifiex_types_power_group *)
+ ((u8 *)txp_cfg +
+ sizeof(struct host_cmd_ds_txpwr_cfg));
- pg = (struct mwifiex_power_group *)
- ((u8 *) pg_tlv_hdr +
- sizeof(struct mwifiex_types_power_group));
+ pg = (struct mwifiex_power_group *)
+ ((u8 *)pg_tlv_hdr +
+ sizeof(struct mwifiex_types_power_group));
+ tlv_buf_left = le16_to_cpu(resp->size) - S_DS_GEN - sizeof(*txp_cfg);
+ if (tlv_buf_left <
+ le16_to_cpu(pg_tlv_hdr->length) + sizeof(*pg_tlv_hdr))
+ return 0;
+
+ switch (action) {
+ case HostCmd_ACT_GEN_GET:
if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING)
- mwifiex_get_power_level(priv, txp_cfg);
+ mwifiex_get_power_level(priv, pg_tlv_hdr);
priv->tx_power_level = (u16) pg->power_min;
break;
@@ -404,14 +409,6 @@ static int mwifiex_ret_tx_power_cfg(struct mwifiex_private *priv,
if (!le32_to_cpu(txp_cfg->mode))
break;
- pg_tlv_hdr = (struct mwifiex_types_power_group *)
- ((u8 *) txp_cfg +
- sizeof(struct host_cmd_ds_txpwr_cfg));
-
- pg = (struct mwifiex_power_group *)
- ((u8 *) pg_tlv_hdr +
- sizeof(struct mwifiex_types_power_group));
-
if (pg->power_max == pg->power_min)
priv->tx_power_level = (u16) pg->power_min;
break;
--
1.8.2.3
From: Dan Carpenter <[email protected]>
Before we loop for next iteration we adjust the buffer pointer and
"resp_len":
curr += (tlv_len + sizeof(tlv_hdr->header));
resp_len -= (tlv_len + sizeof(tlv_hdr->header));
If "resp_len" gets set to negative then it counts as a high positive
value.
Signed-off-by: Dan Carpenter <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/wmm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index 5dd0ccc..13eaeed 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -722,6 +722,9 @@ int mwifiex_ret_wmm_get_status(struct mwifiex_private *priv,
tlv_hdr = (struct mwifiex_ie_types_data *) curr;
tlv_len = le16_to_cpu(tlv_hdr->header.len);
+ if (resp_len < tlv_len + sizeof(tlv_hdr->header))
+ break;
+
switch (le16_to_cpu(tlv_hdr->header.type)) {
case TLV_TYPE_WMMQSTATUS:
tlv_wmm_qstatus =
--
1.8.2.3
From: Amitkumar Karwar <[email protected]>
__le16 to u16 conversion is missing for "pg_tlv_hdr->length"
in mwifiex_get_power_level(). This creates a problem on big
endian machines.
It is resolved by changing definition of the structure
and making required endianness changes.
Signed-off-by: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/fw.h | 4 ++--
drivers/net/wireless/mwifiex/sta_cmd.c | 4 ++--
drivers/net/wireless/mwifiex/sta_cmdresp.c | 4 ++--
drivers/net/wireless/mwifiex/sta_ioctl.c | 5 +++--
4 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/fw.h b/drivers/net/wireless/mwifiex/fw.h
index f80f30b..c8385ec 100644
--- a/drivers/net/wireless/mwifiex/fw.h
+++ b/drivers/net/wireless/mwifiex/fw.h
@@ -1020,8 +1020,8 @@ struct mwifiex_power_group {
} __packed;
struct mwifiex_types_power_group {
- u16 type;
- u16 length;
+ __le16 type;
+ __le16 length;
} __packed;
struct host_cmd_ds_txpwr_cfg {
diff --git a/drivers/net/wireless/mwifiex/sta_cmd.c b/drivers/net/wireless/mwifiex/sta_cmd.c
index 7d66018..2181ee2 100644
--- a/drivers/net/wireless/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/mwifiex/sta_cmd.c
@@ -239,14 +239,14 @@ static int mwifiex_cmd_tx_power_cfg(struct host_cmd_ds_command *cmd,
memmove(cmd_txp_cfg, txp,
sizeof(struct host_cmd_ds_txpwr_cfg) +
sizeof(struct mwifiex_types_power_group) +
- pg_tlv->length);
+ le16_to_cpu(pg_tlv->length));
pg_tlv = (struct mwifiex_types_power_group *) ((u8 *)
cmd_txp_cfg +
sizeof(struct host_cmd_ds_txpwr_cfg));
cmd->size = cpu_to_le16(le16_to_cpu(cmd->size) +
sizeof(struct mwifiex_types_power_group) +
- pg_tlv->length);
+ le16_to_cpu(pg_tlv->length));
} else {
memmove(cmd_txp_cfg, txp, sizeof(*txp));
}
diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c b/drivers/net/wireless/mwifiex/sta_cmdresp.c
index 58a6013..bdf50fd 100644
--- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
@@ -340,7 +340,7 @@ static int mwifiex_get_power_level(struct mwifiex_private *priv, void *data_buf)
((u8 *) data_buf + sizeof(struct host_cmd_ds_txpwr_cfg));
pg = (struct mwifiex_power_group *)
((u8 *) pg_tlv_hdr + sizeof(struct mwifiex_types_power_group));
- length = pg_tlv_hdr->length;
+ length = le16_to_cpu(pg_tlv_hdr->length);
if (length > 0) {
max_power = pg->power_max;
min_power = pg->power_min;
@@ -356,7 +356,7 @@ static int mwifiex_get_power_level(struct mwifiex_private *priv, void *data_buf)
length -= sizeof(struct mwifiex_power_group);
}
- if (pg_tlv_hdr->length > 0) {
+ if (le16_to_cpu(pg_tlv_hdr->length) > 0) {
priv->min_tx_power_level = (u8) min_power;
priv->max_tx_power_level = (u8) max_power;
}
diff --git a/drivers/net/wireless/mwifiex/sta_ioctl.c b/drivers/net/wireless/mwifiex/sta_ioctl.c
index f084412..c8e029d 100644
--- a/drivers/net/wireless/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/mwifiex/sta_ioctl.c
@@ -638,8 +638,9 @@ int mwifiex_set_tx_power(struct mwifiex_private *priv,
txp_cfg->mode = cpu_to_le32(1);
pg_tlv = (struct mwifiex_types_power_group *)
(buf + sizeof(struct host_cmd_ds_txpwr_cfg));
- pg_tlv->type = TLV_TYPE_POWER_GROUP;
- pg_tlv->length = 4 * sizeof(struct mwifiex_power_group);
+ pg_tlv->type = cpu_to_le16(TLV_TYPE_POWER_GROUP);
+ pg_tlv->length =
+ cpu_to_le16(4 * sizeof(struct mwifiex_power_group));
pg = (struct mwifiex_power_group *)
(buf + sizeof(struct host_cmd_ds_txpwr_cfg)
+ sizeof(struct mwifiex_types_power_group));
--
1.8.2.3
From: Amitkumar Karwar <[email protected]>
With "while (length)" check we may end up in accessing invalid
memory in last iteration.
This patch makes sure that tlv length is not less than the length
of structure mwifiex_power_group when min/max power is calculated.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/sta_cmdresp.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c b/drivers/net/wireless/mwifiex/sta_cmdresp.c
index bdf50fd..5edea4d 100644
--- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
@@ -341,12 +341,16 @@ static int mwifiex_get_power_level(struct mwifiex_private *priv, void *data_buf)
pg = (struct mwifiex_power_group *)
((u8 *) pg_tlv_hdr + sizeof(struct mwifiex_types_power_group));
length = le16_to_cpu(pg_tlv_hdr->length);
- if (length > 0) {
- max_power = pg->power_max;
- min_power = pg->power_min;
- length -= sizeof(struct mwifiex_power_group);
- }
- while (length) {
+
+ /* At least one structure required to update power */
+ if (length < sizeof(struct mwifiex_power_group))
+ return 0;
+
+ max_power = pg->power_max;
+ min_power = pg->power_min;
+ length -= sizeof(struct mwifiex_power_group);
+
+ while (length >= sizeof(struct mwifiex_power_group)) {
pg++;
if (max_power < pg->power_max)
max_power = pg->power_max;
@@ -356,10 +360,8 @@ static int mwifiex_get_power_level(struct mwifiex_private *priv, void *data_buf)
length -= sizeof(struct mwifiex_power_group);
}
- if (le16_to_cpu(pg_tlv_hdr->length) > 0) {
- priv->min_tx_power_level = (u8) min_power;
- priv->max_tx_power_level = (u8) max_power;
- }
+ priv->min_tx_power_level = (u8) min_power;
+ priv->max_tx_power_level = (u8) max_power;
return 0;
}
--
1.8.2.3