[Bluez PATCH v1] a2dp: set session to NULL when freeing channel

[Bluez PATCH v1] a2dp: set session to NULL when freeing channel

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

There is a possibility that avdtp_channel is freed when avdtp_setup
is not yet freed. This could cause crashes because setup->chan will
then point to an invalid address.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
Reviewed-by: Michael Sun <michaelfsun@google.com>
Reviewed-by: Alain Michaud <alainm@chromium.org>
---

 profiles/audio/a2dp.c   | 34 ++++++++++++++++++++++++++++------
 profiles/audio/a2dp.h   |  4 ++--
 profiles/audio/avdtp.c  |  6 ++++++
 profiles/audio/sink.c   |  5 ++++-
 profiles/audio/source.c |  5 ++++-
 5 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 624d0d42d2..98cae97b93 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -379,7 +379,8 @@ static void finalize_select(struct a2dp_setup *s)
 		if (!cb->select_cb)
 			continue;
 
-		cb->select_cb(s->session, s->sep, s->caps, cb->user_data);
+		cb->select_cb(s->session, s->sep, s->caps,
+					error_to_errno(s->err), cb->user_data);
 		setup_cb_free(cb);
 	}
 }
@@ -457,7 +458,7 @@ static void stream_state_changed(struct avdtp_stream *stream,
 		int err;
 
 		setup = find_setup_by_stream(stream);
-		if (!setup || !setup->start)
+		if (!setup || !setup->start || setup->err)
 			return;
 
 		setup->start = FALSE;
@@ -606,7 +607,7 @@ static gboolean endpoint_setconf_ind(struct avdtp *session,
 		DBG("Source %p: Set_Configuration_Ind", sep);
 
 	setup = a2dp_setup_get(session);
-	if (!session)
+	if (!setup)
 		return FALSE;
 
 	a2dp_sep->stream = stream;
@@ -715,7 +716,7 @@ static gboolean endpoint_getcap_ind(struct avdtp *session,
 
 static void endpoint_open_cb(struct a2dp_setup *setup, gboolean ret)
 {
-	int err;
+	int err = error_to_errno(setup->err);
 
 	if (ret == FALSE) {
 		setup->stream = NULL;
@@ -723,7 +724,9 @@ static void endpoint_open_cb(struct a2dp_setup *setup, gboolean ret)
 		goto done;
 	}
 
-	err = avdtp_open(setup->session, setup->stream);
+	if (err == 0)
+		err = avdtp_open(setup->session, setup->stream);
+
 	if (err == 0)
 		goto done;
 
@@ -1172,6 +1175,11 @@ static gboolean a2dp_reconfigure(gpointer data)
 
 	setup->id = 0;
 
+	if (setup->err) {
+		posix_err = error_to_errno(setup->err);
+		goto failed;
+	}
+
 	if (!sep->lsep) {
 		error("no valid local SEP");
 		posix_err = -EINVAL;
@@ -1510,6 +1518,7 @@ static void remove_remote_sep(void *data)
 static void channel_free(void *data)
 {
 	struct a2dp_channel *chan = data;
+	struct a2dp_setup *setup;
 
 	if (chan->auth_id > 0)
 		btd_cancel_authorization(chan->auth_id);
@@ -1526,6 +1535,15 @@ static void channel_free(void *data)
 
 	queue_destroy(chan->seps, remove_remote_sep);
 	free(chan->last_used);
+
+	setup = find_setup_by_session(chan->session);
+	if (setup) {
+		setup->chan = NULL;
+		avdtp_unref(setup->session);
+		setup->session = NULL;
+		finalize_setup_errno(setup, -ENOTCONN, NULL);
+	}
+
 	g_free(chan);
 }
 
@@ -2561,6 +2579,9 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
 	struct avdtp_media_codec_capability *codec;
 	int err;
 
+	if (setup->err)
+		goto done;
+
 	if (size >= 0) {
 		caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
 		goto done;
@@ -2727,7 +2748,8 @@ static void discover_cb(struct avdtp *session, GSList *seps,
 	DBG("version 0x%04x err %p", version, err);
 
 	setup->seps = seps;
-	setup->err = err;
+	if (err)
+		setup->err = err;
 
 	if (!err) {
 		g_slist_foreach(seps, register_remote_sep, setup->chan);
diff --git a/profiles/audio/a2dp.h b/profiles/audio/a2dp.h
index b40bd47364..615b641c9a 100644
--- a/profiles/audio/a2dp.h
+++ b/profiles/audio/a2dp.h
@@ -42,8 +42,8 @@ struct a2dp_endpoint {
 
 typedef void (*a2dp_discover_cb_t) (struct avdtp *session, GSList *seps,
 						int err, void *user_data);
-typedef void (*a2dp_select_cb_t) (struct avdtp *session,
-					struct a2dp_sep *sep, GSList *caps,
+typedef void (*a2dp_select_cb_t) (struct avdtp *session, struct a2dp_sep *sep,
+					GSList *caps, int err,
 					void *user_data);
 typedef void (*a2dp_config_cb_t) (struct avdtp *session, struct a2dp_sep *sep,
 					struct avdtp_stream *stream, int err,
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index b8ef5f6824..fa72ef66a9 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -3847,11 +3847,17 @@ uint8_t avdtp_sep_get_seid(struct avdtp_local_sep *sep)
 
 struct btd_adapter *avdtp_get_adapter(struct avdtp *session)
 {
+	if (!session)
+		return NULL;
+
 	return device_get_adapter(session->device);
 }
 
 struct btd_device *avdtp_get_device(struct avdtp *session)
 {
+	if (!session)
+		return NULL;
+
 	return session->device;
 }
 
diff --git a/profiles/audio/sink.c b/profiles/audio/sink.c
index d672670e25..56c4917780 100644
--- a/profiles/audio/sink.c
+++ b/profiles/audio/sink.c
@@ -183,13 +183,16 @@ static void stream_setup_complete(struct avdtp *session, struct a2dp_sep *sep,
 }
 
 static void select_complete(struct avdtp *session, struct a2dp_sep *sep,
-			GSList *caps, void *user_data)
+			GSList *caps, int err, void *user_data)
 {
 	struct sink *sink = user_data;
 	int id;
 
 	sink->connect_id = 0;
 
+	if (err)
+		goto failed;
+
 	id = a2dp_config(session, sep, stream_setup_complete, caps, sink);
 	if (id == 0)
 		goto failed;
diff --git a/profiles/audio/source.c b/profiles/audio/source.c
index c706c928a7..c6009d0ea2 100644
--- a/profiles/audio/source.c
+++ b/profiles/audio/source.c
@@ -180,13 +180,16 @@ static void stream_setup_complete(struct avdtp *session, struct a2dp_sep *sep,
 }
 
 static void select_complete(struct avdtp *session, struct a2dp_sep *sep,
-			GSList *caps, void *user_data)
+			GSList *caps, int err, void *user_data)
 {
 	struct source *source = user_data;
 	int id;
 
 	source->connect_id = 0;
 
+	if (err)
+		goto failed;
+
 	if (caps == NULL)
 		goto failed;
 
-- 
2.30.0.365.g02bc693789-goog

RE: [Bluez,v1] a2dp: set session to NULL when freeing channel

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=428439

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

Re: [Bluez,v1] a2dp: set session to NULL when freeing channel

[Luiz Augusto von Dentz]

Hi Archie,

On Thu, Feb 4, 2021 at 8:44 PM <bluez.test.bot@gmail.com> wrote:
>
> This is automated email and please do not reply to this email!
>
> Dear submitter,
>
> Thank you for submitting the patches to the linux bluetooth mailing list.
> This is a CI test results with your patch series:
> PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=428439
>
> ---Test result---
>
> ##############################
> Test: CheckPatch - PASS
>
> ##############################
> Test: CheckGitLint - PASS
>
> ##############################
> Test: CheckBuild - PASS
>
> ##############################
> Test: MakeCheck - PASS
>
>
>
> ---
> Regards,
> Linux Bluetooth

Applied, thanks.

-- 
Luiz Augusto von Dentz