[PATCH 1/2] ath6kl: Fix potential skb double free in ath6kl_wmi_sync_point()

[PATCH 1/2] ath6kl: Fix potential skb double free in ath6kl_wmi_sync_point()

[Vasanthakumar Thiagarajan]

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 <vthiagar@qca.qualcomm.com>
---
 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

[PATCH 2/2] ath6kl: Fix potential memory leak in ath6kl_tx_complete()

[Vasanthakumar Thiagarajan]

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 <yufengw@qca.qualcomm.com>
Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/txrx.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index aab8251..4b26ba0 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -698,21 +698,29 @@ 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 (!ath6kl_cookie) {
+			WARN_ON(1);
+			continue;
+		}
 
 		status = packet->status;
 		skb = ath6kl_cookie->skb;
 		eid = packet->endpoint;
 		map_no = ath6kl_cookie->map_no;
 
-		if (!skb || !skb->data)
-			goto fatal;
+		if (!skb || !skb->data) {
+			WARN_ON(1);
+			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 (!status && (packet->act_len != skb->len)) {
+			ath6kl_free_cookie(ar, ath6kl_cookie);
+			continue;
+		}
 
 		ar->tx_pending[eid]--;
 
@@ -794,11 +802,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

Re: [PATCH 2/2] ath6kl: Fix potential memory leak in ath6kl_tx_complete()

[Kalle Valo]

On 08/13/2012 04:48 PM, Vasanthakumar Thiagarajan wrote:
> 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 <yufengw@qca.qualcomm.com>
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
> ---
>  drivers/net/wireless/ath/ath6kl/txrx.c |   25 ++++++++++++++-----------
>  1 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
> index aab8251..4b26ba0 100644
> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
> @@ -698,21 +698,29 @@ 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 (!ath6kl_cookie) {
> +			WARN_ON(1);
> +			continue;
> +		}

Please use WARN_ON_ONCE() to avoid excess log messages (as this is in
data path) and put the WARN_ON() inside if:

if (WARN_ON_ONCE(!ath6kl_cookie))
	continue;

> +		if (!skb || !skb->data) {
> +			WARN_ON(1);
> +			dev_kfree_skb(skb);
> +			ath6kl_free_cookie(ar, ath6kl_cookie);
> +			continue;
> +		}

Same here.

Kalle

Re: [PATCH 2/2] ath6kl: Fix potential memory leak in ath6kl_tx_complete()

[Vasanthakumar Thiagarajan]

On Mon, Aug 13, 2012 at 05:57:09PM +0300, Kalle Valo wrote:
> On 08/13/2012 04:48 PM, Vasanthakumar Thiagarajan wrote:
> > 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 <yufengw@qca.qualcomm.com>
> > Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
> > ---
> >  drivers/net/wireless/ath/ath6kl/txrx.c |   25 ++++++++++++++-----------
> >  1 files changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
> > index aab8251..4b26ba0 100644
> > --- a/drivers/net/wireless/ath/ath6kl/txrx.c
> > +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
> > @@ -698,21 +698,29 @@ 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 (!ath6kl_cookie) {
> > +			WARN_ON(1);
> > +			continue;
> > +		}
> 
> Please use WARN_ON_ONCE() to avoid excess log messages (as this is in
> data path) and put the WARN_ON() inside if:
> 
> if (WARN_ON_ONCE(!ath6kl_cookie))
> 	continue;

Sure, thanks!.

> 
> > +		if (!skb || !skb->data) {
> > +			WARN_ON(1);
> > +			dev_kfree_skb(skb);
> > +			ath6kl_free_cookie(ar, ath6kl_cookie);
> > +			continue;
> > +		}
> 
> Same here.

Sure.

Vasanth