Skip to contentSkip to conversation

a2dp: Fix crash when SEP codec has not been initialized

PATCH Superseded Frédéric Danis 3 participants 4 messages Mar 25, 2022 – Mar 28, 2022 Bluetooth drivers

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Frédéric Danis

author
#0 lore raw

PATCH 1/1 a2dp: Fix crash when SEP codec has not been initialized

If SEP has not been properly discovered avdtp_get_codec may return NULL thus causing crashes such as when running AVRCP/TG/VLH/BI-01-C after AVRCP/TG/RCR/BV-04-C

---
 profiles/audio/a2dp.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
profiles/audio/a2dp.c +14 -3
index f761dbe54..7da008071 100644
@@ -1995,7 +1995,12 @@ static gboolean get_codec(const GDBusPropertyTable *property,
 {
 	struct a2dp_remote_sep *sep = data;
 	struct avdtp_service_capability *cap = avdtp_get_codec(sep->sep);
-	struct avdtp_media_codec_capability *codec = (void *) cap->data;
+	struct avdtp_media_codec_capability *codec;
+
+	if (!cap)
+		return FALSE;
+
+	codec = (void *) cap->data;
 
 	dbus_message_iter_append_basic(iter, DBUS_TYPE_BYTE,
 						&codec->media_codec_type);

@@ -2008,10 +2013,16 @@ static gboolean get_capabilities(const GDBusPropertyTable *property,
 {
 	struct a2dp_remote_sep *sep = data;
 	struct avdtp_service_capability *service = avdtp_get_codec(sep->sep);
-	struct avdtp_media_codec_capability *codec = (void *) service->data;
-	uint8_t *caps = codec->data;
+	struct avdtp_media_codec_capability *codec;
+	uint8_t *caps;
 	DBusMessageIter array;
 
+	if (!service)
+		return FALSE;
+
+	codec = (void *) service->data;
+	caps = codec->data;
+
 	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
 					DBUS_TYPE_BYTE_AS_STRING, &array);
Signature · 1 line
2.25.1
#1 lore raw

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=626269

---Test result---

Test Summary:

CheckPatch                    PASS      1.59 seconds
GitLint                       PASS      1.10 seconds
Prep - Setup ELL              PASS      52.73 seconds
Build - Prep                  PASS      0.92 seconds
Build - Configure             PASS      10.45 seconds
Build - Make                  PASS      1841.06 seconds
Make Check                    PASS      12.83 seconds
Make Check w/Valgrind         PASS      544.85 seconds
Make Distcheck                PASS      292.04 seconds
Build w/ext ELL - Configure   PASS      10.58 seconds
Build w/ext ELL - Make        PASS      1798.41 seconds
Incremental Build with patchesPASS      0.00 seconds

---
Regards,
Linux Bluetooth

maintainer
#2 lore raw
In reply to Frédéric Danis

Hi Frédéric,

Quoted 46 lines · Frédéric Danis

If SEP has not been properly discovered avdtp_get_codec may return NULL thus causing crashes such as when running AVRCP/TG/VLH/BI-01-C after AVRCP/TG/RCR/BV-04-C

---
 profiles/audio/a2dp.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
profiles/audio/a2dp.c +14 -4
index f761dbe54..7da008071 100644
@@ -1995,7 +1995,12 @@ static gboolean get_codec(const GDBusPropertyTable *property,
 {
        struct a2dp_remote_sep *sep = data;
        struct avdtp_service_capability *cap = avdtp_get_codec(sep->sep);
-       struct avdtp_media_codec_capability *codec = (void *) cap->data;
+       struct avdtp_media_codec_capability *codec;
+
+       if (!cap)
+               return FALSE;
+
+       codec = (void *) cap->data;
 
        dbus_message_iter_append_basic(iter, DBUS_TYPE_BYTE,
                                                &codec->media_codec_type);

@@ -2008,10 +2013,16 @@ static gboolean get_capabilities(const GDBusPropertyTable *property,
 {
        struct a2dp_remote_sep *sep = data;
        struct avdtp_service_capability *service = avdtp_get_codec(sep->sep);
-       struct avdtp_media_codec_capability *codec = (void *) service->data;
-       uint8_t *caps = codec->data;
+       struct avdtp_media_codec_capability *codec;
+       uint8_t *caps;
        DBusMessageIter array;
 
+       if (!service)
+               return FALSE;
+
+       codec = (void *) service->data;
+       caps = codec->data;
+
        dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
                                        DBUS_TYPE_BYTE_AS_STRING, &array);
 
--
Quoted 1 line

2.25.1

We should either have a .exist callback or not have the endpoint registered if its codec is not available, I'm leaning toward the latter given that it is useless to have the endpoint if it cannot be used without the codec information.

Signature · 1 line
Luiz Augusto von Dentz
maintainer
#3 lore raw

Hi Frédéric,

Quoted 50 lines · Luiz Augusto von Dentz

Hi Frédéric,

Quoted 45 lines · Frédéric Danis

If SEP has not been properly discovered avdtp_get_codec may return NULL thus causing crashes such as when running AVRCP/TG/VLH/BI-01-C after AVRCP/TG/RCR/BV-04-C

---
 profiles/audio/a2dp.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
profiles/audio/a2dp.c +14 -3
index f761dbe54..7da008071 100644
@@ -1995,7 +1995,12 @@ static gboolean get_codec(const GDBusPropertyTable *property,
 {
        struct a2dp_remote_sep *sep = data;
        struct avdtp_service_capability *cap = avdtp_get_codec(sep->sep);
-       struct avdtp_media_codec_capability *codec = (void *) cap->data;
+       struct avdtp_media_codec_capability *codec;
+
+       if (!cap)
+               return FALSE;
+
+       codec = (void *) cap->data;
 
        dbus_message_iter_append_basic(iter, DBUS_TYPE_BYTE,
                                                &codec->media_codec_type);

@@ -2008,10 +2013,16 @@ static gboolean get_capabilities(const GDBusPropertyTable *property,
 {
        struct a2dp_remote_sep *sep = data;
        struct avdtp_service_capability *service = avdtp_get_codec(sep->sep);
-       struct avdtp_media_codec_capability *codec = (void *) service->data;
-       uint8_t *caps = codec->data;
+       struct avdtp_media_codec_capability *codec;
+       uint8_t *caps;
        DBusMessageIter array;
 
+       if (!service)
+               return FALSE;
+
+       codec = (void *) service->data;
+       caps = codec->data;
+
        dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
                                        DBUS_TYPE_BYTE_AS_STRING, &array);
Quoted 7 lines
Quoted 2 lines

--
2.25.1

We should either have a .exist callback or not have the endpoint registered if its codec is not available, I'm leaning toward the latter given that it is useless to have the endpoint if it cannot be used without the codec information.

In case you missed my response on slack, here is the suggestion change:

profiles/audio/a2dp.c +0 -0
index c3ac432a7..28654924b 100644
@@ -2074,6 +2074,11 @@ static struct a2dp_remote_sep

*register_remote_sep(void *data, void *user_data)

        if (sep)
                return sep;

+ if (avdtp_get_codec(rsep)) {
+ error("Unable to get remote sep codec");
+ return NULL;
+ }
+

        sep = new0(struct a2dp_remote_sep, 1);
        sep->chan = chan;
        sep->sep = rsep;
Quoted 5 lines

--
Luiz Augusto von Dentz

Signature · 1 line
Luiz Augusto von Dentz