[PATCH] bluetooth: Do not free priv until timer handler hasn't actually stopped using it

[PATCH] bluetooth: Do not free priv until timer handler hasn't actually stopped using it

[Kirill Tkhai]

Function del_timer() does not guarantee that timer was really deleted.
If the timer handler is beeing executed at the moment, the function
just returns. So, it's possible to use already freed memory in the handler:

[ref: Documentation/DocBook/kernel-locking.tmpl]

This was found using grep and compile-tested only.

Signed-off-by: Kirill Tkhai <ktkhai@parallels.com>
CC: Marcel Holtmann <marcel@holtmann.org>
CC: Gustavo Padovan <gustavo@padovan.org>
CC: Johan Hedberg <johan.hedberg@gmail.com>
---
 drivers/bluetooth/hci_bcsp.c |    2 +-
 drivers/bluetooth/hci_h5.c   |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index 0bc87f7..d796a6d 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -722,7 +722,7 @@ static int bcsp_close(struct hci_uart *hu)
 	skb_queue_purge(&bcsp->unack);
 	skb_queue_purge(&bcsp->rel);
 	skb_queue_purge(&bcsp->unrel);
-	del_timer(&bcsp->tbcsp);
+	del_timer_sync(&bcsp->tbcsp);
 
 	kfree(bcsp);
 	return 0;
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index f6f4974..9f007f6 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -210,7 +210,7 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
-	del_timer(&h5->timer);
+	del_timer_sync(&h5->timer);
 
 	kfree(h5);
 

Re: [PATCH] bluetooth: Do not free priv until timer handler hasn't actually stopped using it

[Michael Knudsen]

On 02/14/2014 12:35 PM, Kirill Tkhai wrote:
> Function del_timer() does not guarantee that timer was really deleted.
> If the timer handler is beeing executed at the moment, the function
> just returns. So, it's possible to use already freed memory in the handler:

This is not enough.  The timer must be deleted in bcsp_close() before
hu->priv is set to NULL as the timer code dereferences hu->priv.

There is a similar issue in hci_h5.c where the timer must be stopped
before purging h5->unack.

-m.

Re: [PATCH] bluetooth: Do not free priv until timer handler hasn't actually stopped using it

[Kirill Tkhai]

В Птн, 14/02/2014 в 13:01 +0100, Michael Knudsen пишет:
> On 02/14/2014 12:35 PM, Kirill Tkhai wrote:
> > Function del_timer() does not guarantee that timer was really deleted.
> > If the timer handler is beeing executed at the moment, the function
> > just returns. So, it's possible to use already freed memory in the handler:
> 
> This is not enough.  The timer must be deleted in bcsp_close() before
> hu->priv is set to NULL as the timer code dereferences hu->priv.
> 
> There is a similar issue in hci_h5.c where the timer must be stopped
> before purging h5->unack.
> 
> -m.

Good, consider my email as reported-by. Please, fix that if you get on
well with bluetooth stack. I am far from it.

Kirill