2014-02-14 11:35:48

by Kirill Tkhai

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

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 <[email protected]>
CC: Marcel Holtmann <[email protected]>
CC: Gustavo Padovan <[email protected]>
CC: Johan Hedberg <[email protected]>
---
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);

2014-02-14 12:10:10

by Kirill Tkhai

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

В Птн, 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

2014-02-14 12:01:06

by Michael Knudsen

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

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.