2012-09-20 06:21:33

by Raja Mani

Subject: [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair

From: Raja Mani <[email protected]>

Return value of ath6kl_get_regpair() is stored in 'regpair' in
ath6kl_wmi_regdomain_event() func and it's directly accessed
in the debug prints without checking for NULL value. There are
situation to get NULL pointer as a return value from
ath6kl_get_regpair() func. Fix this.

Found this on code review.

Signed-off-by: Raja Mani <[email protected]>
---
drivers/net/wireless/ath/ath6kl/wmi.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index 68b46bd..d5263ff 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -936,8 +936,9 @@ static void ath6kl_wmi_regdomain_event(struct wmi *wmi, u8 *datap, int len)

regpair = ath6kl_get_regpair((u16) reg_code);
country = ath6kl_regd_find_country_by_rd((u16) reg_code);
- ath6kl_dbg(ATH6KL_DBG_WMI, "Regpair used: 0x%0x\n",
- regpair->regDmnEnum);
+ if (regpair)
+ ath6kl_dbg(ATH6KL_DBG_WMI, "Regpair used: 0x%0x\n",
+ regpair->regDmnEnum);
}

if (country && wmi->parent_dev->wiphy_registered) {
--
1.7.1

2012-09-20 06:21:40

by Raja Mani

Subject: [PATCH 2/3] ath6kl: Check for valid rate table index

From: Raja Mani <[email protected]>

There are 28 items defined in rate table array 'wmi_rate_tbl'.
The rate table index (reply->rate_index) in ath6kl_wmi_bitrate_reply_rx()
func is not checked for the valid max limit index before accessing
rate table array. There may be some incidents to get memory crashes
without safe max check. Fix this.

Found this on code review.

Signed-off-by: Raja Mani <[email protected]>
---
drivers/net/wireless/ath/ath6kl/wmi.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index d5263ff..666f56d 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -1171,6 +1171,9 @@ static int ath6kl_wmi_bitrate_reply_rx(struct wmi *wmi, u8 *datap, int len)
rate = RATE_AUTO;
} else {
index = reply->rate_index & 0x7f;
+ if (index > (RATE_MCS_7_40 + 1))
+ return -EINVAL;
+
sgi = (reply->rate_index & 0x80) ? 1 : 0;
rate = wmi_rate_tbl[index][sgi];
}
--
1.7.1

2012-09-20 06:22:07

by Raja Mani

Subject: [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()

From: Raja Mani <[email protected]>

It's safe to check endpoint id values before it get
really used. Found this on code review.

Signed-off-by: Raja Mani <[email protected]>
---
drivers/net/wireless/ath/ath6kl/txrx.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index e867193..4ea6559 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -294,6 +294,12 @@ int ath6kl_control_tx(void *devt, struct sk_buff *skb,
return -EACCES;
}

+ if (WARN_ON_ONCE(eid == ENDPOINT_UNUSED ||
+ eid >= ENDPOINT_MAX)) {
+ status = -EINVAL;
+ goto fail_ctrl_tx;
+ }
+
spin_lock_bh(&ar->lock);

ath6kl_dbg(ATH6KL_DBG_WLAN_TX,
--
1.7.1

2012-09-21 09:35:26

by Raja Mani

Subject: Re: [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()

On Friday 21 September 2012 02:48 PM, Kalle Valo wrote:
> On 09/20/2012 09:21 AM, [email protected] wrote:
>> From: Raja Mani<[email protected]>
>>
>> It's safe to check endpoint id values before it get
>> really used. Found this on code review.
>>
>> Signed-off-by: Raja Mani<[email protected]>
>> ---
>> drivers/net/wireless/ath/ath6kl/txrx.c | 6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
>> index e867193..4ea6559 100644
>> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
>> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
>> @@ -294,6 +294,12 @@ int ath6kl_control_tx(void *devt, struct sk_buff *skb,
>> return -EACCES;
>> }
>>
>> + if (WARN_ON_ONCE(eid == ENDPOINT_UNUSED ||
>> + eid>= ENDPOINT_MAX)) {
>> + status = -EINVAL;
>> + goto fail_ctrl_tx;
>> + }
>
> Indentation for the line starting with eid doesn't look right (or my
> thunderbird is corrupting it again). But I can fix it up when I commit
> the patch. (I assume you will send v2 of this patchset.)

Thanks for the review. I'll submit v2 with your review comments
addressed in this patch series.

>
> Kalle

2012-09-21 09:18:33

by Kalle Valo

Subject: Re: [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()

On 09/20/2012 09:21 AM, [email protected] wrote:
> From: Raja Mani <[email protected]>
>
> It's safe to check endpoint id values before it get
> really used. Found this on code review.
>
> Signed-off-by: Raja Mani <[email protected]>
> ---
> drivers/net/wireless/ath/ath6kl/txrx.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
> index e867193..4ea6559 100644
> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
> @@ -294,6 +294,12 @@ int ath6kl_control_tx(void *devt, struct sk_buff *skb,
> return -EACCES;
> }
>
> + if (WARN_ON_ONCE(eid == ENDPOINT_UNUSED ||
> + eid >= ENDPOINT_MAX)) {
> + status = -EINVAL;
> + goto fail_ctrl_tx;
> + }

Indentation for the line starting with eid doesn't look right (or my
thunderbird is corrupting it again). But I can fix it up when I commit
the patch. (I assume you will send v2 of this patchset.)

Kalle

2012-09-21 09:16:54

by Kalle Valo

Subject: Re: [PATCH 2/3] ath6kl: Check for valid rate table index

On 09/20/2012 09:21 AM, [email protected] wrote:
> From: Raja Mani <[email protected]>
>
> There are 28 items defined in rate table array 'wmi_rate_tbl'.
> The rate table index (reply->rate_index) in ath6kl_wmi_bitrate_reply_rx()
> func is not checked for the valid max limit index before accessing
> rate table array. There may be some incidents to get memory crashes
> without safe max check. Fix this.
>
> Found this on code review.
>
> Signed-off-by: Raja Mani <[email protected]>
> ---
> drivers/net/wireless/ath/ath6kl/wmi.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> index d5263ff..666f56d 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -1171,6 +1171,9 @@ static int ath6kl_wmi_bitrate_reply_rx(struct wmi *wmi, u8 *datap, int len)
> rate = RATE_AUTO;
> } else {
> index = reply->rate_index & 0x7f;
> + if (index > (RATE_MCS_7_40 + 1))
> + return -EINVAL;

Please add WARN_ON_ONCE() to catch this easily:

if (WARN_ON_ONCE(index > (RATE_MCS_7_40 + 1)))
return -EINVAL;

Kalle

2012-09-21 09:34:25

by Raja Mani

Subject: Re: [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()

On Friday 21 September 2012 02:48 PM, Kalle Valo wrote:
> On 09/20/2012 09:21 AM, [email protected] wrote:
>> From: Raja Mani<[email protected]>
>>
>> It's safe to check endpoint id values before it get
>> really used. Found this on code review.
>>
>> Signed-off-by: Raja Mani<[email protected]>
>> ---
>> drivers/net/wireless/ath/ath6kl/txrx.c | 6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
>> index e867193..4ea6559 100644
>> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
>> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
>> @@ -294,6 +294,12 @@ int ath6kl_control_tx(void *devt, struct sk_buff *skb,
>> return -EACCES;
>> }
>>
>> + if (WARN_ON_ONCE(eid == ENDPOINT_UNUSED ||
>> + eid>= ENDPOINT_MAX)) {
>> + status = -EINVAL;
>> + goto fail_ctrl_tx;
>> + }
>
> Indentation for the line starting with eid doesn't look right (or my
> thunderbird is corrupting it again). But I can fix it up when I commit
> the patch. (I assume you will send v2 of this patchset.)

Thanks for the review. I'll submit v2 with your review comments addressed.

>
> Kalle

2012-09-21 09:14:59

by Kalle Valo

Subject: Re: [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair

On 09/20/2012 09:21 AM, [email protected] wrote:
> From: Raja Mani <[email protected]>
>
> Return value of ath6kl_get_regpair() is stored in 'regpair' in
> ath6kl_wmi_regdomain_event() func and it's directly accessed
> in the debug prints without checking for NULL value. There are
> situation to get NULL pointer as a return value from
> ath6kl_get_regpair() func. Fix this.
>
> Found this on code review.
>
> Signed-off-by: Raja Mani <[email protected]>
> ---
> drivers/net/wireless/ath/ath6kl/wmi.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> index 68b46bd..d5263ff 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -936,8 +936,9 @@ static void ath6kl_wmi_regdomain_event(struct wmi *wmi, u8 *datap, int len)
>
> regpair = ath6kl_get_regpair((u16) reg_code);
> country = ath6kl_regd_find_country_by_rd((u16) reg_code);
> - ath6kl_dbg(ATH6KL_DBG_WMI, "Regpair used: 0x%0x\n",
> - regpair->regDmnEnum);
> + if (regpair)
> + ath6kl_dbg(ATH6KL_DBG_WMI, "Regpair used: 0x%0x\n",
> + regpair->regDmnEnum);

The problem with this is that the regpair debug print is not printed at
all. Maybe something like this:

if (regpair)
ath6kl_dbg(ATH6KL_DBG_WMI, "Regpair used: 0x%0x\n",
regpair->regDmnEnum);
else
ath6kl_warn("Regpair not found reg_code 0x%0x\n",
reg_code);

I used the ath6kl_warn() here as this should not happen and we can more
easily notice the issue with ath6kl_warn(). Actually someone reported
about this crash on IRC earlier this week.

Kalle