[patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Dan Carpenter]

If "resp_len" gets set to negative then it counts as a high positive
value.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I spotted this reviewing the int => bool changes, but I don't have the
hardware and can't test it.

diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index 2e8f9cd..3c6ee3a 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -772,6 +772,8 @@ int mwifiex_ret_wmm_get_status(struct mwifiex_private *priv,
 			break;
 		}
 
+		if (resp_len < tlv_len + sizeof(tlv_hdr->header))
+			break;
 		curr += (tlv_len + sizeof(tlv_hdr->header));
 		resp_len -= (tlv_len + sizeof(tlv_hdr->header));
 	}

RE: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Bing Zhao]

Hi Dan,

> If "resp_len" gets set to negative then it counts as a high positive value.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I spotted this reviewing the int => bool changes, but I don't have the
> hardware and can't test it.

Thanks for spotting this potential integer underflow problem.

I think we can change the 'resp_len' variable type to a signed integer to fix this issue.

Thanks,
Bing

> 
> diff --git a/drivers/net/wireless/mwifiex/wmm.c
> b/drivers/net/wireless/mwifiex/wmm.c
> index 2e8f9cd..3c6ee3a 100644
> --- a/drivers/net/wireless/mwifiex/wmm.c
> +++ b/drivers/net/wireless/mwifiex/wmm.c
> @@ -772,6 +772,8 @@ int mwifiex_ret_wmm_get_status(struct
> mwifiex_private *priv,
>  			break;
>  		}
> 
> +		if (resp_len < tlv_len + sizeof(tlv_hdr->header))
> +			break;
>  		curr += (tlv_len + sizeof(tlv_hdr->header));
>  		resp_len -= (tlv_len + sizeof(tlv_hdr->header));
>  	}

Re: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Dan Carpenter]

On Wed, Sep 25, 2013 at 09:25:46AM -0700, Bing Zhao wrote:
> Hi Dan,
> 
> > If "resp_len" gets set to negative then it counts as a high positive value.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > I spotted this reviewing the int => bool changes, but I don't have the
> > hardware and can't test it.
> 
> Thanks for spotting this potential integer underflow problem.
> 
> I think we can change the 'resp_len' variable type to a signed integer
> to fix this issue.

No, that doesn't work because the comparison against sizeof() get's
promoted to size_t.  In other words, negative values still count as
large positive values.

regards,
dan carpenter

RE: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Bing Zhao]

Hi Dan,

Thanks for your comments.

> > I think we can change the 'resp_len' variable type to a signed integer
> > to fix this issue.
> 
> No, that doesn't work because the comparison against sizeof() get's
> promoted to size_t.  In other words, negative values still count as
> large positive values.

You are right. The negative value counts as a large positive number while comparing against sizeof().
I can add a "const int hdr_size" variable to store the value of sizeof(...) and compare resp_len to hdr_size. The "sizeof(...)" has been used multiple times in this function, so I think it's worth adding a variable for it.


diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index 95fa359..c97df5a 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -707,10 +707,10 @@ int mwifiex_ret_wmm_get_status(struct mwifiex_private *priv,
                               const struct host_cmd_ds_command *resp)
 {
        u8 *curr = (u8 *) &resp->params.get_wmm_status;
-       uint16_t resp_len = le16_to_cpu(resp->size), tlv_len;
+       int resp_len = le16_to_cpu(resp->size), tlv_len;
        int valid = true;
-
        struct mwifiex_ie_types_data *tlv_hdr;
+       const int hdr_size = sizeof(tlv_hdr->header);
        struct mwifiex_ie_types_wmm_queue_status *tlv_wmm_qstatus;
        struct ieee_types_wmm_parameter *wmm_param_ie = NULL;
        struct mwifiex_wmm_ac_status *ac_status;
@@ -718,7 +718,7 @@ int mwifiex_ret_wmm_get_status(struct mwifiex_private *priv,
        dev_dbg(priv->adapter->dev, "info: WMM: WMM_GET_STATUS cmdresp received: %d\n",
                resp_len);

-       while ((resp_len >= sizeof(tlv_hdr->header)) && valid) {
+       while (resp_len >= hdr_size && valid) {
                tlv_hdr = (struct mwifiex_ie_types_data *) curr;
                tlv_len = le16_to_cpu(tlv_hdr->header.len);

@@ -772,8 +772,8 @@ int mwifiex_ret_wmm_get_status(struct mwifiex_private *priv,
                        break;
                }

-               curr += (tlv_len + sizeof(tlv_hdr->header));
-               resp_len -= (tlv_len + sizeof(tlv_hdr->header));
+               curr += tlv_len + hdr_size;
+               resp_len -= tlv_len + hdr_size;
        }

        mwifiex_wmm_setup_queue_priorities(priv, wmm_param_ie);


Thanks,
Bing

RE: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Bing Zhao]

Hi Dan,

> > No, that doesn't work because the comparison against sizeof() get's
> > promoted to size_t.  In other words, negative values still count as
> > large positive values.
> 
> You are right. The negative value counts as a large positive number while comparing against sizeof().
> I can add a "const int hdr_size" variable to store the value of sizeof(...) and compare resp_len to
> hdr_size. The "sizeof(...)" has been used multiple times in this function, so I think it's worth
> adding a variable for it.

Well, there is another problem here.
We might have accessed invalid memory while handling switch case in last iteration, because we didn't count tlv_len in "while(...)" condition check.

To fix this, Dan's change makes sense. But we should check the length before accessing the buffer.

@@ -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 =

Thanks,
Bing

Re: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Dan Carpenter]

There are several other similar functions as well.  For example, the
while loops in mwifiex_ret_tx_rate_cfg() and mwifiex_get_power_level().

regards,
dan carpenter

RE: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Bing Zhao]

Hi Dan,

> There are several other similar functions as well.  For example, the
> while loops in mwifiex_ret_tx_rate_cfg() and mwifiex_get_power_level().

Thanks for pointing it out. We will have them fixed.

Regards,
Bing

Re: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[John W. Linville]

On Thu, Sep 26, 2013 at 02:57:21PM -0700, Bing Zhao wrote:
> Hi Dan,
> 
> > There are several other similar functions as well.  For example, the
> > while loops in mwifiex_ret_tx_rate_cfg() and mwifiex_get_power_level().
> 
> Thanks for pointing it out. We will have them fixed.
> 
> Regards,
> Bing

OK, I'm confused by this thread...I'm dropping it.

Dan & Bing,

Please repost as a series of patches that I don't have to piece
together from a series of replies...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

RE: [patch] mwifiex: potential integer underflow in mwifiex_ret_wmm_get_status()

[Bing Zhao]

Hi John,

> OK, I'm confused by this thread...I'm dropping it.

Yes, please drop it.

> 
> Dan & Bing,
> 
> Please repost as a series of patches that I don't have to piece
> together from a series of replies...

I'm working on some more patches that fix the similar issue.
I will repost Dan's patch along with these patches in a series.

Thanks,
Bing