* [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair
@ 2012-09-20 6:21 rmani
2012-09-20 6:21 ` [PATCH 2/3] ath6kl: Check for valid rate table index rmani
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: rmani @ 2012-09-20 6:21 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Raja Mani
From: Raja Mani <rmani@qca.qualcomm.com>
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 <rmani@qca.qualcomm.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/3] ath6kl: Check for valid rate table index
2012-09-20 6:21 [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair rmani
@ 2012-09-20 6:21 ` rmani
2012-09-21 9:16 ` Kalle Valo
2012-09-20 6:21 ` [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx() rmani
2012-09-21 9:14 ` [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair Kalle Valo
2 siblings, 1 reply; 8+ messages in thread
From: rmani @ 2012-09-20 6:21 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Raja Mani
From: Raja Mani <rmani@qca.qualcomm.com>
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 <rmani@qca.qualcomm.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] ath6kl: Check for valid rate table index
2012-09-20 6:21 ` [PATCH 2/3] ath6kl: Check for valid rate table index rmani
@ 2012-09-21 9:16 ` Kalle Valo
0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2012-09-21 9:16 UTC (permalink / raw)
To: rmani; +Cc: linux-wireless, ath6kl-devel
On 09/20/2012 09:21 AM, rmani@qca.qualcomm.com wrote:
> From: Raja Mani <rmani@qca.qualcomm.com>
>
> 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 <rmani@qca.qualcomm.com>
> ---
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()
2012-09-20 6:21 [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair rmani
2012-09-20 6:21 ` [PATCH 2/3] ath6kl: Check for valid rate table index rmani
@ 2012-09-20 6:21 ` rmani
2012-09-21 9:18 ` Kalle Valo
2012-09-21 9:14 ` [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair Kalle Valo
2 siblings, 1 reply; 8+ messages in thread
From: rmani @ 2012-09-20 6:21 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Raja Mani
From: Raja Mani <rmani@qca.qualcomm.com>
It's safe to check endpoint id values before it get
really used. Found this on code review.
Signed-off-by: Raja Mani <rmani@qca.qualcomm.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()
2012-09-20 6:21 ` [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx() rmani
@ 2012-09-21 9:18 ` Kalle Valo
2012-09-21 9:34 ` Raja Mani
2012-09-21 9:35 ` Raja Mani
0 siblings, 2 replies; 8+ messages in thread
From: Kalle Valo @ 2012-09-21 9:18 UTC (permalink / raw)
To: rmani; +Cc: linux-wireless, ath6kl-devel
On 09/20/2012 09:21 AM, rmani@qca.qualcomm.com wrote:
> From: Raja Mani <rmani@qca.qualcomm.com>
>
> It's safe to check endpoint id values before it get
> really used. Found this on code review.
>
> Signed-off-by: Raja Mani <rmani@qca.qualcomm.com>
> ---
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()
2012-09-21 9:18 ` Kalle Valo
@ 2012-09-21 9:34 ` Raja Mani
2012-09-21 9:35 ` Raja Mani
1 sibling, 0 replies; 8+ messages in thread
From: Raja Mani @ 2012-09-21 9:34 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, ath6kl-devel
On Friday 21 September 2012 02:48 PM, Kalle Valo wrote:
> On 09/20/2012 09:21 AM, rmani@qca.qualcomm.com wrote:
>> From: Raja Mani<rmani@qca.qualcomm.com>
>>
>> It's safe to check endpoint id values before it get
>> really used. Found this on code review.
>>
>> Signed-off-by: Raja Mani<rmani@qca.qualcomm.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx()
2012-09-21 9:18 ` Kalle Valo
2012-09-21 9:34 ` Raja Mani
@ 2012-09-21 9:35 ` Raja Mani
1 sibling, 0 replies; 8+ messages in thread
From: Raja Mani @ 2012-09-21 9:35 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, ath6kl-devel
On Friday 21 September 2012 02:48 PM, Kalle Valo wrote:
> On 09/20/2012 09:21 AM, rmani@qca.qualcomm.com wrote:
>> From: Raja Mani<rmani@qca.qualcomm.com>
>>
>> It's safe to check endpoint id values before it get
>> really used. Found this on code review.
>>
>> Signed-off-by: Raja Mani<rmani@qca.qualcomm.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair
2012-09-20 6:21 [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair rmani
2012-09-20 6:21 ` [PATCH 2/3] ath6kl: Check for valid rate table index rmani
2012-09-20 6:21 ` [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx() rmani
@ 2012-09-21 9:14 ` Kalle Valo
2 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2012-09-21 9:14 UTC (permalink / raw)
To: rmani; +Cc: linux-wireless, ath6kl-devel
On 09/20/2012 09:21 AM, rmani@qca.qualcomm.com wrote:
> From: Raja Mani <rmani@qca.qualcomm.com>
>
> 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 <rmani@qca.qualcomm.com>
> ---
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-09-21 9:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-20 6:21 [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair rmani
2012-09-20 6:21 ` [PATCH 2/3] ath6kl: Check for valid rate table index rmani
2012-09-21 9:16 ` Kalle Valo
2012-09-20 6:21 ` [PATCH 3/3] ath6kl: Check for valid endpoint ID values in ath6kl_control_tx() rmani
2012-09-21 9:18 ` Kalle Valo
2012-09-21 9:34 ` Raja Mani
2012-09-21 9:35 ` Raja Mani
2012-09-21 9:14 ` [PATCH 1/3] ath6kl: Avoid null ptr dereference while printing reg domain pair Kalle Valo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).