[PATCH 3/3] Fix invalid memory access issues in avdtp module

[PATCH 3/3] Fix invalid memory access issues in avdtp module

[Rafal Michalski]

Changing stream state from STREAMING to IDLE can be associated with side
effects under some circumstances (such as terminating bluetoothd during
music is streamed). In this case, after connection is lost, stream state
changes from STREAMING to IDLE - "avdtp_sep_set_state" is triggered which
invokes callback called "stream_state_changed" which internally invokes
"avdtp_sep_set_state" (state of stream doesn't change and stays as IDLE)
second time and then stream object is freed by "stream_free" at the end
of "avdtp_sep_set_state". After returning from callback
"stream_state_changed", first triggered "avdtp_sep_set_state" attempts
to free stream object again ("if (state == AVDTP_STATE_IDLE)" condition
is still satisfied) and it leads to invalid read/write/free issues
(reported by valgrind) in "stream free" body, since "stream" is "alias"
pointer to stream object which is already out of date (memory for stream
object has been already freed). Moreover freeing stream object also
discards its callback list so after finishing iteration on this list
(in "avdtp_sep_set_state") "l" pointer is out of date and getting
"l->next" pointer (returned by "g_slist_next(l)") leads to invalid read
issue.

This patch prevents from this special case by freeing stream object only
when it is present on streams list and removing from this list when
stream object would be freed. Additionally to avoid issue with callback
list loop for this has been modified and "while" used instead of "for"
loop variant.
---
 audio/avdtp.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 6018c37..19c9fd1 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1086,7 +1086,6 @@ static void avdtp_sep_set_state(struct avdtp *session,
 			g_source_remove(stream->idle_timer);
 			stream->idle_timer = 0;
 		}
-		session->streams = g_slist_remove(session->streams, stream);
 		if (session->pending_open == stream)
 			handle_transport_connect(session, NULL, 0, 0);
 		if (session->req && session->req->stream == stream)
@@ -1098,13 +1097,18 @@ static void avdtp_sep_set_state(struct avdtp *session,
 		break;
 	}
 
-	for (l = stream->callbacks; l != NULL; l = g_slist_next(l)) {
+	l = stream->callbacks;
+	while (l != NULL) {
 		struct stream_callback *cb = l->data;
+		l = g_slist_next(l);
 		cb->cb(stream, old_state, state, err_ptr, cb->user_data);
 	}
 
-	if (state == AVDTP_STATE_IDLE)
+	if (state == AVDTP_STATE_IDLE &&
+				g_slist_find(session->streams, stream)) {
+		session->streams = g_slist_remove(session->streams, stream);
 		stream_free(stream);
+	}
 }
 
 static void finalize_discovery(struct avdtp *session, int err)
-- 
1.6.3.3

Re: [PATCH 3/3] Fix invalid memory access issues in avdtp module

[Luiz Augusto von Dentz]

Hi Rafal,

On Fri, Jun 10, 2011 at 10:34 PM, Rafal Michalski
<michalski.raf@gmail.com> wrote:
> Changing stream state from STREAMING to IDLE can be associated with side
> effects under some circumstances (such as terminating bluetoothd during
> music is streamed). In this case, after connection is lost, stream state
> changes from STREAMING to IDLE - "avdtp_sep_set_state" is triggered which
> invokes callback called "stream_state_changed" which internally invokes
> "avdtp_sep_set_state" (state of stream doesn't change and stays as IDLE)
> second time and then stream object is freed by "stream_free" at the end
> of "avdtp_sep_set_state". After returning from callback
> "stream_state_changed", first triggered "avdtp_sep_set_state" attempts
> to free stream object again ("if (state == AVDTP_STATE_IDLE)" condition
> is still satisfied) and it leads to invalid read/write/free issues
> (reported by valgrind) in "stream free" body, since "stream" is "alias"
> pointer to stream object which is already out of date (memory for stream
> object has been already freed). Moreover freeing stream object also
> discards its callback list so after finishing iteration on this list
> (in "avdtp_sep_set_state") "l" pointer is out of date and getting
> "l->next" pointer (returned by "g_slist_next(l)") leads to invalid read
> issue.
>
> This patch prevents from this special case by freeing stream object only
> when it is present on streams list and removing from this list when
> stream object would be freed. Additionally to avoid issue with callback
> list loop for this has been modified and "while" used instead of "for"
> loop variant.
> ---
>  audio/avdtp.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/audio/avdtp.c b/audio/avdtp.c
> index 6018c37..19c9fd1 100644
> --- a/audio/avdtp.c
> +++ b/audio/avdtp.c
> @@ -1086,7 +1086,6 @@ static void avdtp_sep_set_state(struct avdtp *session,
>                        g_source_remove(stream->idle_timer);
>                        stream->idle_timer = 0;
>                }
> -               session->streams = g_slist_remove(session->streams, stream);
>                if (session->pending_open == stream)
>                        handle_transport_connect(session, NULL, 0, 0);
>                if (session->req && session->req->stream == stream)
> @@ -1098,13 +1097,18 @@ static void avdtp_sep_set_state(struct avdtp *session,
>                break;
>        }
>
> -       for (l = stream->callbacks; l != NULL; l = g_slist_next(l)) {
> +       l = stream->callbacks;
> +       while (l != NULL) {
>                struct stream_callback *cb = l->data;
> +               l = g_slist_next(l);
>                cb->cb(stream, old_state, state, err_ptr, cb->user_data);
>        }

I guess we it would be better to have this as a separate patch since
it is not exactly the same problem is it? Or this only gets triggered
because of the change bellow? If that is the case Im fine.

> -       if (state == AVDTP_STATE_IDLE)
> +       if (state == AVDTP_STATE_IDLE &&
> +                               g_slist_find(session->streams, stream)) {
> +               session->streams = g_slist_remove(session->streams, stream);
>                stream_free(stream);
> +       }
>  }
>
>  static void finalize_discovery(struct avdtp *session, int err)
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Luiz Augusto von Dentz
Computer Engineer