* [PATCH 1/6] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()
2013-10-22 22:24 [PATCH 0/6] mwifiex: potential integer underflow and invalid mem access Bing Zhao
@ 2013-10-22 22:24 ` Bing Zhao
2013-10-22 22:24 ` [PATCH 2/6] mwifiex: replace u16 with __le16 in struct mwifiex_types_power_group Bing Zhao
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bing Zhao @ 2013-10-22 22:24 UTC (permalink / raw)
To: linux-wireless
Cc: John W. Linville, Dan Carpenter, Amitkumar Karwar, Avinash Patil,
Nishant Sarmukadam, Frank Huang, Bing Zhao
From: Dan Carpenter <dan.carpenter@oracle.com>
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 <dan.carpenter@oracle.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/6] mwifiex: replace u16 with __le16 in struct mwifiex_types_power_group
2013-10-22 22:24 [PATCH 0/6] mwifiex: potential integer underflow and invalid mem access Bing Zhao
2013-10-22 22:24 ` [PATCH 1/6] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status() Bing Zhao
@ 2013-10-22 22:24 ` Bing Zhao
2013-10-22 22:24 ` [PATCH 3/6] mwifiex: fix invalid memory access in mwifiex_get_power_level() Bing Zhao
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bing Zhao @ 2013-10-22 22:24 UTC (permalink / raw)
To: linux-wireless
Cc: John W. Linville, Dan Carpenter, Amitkumar Karwar, Avinash Patil,
Nishant Sarmukadam, Frank Huang, Bing Zhao
From: Amitkumar Karwar <akarwar@marvell.com>
__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 <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/6] mwifiex: fix invalid memory access in mwifiex_get_power_level()
2013-10-22 22:24 [PATCH 0/6] mwifiex: potential integer underflow and invalid mem access Bing Zhao
2013-10-22 22:24 ` [PATCH 1/6] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status() Bing Zhao
2013-10-22 22:24 ` [PATCH 2/6] mwifiex: replace u16 with __le16 in struct mwifiex_types_power_group Bing Zhao
@ 2013-10-22 22:24 ` Bing Zhao
2013-10-22 22:24 ` [PATCH 4/6] mwifiex: fix invalid memory access in mwifiex_ret_tx_rate_cfg() Bing Zhao
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bing Zhao @ 2013-10-22 22:24 UTC (permalink / raw)
To: linux-wireless
Cc: John W. Linville, Dan Carpenter, Amitkumar Karwar, Avinash Patil,
Nishant Sarmukadam, Frank Huang, Bing Zhao
From: Amitkumar Karwar <akarwar@marvell.com>
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 <dan.carpenter@oracle.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/6] mwifiex: fix invalid memory access in mwifiex_ret_tx_rate_cfg()
2013-10-22 22:24 [PATCH 0/6] mwifiex: potential integer underflow and invalid mem access Bing Zhao
` (2 preceding siblings ...)
2013-10-22 22:24 ` [PATCH 3/6] mwifiex: fix invalid memory access in mwifiex_get_power_level() Bing Zhao
@ 2013-10-22 22:24 ` Bing Zhao
2013-10-22 22:24 ` [PATCH 5/6] mwifiex: fix invalid memory access in mwifiex_update_autoindex_ies() Bing Zhao
2013-10-22 22:24 ` [PATCH 6/6] mwifiex: improvements in mwifiex_ret_tx_power_cfg() Bing Zhao
5 siblings, 0 replies; 7+ messages in thread
From: Bing Zhao @ 2013-10-22 22:24 UTC (permalink / raw)
To: linux-wireless
Cc: John W. Linville, Dan Carpenter, Amitkumar Karwar, Avinash Patil,
Nishant Sarmukadam, Frank Huang, Bing Zhao
From: Amitkumar Karwar <akarwar@marvell.com>
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 <dan.carpenter@oracle.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 5/6] mwifiex: fix invalid memory access in mwifiex_update_autoindex_ies()
2013-10-22 22:24 [PATCH 0/6] mwifiex: potential integer underflow and invalid mem access Bing Zhao
` (3 preceding siblings ...)
2013-10-22 22:24 ` [PATCH 4/6] mwifiex: fix invalid memory access in mwifiex_ret_tx_rate_cfg() Bing Zhao
@ 2013-10-22 22:24 ` Bing Zhao
2013-10-22 22:24 ` [PATCH 6/6] mwifiex: improvements in mwifiex_ret_tx_power_cfg() Bing Zhao
5 siblings, 0 replies; 7+ messages in thread
From: Bing Zhao @ 2013-10-22 22:24 UTC (permalink / raw)
To: linux-wireless
Cc: John W. Linville, Dan Carpenter, Amitkumar Karwar, Avinash Patil,
Nishant Sarmukadam, Frank Huang, Bing Zhao
From: Amitkumar Karwar <akarwar@marvell.com>
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 <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 6/6] mwifiex: improvements in mwifiex_ret_tx_power_cfg()
2013-10-22 22:24 [PATCH 0/6] mwifiex: potential integer underflow and invalid mem access Bing Zhao
` (4 preceding siblings ...)
2013-10-22 22:24 ` [PATCH 5/6] mwifiex: fix invalid memory access in mwifiex_update_autoindex_ies() Bing Zhao
@ 2013-10-22 22:24 ` Bing Zhao
5 siblings, 0 replies; 7+ messages in thread
From: Bing Zhao @ 2013-10-22 22:24 UTC (permalink / raw)
To: linux-wireless
Cc: John W. Linville, Dan Carpenter, Amitkumar Karwar, Avinash Patil,
Nishant Sarmukadam, Frank Huang, Bing Zhao
From: Amitkumar Karwar <akarwar@marvell.com>
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 <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread