* [PATCH] bluetooth: btmrvl: skb resource leak, and double free. @ 2015-09-01 19:24 Kieran Bingham 2015-09-01 22:12 ` Marcel Holtmann 0 siblings, 1 reply; 3+ messages in thread From: Kieran Bingham @ 2015-09-01 19:24 UTC (permalink / raw) To: marcel, gustavo, johan.hedberg; +Cc: linux-bluetooth, akarwar, Kieran Bingham if btmrvl_tx_pkt() is called, and the branch if (skb_headroom(skb) < BTM_HEADER_LEN) evaluates positive, a new skb is allocated via skb_realloc_headroom. The original skb is stored in a tmp variable, before being free'd. However on success, the new skb, is not free'd, nor is it returned to the caller which will then double-free the original skb. This issue exists from the original driver submission in commit: #132ff4e5fa8dfb71a7d99902f88043113947e972 Move the error handling, and kfree_skb inside the helper function, where the statistics can be updated, and the skb free'd at the appropriate occasion. Reported by coverity (CID 113422) Signed-off-by: Kieran Bingham <kieranbingham@gmail.com> --- drivers/bluetooth/btmrvl_main.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c index de05deb..ceaef95 100644 --- a/drivers/bluetooth/btmrvl_main.c +++ b/drivers/bluetooth/btmrvl_main.c @@ -366,15 +366,15 @@ void btmrvl_firmware_dump(struct btmrvl_private *priv) static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) { - int ret = 0; + int ret = -EINVAL; if (!skb || !skb->data) - return -EINVAL; + goto tx_pkt_out; if (!skb->len || ((skb->len + BTM_HEADER_LEN) > BTM_UPLD_SIZE)) { BT_ERR("Tx Error: Bad skb length %d : %d", skb->len, BTM_UPLD_SIZE); - return -EINVAL; + goto tx_pkt_out; } if (skb_headroom(skb) < BTM_HEADER_LEN) { @@ -385,7 +385,7 @@ static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) BT_ERR("Tx Error: realloc_headroom failed %d", BTM_HEADER_LEN); skb = tmp; - return -EINVAL; + goto tx_pkt_out; } kfree_skb(tmp); @@ -406,6 +406,14 @@ static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) if (priv->hw_host_to_card) ret = priv->hw_host_to_card(priv, skb->data, skb->len); +tx_pkt_out: + if (ret) + priv->btmrvl_dev.hcidev->stat.err_tx++; + else + priv->btmrvl_dev.hcidev->stat.byte_tx += skb->len; + + kfree_skb(skb); + return ret; } @@ -670,14 +678,8 @@ static int btmrvl_service_main_thread(void *data) continue; skb = skb_dequeue(&adapter->tx_queue); - if (skb) { - if (btmrvl_tx_pkt(priv, skb)) - priv->btmrvl_dev.hcidev->stat.err_tx++; - else - priv->btmrvl_dev.hcidev->stat.byte_tx += skb->len; - - kfree_skb(skb); - } + if (skb) + btmrvl_tx_pkt(priv, skb); } return 0; -- 2.1.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] bluetooth: btmrvl: skb resource leak, and double free. 2015-09-01 19:24 [PATCH] bluetooth: btmrvl: skb resource leak, and double free Kieran Bingham @ 2015-09-01 22:12 ` Marcel Holtmann 2015-09-02 12:22 ` Kieran Bingham 0 siblings, 1 reply; 3+ messages in thread From: Marcel Holtmann @ 2015-09-01 22:12 UTC (permalink / raw) To: Kieran Bingham Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, akarwar Hi Kieran, > if btmrvl_tx_pkt() is called, and the branch > if (skb_headroom(skb) < BTM_HEADER_LEN) > evaluates positive, a new skb is allocated via skb_realloc_headroom. is that branch actually valid and can happen? I mean someone must have thought about it, but skb usage in Bluetooth is pretty linear and normally all skb should be allocated with a 8 byte headroom. For example when we look at btsdio_tx_packet() from btsdio.c, then we do not even bother with checking that we have enough headroom. We know that the skb we got from the core was allocated with enough headroom. I am almost convinced that coverity found dead code here that is never executed. > The original skb is stored in a tmp variable, before being free'd. > However on success, the new skb, is not free'd, nor is it > returned to the caller which will then double-free the original skb. > > This issue exists from the original driver submission in > commit: #132ff4e5fa8dfb71a7d99902f88043113947e972 > > Move the error handling, and kfree_skb inside the helper function, > where the statistics can be updated, and the skb free'd at the > appropriate occasion. > > Reported by coverity (CID 113422) > > Signed-off-by: Kieran Bingham <kieranbingham@gmail.com> > --- > drivers/bluetooth/btmrvl_main.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c > index de05deb..ceaef95 100644 > --- a/drivers/bluetooth/btmrvl_main.c > +++ b/drivers/bluetooth/btmrvl_main.c > @@ -366,15 +366,15 @@ void btmrvl_firmware_dump(struct btmrvl_private *priv) > > static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) > { > - int ret = 0; > + int ret = -EINVAL; > > if (!skb || !skb->data) > - return -EINVAL; > + goto tx_pkt_out; with your change the skb can never be NULL since you are checking this before calling this function. > > if (!skb->len || ((skb->len + BTM_HEADER_LEN) > BTM_UPLD_SIZE)) { > BT_ERR("Tx Error: Bad skb length %d : %d", > skb->len, BTM_UPLD_SIZE); > - return -EINVAL; > + goto tx_pkt_out; > } > > if (skb_headroom(skb) < BTM_HEADER_LEN) { > @@ -385,7 +385,7 @@ static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) > BT_ERR("Tx Error: realloc_headroom failed %d", > BTM_HEADER_LEN); > skb = tmp; > - return -EINVAL; > + goto tx_pkt_out; > } > > kfree_skb(tmp); > @@ -406,6 +406,14 @@ static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) > if (priv->hw_host_to_card) > ret = priv->hw_host_to_card(priv, skb->data, skb->len); > > +tx_pkt_out: > + if (ret) > + priv->btmrvl_dev.hcidev->stat.err_tx++; > + else > + priv->btmrvl_dev.hcidev->stat.byte_tx += skb->len; I do not like this conditional based on ret. I have to twist my brain to ensure that ret == 0 is ensured so that the skb->len gets added correctly. > + > + kfree_skb(skb); > + > return ret; > } > > @@ -670,14 +678,8 @@ static int btmrvl_service_main_thread(void *data) > continue; > > skb = skb_dequeue(&adapter->tx_queue); > - if (skb) { > - if (btmrvl_tx_pkt(priv, skb)) > - priv->btmrvl_dev.hcidev->stat.err_tx++; > - else > - priv->btmrvl_dev.hcidev->stat.byte_tx += skb->len; > - > - kfree_skb(skb); > - } > + if (skb) > + btmrvl_tx_pkt(priv, skb); > } Regards Marcel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] bluetooth: btmrvl: skb resource leak, and double free. 2015-09-01 22:12 ` Marcel Holtmann @ 2015-09-02 12:22 ` Kieran Bingham 0 siblings, 0 replies; 3+ messages in thread From: Kieran Bingham @ 2015-09-02 12:22 UTC (permalink / raw) To: Marcel Holtmann Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, Amitkumar Karwar Hi Marcel, On 1 September 2015 at 23:12, Marcel Holtmann <marcel@holtmann.org> wrote: > Hi Kieran, > >> if btmrvl_tx_pkt() is called, and the branch >> if (skb_headroom(skb) < BTM_HEADER_LEN) >> evaluates positive, a new skb is allocated via skb_realloc_headroom. > > is that branch actually valid and can happen? I mean someone must have thought about it, but skb usage in Bluetooth is pretty linear and normally all skb should be allocated with a 8 byte headroom. > > For example when we look at btsdio_tx_packet() from btsdio.c, then we do not even bother with checking that we have enough headroom. We know that the skb we got from the core was allocated with enough headroom. > > I am almost convinced that coverity found dead code here that is never executed. I started looking at coverity bugs, ordered by oldest first. So this has been around for a long time, without anyone looking into it or changing it (or noticing a leak) So I suspect this could actually all be evidence towards dead-code. In fact, now I think about it more - it would have resulted in a double-free - which should have been caught had it occurred ... I've looked through the other drivers, and see that most only ever skb_push with 1 byte, and are always happy to do that. And yes, the btsdio pushes 4 bytes, just like this one, and it is sharing the same hci_register_dev structure and hierarchy. I'll re-spin with a removal of the broken / dead code instead, much cleaner and simpler. >> The original skb is stored in a tmp variable, before being free'd. >> However on success, the new skb, is not free'd, nor is it >> returned to the caller which will then double-free the original skb. >> >> This issue exists from the original driver submission in >> commit: #132ff4e5fa8dfb71a7d99902f88043113947e972 >> >> Move the error handling, and kfree_skb inside the helper function, >> where the statistics can be updated, and the skb free'd at the >> appropriate occasion. >> >> Reported by coverity (CID 113422) >> >> Signed-off-by: Kieran Bingham <kieranbingham@gmail.com> >> --- >> drivers/bluetooth/btmrvl_main.c | 26 ++++++++++++++------------ >> 1 file changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c >> index de05deb..ceaef95 100644 >> --- a/drivers/bluetooth/btmrvl_main.c >> +++ b/drivers/bluetooth/btmrvl_main.c >> @@ -366,15 +366,15 @@ void btmrvl_firmware_dump(struct btmrvl_private *priv) >> >> static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) >> { >> - int ret = 0; >> + int ret = -EINVAL; >> >> if (!skb || !skb->data) >> - return -EINVAL; >> + goto tx_pkt_out; > > with your change the skb can never be NULL since you are checking this before calling this function. The calling function provides the check before my patch, so yes even without my change it couldn't have been null ... perhaps the null check can be removed, although its fairly harmless here. >> >> if (!skb->len || ((skb->len + BTM_HEADER_LEN) > BTM_UPLD_SIZE)) { >> BT_ERR("Tx Error: Bad skb length %d : %d", >> skb->len, BTM_UPLD_SIZE); >> - return -EINVAL; >> + goto tx_pkt_out; >> } >> >> if (skb_headroom(skb) < BTM_HEADER_LEN) { >> @@ -385,7 +385,7 @@ static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) >> BT_ERR("Tx Error: realloc_headroom failed %d", >> BTM_HEADER_LEN); >> skb = tmp; >> - return -EINVAL; >> + goto tx_pkt_out; >> } >> >> kfree_skb(tmp); >> @@ -406,6 +406,14 @@ static int btmrvl_tx_pkt(struct btmrvl_private *priv, struct sk_buff *skb) >> if (priv->hw_host_to_card) >> ret = priv->hw_host_to_card(priv, skb->data, skb->len); >> >> +tx_pkt_out: >> + if (ret) >> + priv->btmrvl_dev.hcidev->stat.err_tx++; >> + else >> + priv->btmrvl_dev.hcidev->stat.byte_tx += skb->len; > > I do not like this conditional based on ret. I have to twist my brain to ensure that ret == 0 is ensured so that the skb->len gets added correctly. This code can go back to being untouched, and left in the parent function. (Unless you'd prefer it to be rewritten there too?) >> + >> + kfree_skb(skb); >> + >> return ret; >> } >> >> @@ -670,14 +678,8 @@ static int btmrvl_service_main_thread(void *data) >> continue; >> >> skb = skb_dequeue(&adapter->tx_queue); >> - if (skb) { >> - if (btmrvl_tx_pkt(priv, skb)) >> - priv->btmrvl_dev.hcidev->stat.err_tx++; >> - else >> - priv->btmrvl_dev.hcidev->stat.byte_tx += skb->len; >> - >> - kfree_skb(skb); >> - } >> + if (skb) >> + btmrvl_tx_pkt(priv, skb); >> } > > Regards > > Marcel -- Regards Kieran ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-02 12:22 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-01 19:24 [PATCH] bluetooth: btmrvl: skb resource leak, and double free Kieran Bingham 2015-09-01 22:12 ` Marcel Holtmann 2015-09-02 12:22 ` Kieran Bingham
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox