Hi Gustavo,
On 21 August 2012 19:56, Gustavo Padovan <gustavo@xxxxxxxxxxx> wrote:
> Hi Dean,
>
> * Dean Jenkins <djenkins@xxxxxxxxxx> [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.
>
>>
>> Signed-off-by: Dean Jenkins <djenkins@xxxxxxxxxx>
>> ---
>> 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
Thanks for your feedback. Sorry for the delay in getting back to you.
My gmail failed to filter out your reply.
OK, I'll start with a patch for del_timer_sync(). This can be a
standalone patch.
For the removal of the refcnt, do you propose 1 patch to remove the
refcnt AND to manage the rfcomm session pointer ? I have doubts now
because you don't wish the stack to be broken between patches. Perhaps
it is possible to add a patch to manage the rfcomm session pointer
with the refcnt in place so we have "belt and braces" then have a
patch to remove the refcnt as the last thing to do.
I am open to suggestions.
Thanks,
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Professional Services UK/EMEA
MontaVista Software, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
