Re: [PATCH 3/4] Bluetooth: Avoid rfcomm_session_timeout using freed pointer
From: Gustavo Padovan <hidden>
Date: 2012-08-21 18:56:06
Hi Dean, * Dean Jenkins [off-list ref] [2012-08-11 19:47:09 +0100]:
rfcomm_session_timeout() protects the scenario of the remote Bluetooth device failing to send a DISC on the rfcomm control channel after the last data DLC channel has been closed. There is a race condition between the timer expiring causing rfcomm_session_timeout() to run and the rfcomm session being deleted. If the rfcomm session is deleted then rfcomm_session_timeout() would use a freed rfcomm session pointer resulting in a potential kernel crash or memory corruption. Note the timer is cleared before the rfcomm session is deleted by del_timer() so the circumstances for a failure to occur are as follows: rfcomm_session_timeout() needs to be executing before the del_timer() is called to clear the timer but rfcomm_session_timeout() needs to be delayed from using the rfcomm session pointer until after the session has been deleted. Therefore, there is a very small window of opportunity for failure. The solution is to use del_timer_sync() instead of del_timer() as this ensures that rfcomm_session_timeout() is not running when del_timer_sync() returns. This means that it is not possible for rfcomm_session_timeout() to run after the rfcomm session has been deleted.
quoted hunk ↗ jump to hunk
Signed-off-by: Dean Jenkins <redacted> --- net/bluetooth/rfcomm/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index 24d4d3c..c7921fd 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c@@ -264,8 +264,8 @@ static void rfcomm_session_clear_timer(struct rfcomm_session *s) { BT_DBG("session %p state %ld", s, s->state); - if (timer_pending(&s->timer)) - del_timer(&s->timer); + /* ensure rfcomm_session_timeout() is not running past this point */ + del_timer_sync(&s->timer);
I'm not happy of the idea of let the stack broken between patches 2 and 3 (this one). As you said if we use del_timer_sync() we don't need rfcnt here, can you add this as a first patch, maybe? and after this continue to remove the rest of the refcount code? Gustavo