| Message ID | 1350816727-1381-5-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Julia, On 10/21/2012 01:52 PM, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > This makes two changes. In ath6kl_wmi_cmd_send, a call to dev_kfree_skb on > the skb argument is added to the initial sanity check to more completely > establish the invariant that ath6kl_wmi_cmd_send owns its skb argument. > Then, in ath6kl_wmi_sync_point, on failure of the call to > ath6kl_wmi_cmd_send, the clearing of the local skb variable is moved up, so > that the error-handling code at the end of the function does not free it > again. > > A simplified version of the semantic match that finds this problem is as > follows: (http://coccinelle.lip6.fr/) > > // <smpl> > @r@ > identifier f,free,a; > parameter list[n] ps; > type T; > expression e; > @@ > > f(ps,T a,...) { > ... when any > when != a = e > if(...) { ... free(a); ... return ...; } > ... when any > } > > @@ > identifier r.f,r.free; > expression x,a; > expression list[r.n] xs; > @@ > > * x = f(xs,a,...); > if (...) { ... free(a); ... return ...; } > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> I think this patch which is commited to ath6kl.git has fixed this. commit 0616dc1f2bef563d7916c0dcedbb1bff7d9bd80b Author: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com> Date: Tue Aug 14 10:10:33 2012 +0530 ath6kl: Fix potential skb double free in ath6kl_wmi_sync_point() 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. kvalo: fix a checkpatch warning in ath6kl_wmi_cmd_send() Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> https://github.com/kvalo/ath6kl/commit/0616dc1f2bef563d7916c0dcedbb1bff7d9bd80b If you have the time, I would appreciate if you could take a look and confirm. Kalle -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c index c30ab4b..50f50e4 100644 --- a/drivers/net/wireless/ath/ath6kl/wmi.c +++ b/drivers/net/wireless/ath/ath6kl/wmi.c @@ -1677,8 +1677,10 @@ 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); @@ -2348,12 +2350,12 @@ static int ath6kl_wmi_sync_point(struct wmi *wmi, u8 if_idx) ret = ath6kl_wmi_cmd_send(wmi, if_idx, skb, WMI_SYNCHRONIZE_CMDID, NO_SYNC_WMIFLAG); - if (ret) - goto free_skb; - /* cmd buffer sent, we no longer own it */ skb = NULL; + if (ret) + goto free_skb; + for (index = 0; index < num_pri_streams; index++) { if (WARN_ON(!data_sync_bufs[index].skb))