On Mon, May 30, 2011 at 05:48:52PM -0300, Gustavo F. Padovan wrote:
> Hi David,
>
> * David Fries <david@xxxxxxxxx> [2011-05-21 14:02:53 -0500]:
>
> > rfcomm_process_sessions is calling rfcomm_process_rx, but
> > in this case the session is closed and freed leaving a
> > dangling pointer that blows up when rfcomm_process_rx returns
> > and rfcomm_process_dlcs is called with the now dangling session
> > pointer. Check to see if the entry is still in the list.
> >
> > Signed-off-by: David Fries <David@xxxxxxxxx>
> > Cc: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> > Cc: "Gustavo F. Padovan" <padovan@xxxxxxxxxxxxxx>
> > ---
> > I sent out an ealier patch,
> > Date: Mon, 21 Mar 2011 21:38:10 -0500
> > Subject: [PATCH] rfcomm/core.c avoid dangling pointer, check session
> >
> > That version added a return value to rfcomm_session_close to determine
> > if the session was closed. I thought this would be cleaner.
> >
> > I can reproduce using blueman-manager on desktop, and Motorola S305 bluetooth
> > headset, 2.6.39, but it can take a few attempts. Start out with the
> > desktop as the last device the S305 paired with.
> > desktop, connect to the S305,
> > S305, turn on
> > desktop (connection fails)
> > desktop (connection automatically comes up now that S305 is on)
> > desktop disconnect S305
> > desktop (kernel panic)
> >
> > While rfcomm_process_sessions looks symmetrical,
> > rfcomm_session_hold(s);
> > rfcomm_process_rx
> > rfcomm_process_dlcs
> > rfcomm_session_put(s);
> >
> > rfcomm_process_rx
> > if (sk->sk_state == BT_CLOSED) {
> > if (!s->initiator)
> > rfcomm_session_put(s);
> > rfcomm_session_close(s, sk->sk_err);
> >
> > Which isn't symmetrical. I don't know enough about the subsystem to
> > know if there is a better cleaner way to fix this, or if my patch is
> > the best solution.
> >
> > net/bluetooth/rfcomm/core.c | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> > index c997393..ac47ef3 100644
> > --- a/net/bluetooth/rfcomm/core.c
> > +++ b/net/bluetooth/rfcomm/core.c
> > @@ -1952,6 +1952,12 @@ static inline void rfcomm_process_sessions(void)
> >
> > default:
> > rfcomm_process_rx(s);
> > + /* The current session can be closed as part of rx
> > + * in which case s is now dangling. Check if it
> > + * has been removed.
> > + */
> > + if(n->prev != p)
> > + continue;
> > break;
> > }
>
> I don't like this, it's not the proper fix. So I'm trying to figure out this
> and fix it. Can you try this patch:
Not without some explaination as to what it's trying to accomplish
(other than crash my computer), it's just a single CPU system, so it
isn't a very good test case for crashes in use after free anyway.
s->sock has been released by sock_release, s has been freed by kfree
(in rfcomm_session_del), which leaves s->sock-> completely dangling by
now. That's why I was using something other than s to determine if
it's dangling, and looking to see if it was still in the list looked
like the best way to me.
I posted an earlier patch, 'Date: Mon, 21 Mar 2011 21:38:10 -0500'
where I added a return value to to rfcomm_session_put, as another way
to determine if s has been freed and now dangling. The other question
is why check BT_CLOSED outside of default? It's only
rfcomm_process_rx that will delete the session with something still
holding on to it.
I would rather see a fix where rfcomm_process_rx doesn't do the put
which caused this situation. Also, now that I look at it,
rfcomm_process_rx is doing a close after it did a put, which means the
entire close is dealing with a dangling pointer. Looks to me like the
order could be switched, pending the outcome of the propper fix.
if (sk->sk_state == BT_CLOSED) {
if (!s->initiator)
rfcomm_session_put(s);
rfcomm_session_close(s, sk->sk_err);
}
> padovan bluetooth-next-2.6 $ git diff
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 5759bb7..75c58ed 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -1958,6 +1958,9 @@ static inline void rfcomm_process_sessions(void)
> break;
> }
>
> + if (s->sock->sk->sk_state == BT_CLOSED)
> + continue;
> +
> rfcomm_process_dlcs(s);
>
> rfcomm_session_put(s);
>
> --
> Gustavo F. Padovan
> http://profusion.mobi
--
David Fries <david@xxxxxxxxx> PGP pub CB1EE8F0
http://fries.net/~david/
--
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
