skb given to ath6kl_control_tx() is owned by ath6kl_control_tx().
Calling function should not free the skb for error cases.
This is found during code review.
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/txrx.c | 4 ++-
drivers/net/wireless/ath/ath6kl/wmi.c | 35 +++++++++++++++++--------------
2 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index 7dfa0fd..aab8251 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -288,8 +288,10 @@ int ath6kl_control_tx(void *devt, struct sk_buff *skb,
int status = 0;
struct ath6kl_cookie *cookie = NULL;
- if (WARN_ON_ONCE(ar->state == ATH6KL_STATE_WOW))
+ if (WARN_ON_ONCE(ar->state == ATH6KL_STATE_WOW)) {
+ dev_kfree_skb(skb);
return -EACCES;
+ }
spin_lock_bh(&ar->lock);
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index cf91348..039f668 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -1739,8 +1739,11 @@ int ath6kl_wmi_cmd_send(struct wmi *wmi, u8 if_idx, struct sk_buff *skb,
int ret;
u16 info1;
- if (WARN_ON(skb == NULL || (if_idx > (wmi->parent_dev->vif_max - 1))))
+ if (WARN_ON(skb == NULL ||
+ (if_idx > (wmi->parent_dev->vif_max - 1)))) {
+ dev_kfree_skb(skb);
return -EINVAL;
+ }
ath6kl_dbg(ATH6KL_DBG_WMI, "wmi tx id %d len %d flag %d\n",
cmd_id, skb->len, sync_flag);
@@ -2352,8 +2355,10 @@ static int ath6kl_wmi_data_sync_send(struct wmi *wmi, struct sk_buff *skb,
struct wmi_data_hdr *data_hdr;
int ret;
- if (WARN_ON(skb == NULL || ep_id == wmi->ep_id))
+ if (WARN_ON(skb == NULL || ep_id == wmi->ep_id)) {
+ dev_kfree_skb(skb);
return -EINVAL;
+ }
skb_push(skb, sizeof(struct wmi_data_hdr));
@@ -2390,10 +2395,8 @@ static int ath6kl_wmi_sync_point(struct wmi *wmi, u8 if_idx)
spin_unlock_bh(&wmi->lock);
skb = ath6kl_wmi_get_new_buf(sizeof(*cmd));
- if (!skb) {
- ret = -ENOMEM;
- goto free_skb;
- }
+ if (!skb)
+ return -ENOMEM;
cmd = (struct wmi_sync_cmd *) skb->data;
@@ -2416,7 +2419,7 @@ static int ath6kl_wmi_sync_point(struct wmi *wmi, u8 if_idx)
* then do not send the Synchronize cmd on the control ep
*/
if (ret)
- goto free_skb;
+ goto free_cmd_skb;
/*
* Send sync cmd followed by sync data messages on all
@@ -2426,15 +2429,12 @@ static int ath6kl_wmi_sync_point(struct wmi *wmi, u8 if_idx)
NO_SYNC_WMIFLAG);
if (ret)
- goto free_skb;
-
- /* cmd buffer sent, we no longer own it */
- skb = NULL;
+ goto free_data_skb;
for (index = 0; index < num_pri_streams; index++) {
if (WARN_ON(!data_sync_bufs[index].skb))
- break;
+ goto free_data_skb;
ep_id = ath6kl_ac2_endpoint_id(wmi->parent_dev,
data_sync_bufs[index].
@@ -2443,17 +2443,20 @@ static int ath6kl_wmi_sync_point(struct wmi *wmi, u8 if_idx)
ath6kl_wmi_data_sync_send(wmi, data_sync_bufs[index].skb,
ep_id, if_idx);
- if (ret)
- break;
-
data_sync_bufs[index].skb = NULL;
+
+ if (ret)
+ goto free_data_skb;
}
-free_skb:
+ return 0;
+
+free_cmd_skb:
/* free up any resources left over (possibly due to an error) */
if (skb)
dev_kfree_skb(skb);
+free_data_skb:
for (index = 0; index < num_pri_streams; index++) {
if (data_sync_bufs[index].skb != NULL) {
dev_kfree_skb((struct sk_buff *)data_sync_bufs[index].
--
1.7.0.4
On Tuesday 14 August 2012 07:59 PM, Kalle Valo wrote:
> On 08/14/2012 07:40 AM, Vasanthakumar Thiagarajan wrote:
>> skb given to ath6kl_control_tx() is owned by ath6kl_control_tx().
>> Calling function should not free the skb for error cases.
>> This is found during code review.
>>
>> Signed-off-by: Vasanthakumar Thiagarajan<[email protected]>
>
> Thanks, both applied. There was a checkpatch warning about parenthesis
> alignment in the first patch but I fixed it.
Thanks, but for some reason i dont see any checkpatch warnings.
Vasanth
On 08/16/2012 09:21 AM, Vasanthakumar Thiagarajan wrote:
> On Tuesday 14 August 2012 07:59 PM, Kalle Valo wrote:
>> On 08/14/2012 07:40 AM, Vasanthakumar Thiagarajan wrote:
>>> skb given to ath6kl_control_tx() is owned by ath6kl_control_tx().
>>> Calling function should not free the skb for error cases.
>>> This is found during code review.
>>>
>>> Signed-off-by: Vasanthakumar Thiagarajan<[email protected]>
>>
>> Thanks, both applied. There was a checkpatch warning about parenthesis
>> alignment in the first patch but I fixed it.
>
> Thanks, but for some reason i dont see any checkpatch warnings.
I'm using --strict which enables even more checks. But with latest
checkpatch there are false warnings (IMHO) so I have to use an older
version of checkpatch. But don't worry about that, it's enough that you
run checkpatch without --strict, I can deal with the rest.
Kalle
We bail out from ath6kl_tx_complete() if any of the sanity
checks on skb and ath6kl_cookie fails. By doing this we
potentially leak few remaining buffers in packet_queue.
Make sure to proceed processing the remaining buffers
as well. This issue is found during code review.
Reported-by: Wang yufeng <[email protected]>
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
V2 - Change WARN_ON() to WARN_ON_ONCE().
drivers/net/wireless/ath/ath6kl/txrx.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index aab8251..740a488 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -698,21 +698,26 @@ void ath6kl_tx_complete(struct htc_target *target,
list_del(&packet->list);
ath6kl_cookie = (struct ath6kl_cookie *)packet->pkt_cntxt;
- if (!ath6kl_cookie)
- goto fatal;
+ if (WARN_ON_ONCE(!ath6kl_cookie))
+ continue;
status = packet->status;
skb = ath6kl_cookie->skb;
eid = packet->endpoint;
map_no = ath6kl_cookie->map_no;
- if (!skb || !skb->data)
- goto fatal;
+ if (WARN_ON_ONCE(!skb || !skb->data)) {
+ dev_kfree_skb(skb);
+ ath6kl_free_cookie(ar, ath6kl_cookie);
+ continue;
+ }
__skb_queue_tail(&skb_queue, skb);
- if (!status && (packet->act_len != skb->len))
- goto fatal;
+ if (WARN_ON_ONCE(!status && (packet->act_len != skb->len))) {
+ ath6kl_free_cookie(ar, ath6kl_cookie);
+ continue;
+ }
ar->tx_pending[eid]--;
@@ -794,11 +799,6 @@ void ath6kl_tx_complete(struct htc_target *target,
wake_up(&ar->event_wq);
return;
-
-fatal:
- WARN_ON(1);
- spin_unlock_bh(&ar->lock);
- return;
}
void ath6kl_tx_data_cleanup(struct ath6kl *ar)
--
1.7.0.4
On 08/14/2012 07:40 AM, Vasanthakumar Thiagarajan wrote:
> skb given to ath6kl_control_tx() is owned by ath6kl_control_tx().
> Calling function should not free the skb for error cases.
> This is found during code review.
>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
Thanks, both applied. There was a checkpatch warning about parenthesis
alignment in the first patch but I fixed it.
Kalle