From: Chan-yeol Park <[email protected]>
When matching remote SEP to local SEP avdtp does not compare vendor
codecs properly, i.e. neither vendor ID not codec ID are checked,
which may cause wrong endpoint to be selected in case there are more
that one endpoints using vendor codec on remote.
This patch fixes it by making avdtp matching a2dp vendor codec by
a2dp's call back.
---
profiles/audio/a2dp.c | 41 +++++++++++++++++++++++++++++++++++++----
profiles/audio/avdtp.c | 7 ++++++-
profiles/audio/avdtp.h | 6 +++++-
3 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 31c5086..291be35 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -925,6 +925,35 @@ static gboolean close_ind(struct avdtp *session, struct avdtp_local_sep *sep,
return TRUE;
}
+static gboolean a2dp_vndcodec_matched(void *remote_vndcodec, void *user_data)
+{
+ struct a2dp_sep *sep = user_data;
+ a2dp_vendor_codec_t *remote_codec = remote_vndcodec;
+ a2dp_vendor_codec_t *local_codec;
+ uint8_t *capabilities;
+ size_t length;
+
+ if (sep->endpoint == NULL)
+ return FALSE;
+
+ length = sep->endpoint->get_capabilities(sep, &capabilities,
+ sep->user_data);
+ if (length < sizeof(a2dp_vendor_codec_t))
+ return FALSE;
+
+ local_codec = (a2dp_vendor_codec_t *) capabilities;
+
+ if (remote_codec->vendor_id != local_codec->vendor_id)
+ return FALSE;
+
+ if (remote_codec->codec_id != local_codec->codec_id)
+ return FALSE;
+
+ DBG("vendor 0x%08x codec 0x%04x", btohl(remote_codec->vendor_id),
+ btohs(remote_codec->codec_id));
+ return TRUE;
+}
+
static gboolean a2dp_reconfigure(gpointer data)
{
struct a2dp_setup *setup = data;
@@ -939,7 +968,8 @@ static gboolean a2dp_reconfigure(gpointer data)
}
if (!setup->rsep || sep->codec != rsep_codec->media_codec_type)
- setup->rsep = avdtp_find_remote_sep(setup->session, sep->lsep);
+ setup->rsep = avdtp_find_remote_sep(setup->session, sep->lsep,
+ a2dp_vndcodec_matched);
posix_err = avdtp_set_configuration(setup->session, setup->rsep,
sep->lsep,
@@ -1769,7 +1799,8 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
continue;
}
- rsep = avdtp_find_remote_sep(session, sep->lsep);
+ rsep = avdtp_find_remote_sep(session, sep->lsep,
+ a2dp_vndcodec_matched);
if (rsep == NULL)
continue;
@@ -1835,7 +1866,8 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
cb_data->user_data = user_data;
setup->sep = sep;
- setup->rsep = avdtp_find_remote_sep(session, sep->lsep);
+ setup->rsep = avdtp_find_remote_sep(session, sep->lsep,
+ a2dp_vndcodec_matched);
if (setup->rsep == NULL) {
error("Could not find remote sep");
@@ -1934,7 +1966,8 @@ unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
break;
}
- setup->rsep = avdtp_find_remote_sep(session, sep->lsep);
+ setup->rsep = avdtp_find_remote_sep(session, sep->lsep,
+ a2dp_vndcodec_matched);
if (setup->rsep == NULL) {
error("No matching ACP and INT SEPs found");
goto failed;
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index f38188f..ab07128 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -1198,7 +1198,8 @@ static struct avdtp_local_sep *find_local_sep_by_seid(struct avdtp *session,
}
struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
- struct avdtp_local_sep *lsep)
+ struct avdtp_local_sep *lsep,
+ avdtp_vndcodec_matched_cb cb)
{
GSList *l;
@@ -1226,6 +1227,10 @@ struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
if (codec_data->media_codec_type != lsep->codec)
continue;
+ if (codec_data->media_codec_type == 0xff && cb)
+ if (cb(codec_data->data, lsep->user_data) == FALSE)
+ continue;
+
if (sep->stream == NULL)
return sep;
}
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index 1b02232..5886691 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -277,9 +277,13 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
struct avdtp_sep_cfm *cfm,
void *user_data);
+typedef gboolean (*avdtp_vndcodec_matched_cb) (void *remote_vndcodec,
+ void *user_data);
+
/* Find a matching pair of local and remote SEP ID's */
struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
- struct avdtp_local_sep *lsep);
+ struct avdtp_local_sep *lsep,
+ avdtp_vndcodec_matched_cb cb);
int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep);
--
2.1.0
Hi, Luiz
On Mon, 2015-05-18 at 17:42 +0300, Luiz Augusto von Dentz wrote:
> Hi Chan-yeol,
>
> On Mon, May 18, 2015 at 4:33 PM, <[email protected]> wrote:
> > From: Chan-yeol Park <[email protected]>
> >
> > When matching remote SEP to local SEP avdtp does not compare vendor
> > codecs properly, i.e. neither vendor ID not codec ID are checked,
> > which may cause wrong endpoint to be selected in case there are more
> > that one endpoints using vendor codec on remote.
> >
> > This patch fixes it by making avdtp matching a2dp vendor codec by
> > a2dp's call back.
> > ---
> > profiles/audio/a2dp.c | 41 +++++++++++++++++++++++++++++++++++++----
> > profiles/audio/avdtp.c | 7 ++++++-
> > profiles/audio/avdtp.h | 6 +++++-
> > 3 files changed, 48 insertions(+), 6 deletions(-)
> >
> > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > index 31c5086..291be35 100644
> > --- a/profiles/audio/a2dp.c
> > +++ b/profiles/audio/a2dp.c
> > @@ -925,6 +925,35 @@ static gboolean close_ind(struct avdtp *session, struct avdtp_local_sep *sep,
> > return TRUE;
> > }
> >
> > +static gboolean a2dp_vndcodec_matched(void *remote_vndcodec, void *user_data)
> > +{
> > + struct a2dp_sep *sep = user_data;
> > + a2dp_vendor_codec_t *remote_codec = remote_vndcodec;
> > + a2dp_vendor_codec_t *local_codec;
> > + uint8_t *capabilities;
> > + size_t length;
> > +
> > + if (sep->endpoint == NULL)
> > + return FALSE;
> > +
> > + length = sep->endpoint->get_capabilities(sep, &capabilities,
> > + sep->user_data);
> > + if (length < sizeof(a2dp_vendor_codec_t))
> > + return FALSE;
> > +
> > + local_codec = (a2dp_vendor_codec_t *) capabilities;
> > +
> > + if (remote_codec->vendor_id != local_codec->vendor_id)
> > + return FALSE;
> > +
> > + if (remote_codec->codec_id != local_codec->codec_id)
> > + return FALSE;
> > +
> > + DBG("vendor 0x%08x codec 0x%04x", btohl(remote_codec->vendor_id),
> > + btohs(remote_codec->codec_id));
> > + return TRUE;
> > +}
>
> That was not what I has initially in mind, anyway Im not against
> adding a callback to interact with avdtp_find_remote_sep but perhaps
> it should happen when registering the endpoint so AVDTP code don't
> need to check if codec is 0xff, instead it just call the callback if
> it exists after checking the codec. That means adding a match callback
> avdtp_sep_ind:
>
> diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
> index 1b02232..2d55a20 100644
> --- a/profiles/audio/avdtp.h
> +++ b/profiles/audio/avdtp.h
> @@ -172,6 +172,8 @@ struct avdtp_sep_cfm {
> /* Callbacks for indicating when we received a new command. The return value
> * indicates whether the command should be rejected or accepted */
> struct avdtp_sep_ind {
> + gboolean (*match) (struct avdtp *session, struct avdtp_local_sep *lsep,
> + struct avdtp_remote_sep *rsep);
> gboolean (*get_capability) (struct avdtp *session,
> struct avdtp_local_sep *sep,
> gboolean get_all,
>
I thought that struct avdtp_sep_ind are strictly related to AVDTP signal
between devices and it should be called from remote command. So I think
a2dp vendor case is little bit different wit them.
But if we use it, I agree call back could be more simpler than adding
callback in avdtp_find_remote_sep().
I would raise v4.
> > static gboolean a2dp_reconfigure(gpointer data)
> > {
> > struct a2dp_setup *setup = data;
> > @@ -939,7 +968,8 @@ static gboolean a2dp_reconfigure(gpointer data)
> > }
> >
> > if (!setup->rsep || sep->codec != rsep_codec->media_codec_type)
> > - setup->rsep = avdtp_find_remote_sep(setup->session, sep->lsep);
> > + setup->rsep = avdtp_find_remote_sep(setup->session, sep->lsep,
> > + a2dp_vndcodec_matched);
> >
> > posix_err = avdtp_set_configuration(setup->session, setup->rsep,
> > sep->lsep,
> > @@ -1769,7 +1799,8 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> > continue;
> > }
> >
> > - rsep = avdtp_find_remote_sep(session, sep->lsep);
> > + rsep = avdtp_find_remote_sep(session, sep->lsep,
> > + a2dp_vndcodec_matched);
> > if (rsep == NULL)
> > continue;
> >
> > @@ -1835,7 +1866,8 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> > cb_data->user_data = user_data;
> >
> > setup->sep = sep;
> > - setup->rsep = avdtp_find_remote_sep(session, sep->lsep);
> > + setup->rsep = avdtp_find_remote_sep(session, sep->lsep,
> > + a2dp_vndcodec_matched);
> >
> > if (setup->rsep == NULL) {
> > error("Could not find remote sep");
> > @@ -1934,7 +1966,8 @@ unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
> > break;
> > }
> >
> > - setup->rsep = avdtp_find_remote_sep(session, sep->lsep);
> > + setup->rsep = avdtp_find_remote_sep(session, sep->lsep,
> > + a2dp_vndcodec_matched);
> > if (setup->rsep == NULL) {
> > error("No matching ACP and INT SEPs found");
> > goto failed;
> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> > index f38188f..ab07128 100644
> > --- a/profiles/audio/avdtp.c
> > +++ b/profiles/audio/avdtp.c
> > @@ -1198,7 +1198,8 @@ static struct avdtp_local_sep *find_local_sep_by_seid(struct avdtp *session,
> > }
> >
> > struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
> > - struct avdtp_local_sep *lsep)
> > + struct avdtp_local_sep *lsep,
> > + avdtp_vndcodec_matched_cb cb)
> > {
> > GSList *l;
> >
> > @@ -1226,6 +1227,10 @@ struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
> > if (codec_data->media_codec_type != lsep->codec)
> > continue;
> >
> > + if (codec_data->media_codec_type == 0xff && cb)
> > + if (cb(codec_data->data, lsep->user_data) == FALSE)
> > + continue;
> > +
> > if (sep->stream == NULL)
> > return sep;
> > }
> > diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
> > index 1b02232..5886691 100644
> > --- a/profiles/audio/avdtp.h
> > +++ b/profiles/audio/avdtp.h
> > @@ -277,9 +277,13 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> > struct avdtp_sep_cfm *cfm,
> > void *user_data);
> >
> > +typedef gboolean (*avdtp_vndcodec_matched_cb) (void *remote_vndcodec,
> > + void *user_data);
> > +
> > /* Find a matching pair of local and remote SEP ID's */
> > struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
> > - struct avdtp_local_sep *lsep);
> > + struct avdtp_local_sep *lsep,
> > + avdtp_vndcodec_matched_cb cb);
> >
> > int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep);
> >
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
Thanks!
Chanyeol
Hi Chan-yeol,
On Mon, May 18, 2015 at 4:33 PM, <[email protected]> wrote:
> From: Chan-yeol Park <[email protected]>
>
> When matching remote SEP to local SEP avdtp does not compare vendor
> codecs properly, i.e. neither vendor ID not codec ID are checked,
> which may cause wrong endpoint to be selected in case there are more
> that one endpoints using vendor codec on remote.
>
> This patch fixes it by making avdtp matching a2dp vendor codec by
> a2dp's call back.
> ---
> profiles/audio/a2dp.c | 41 +++++++++++++++++++++++++++++++++++++----
> profiles/audio/avdtp.c | 7 ++++++-
> profiles/audio/avdtp.h | 6 +++++-
> 3 files changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index 31c5086..291be35 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -925,6 +925,35 @@ static gboolean close_ind(struct avdtp *session, struct avdtp_local_sep *sep,
> return TRUE;
> }
>
> +static gboolean a2dp_vndcodec_matched(void *remote_vndcodec, void *user_data)
> +{
> + struct a2dp_sep *sep = user_data;
> + a2dp_vendor_codec_t *remote_codec = remote_vndcodec;
> + a2dp_vendor_codec_t *local_codec;
> + uint8_t *capabilities;
> + size_t length;
> +
> + if (sep->endpoint == NULL)
> + return FALSE;
> +
> + length = sep->endpoint->get_capabilities(sep, &capabilities,
> + sep->user_data);
> + if (length < sizeof(a2dp_vendor_codec_t))
> + return FALSE;
> +
> + local_codec = (a2dp_vendor_codec_t *) capabilities;
> +
> + if (remote_codec->vendor_id != local_codec->vendor_id)
> + return FALSE;
> +
> + if (remote_codec->codec_id != local_codec->codec_id)
> + return FALSE;
> +
> + DBG("vendor 0x%08x codec 0x%04x", btohl(remote_codec->vendor_id),
> + btohs(remote_codec->codec_id));
> + return TRUE;
> +}
That was not what I has initially in mind, anyway Im not against
adding a callback to interact with avdtp_find_remote_sep but perhaps
it should happen when registering the endpoint so AVDTP code don't
need to check if codec is 0xff, instead it just call the callback if
it exists after checking the codec. That means adding a match callback
avdtp_sep_ind:
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index 1b02232..2d55a20 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -172,6 +172,8 @@ struct avdtp_sep_cfm {
/* Callbacks for indicating when we received a new command. The return value
* indicates whether the command should be rejected or accepted */
struct avdtp_sep_ind {
+ gboolean (*match) (struct avdtp *session, struct avdtp_local_sep *lsep,
+ struct avdtp_remote_sep *rsep);
gboolean (*get_capability) (struct avdtp *session,
struct avdtp_local_sep *sep,
gboolean get_all,
> static gboolean a2dp_reconfigure(gpointer data)
> {
> struct a2dp_setup *setup = data;
> @@ -939,7 +968,8 @@ static gboolean a2dp_reconfigure(gpointer data)
> }
>
> if (!setup->rsep || sep->codec != rsep_codec->media_codec_type)
> - setup->rsep = avdtp_find_remote_sep(setup->session, sep->lsep);
> + setup->rsep = avdtp_find_remote_sep(setup->session, sep->lsep,
> + a2dp_vndcodec_matched);
>
> posix_err = avdtp_set_configuration(setup->session, setup->rsep,
> sep->lsep,
> @@ -1769,7 +1799,8 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> continue;
> }
>
> - rsep = avdtp_find_remote_sep(session, sep->lsep);
> + rsep = avdtp_find_remote_sep(session, sep->lsep,
> + a2dp_vndcodec_matched);
> if (rsep == NULL)
> continue;
>
> @@ -1835,7 +1866,8 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> cb_data->user_data = user_data;
>
> setup->sep = sep;
> - setup->rsep = avdtp_find_remote_sep(session, sep->lsep);
> + setup->rsep = avdtp_find_remote_sep(session, sep->lsep,
> + a2dp_vndcodec_matched);
>
> if (setup->rsep == NULL) {
> error("Could not find remote sep");
> @@ -1934,7 +1966,8 @@ unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
> break;
> }
>
> - setup->rsep = avdtp_find_remote_sep(session, sep->lsep);
> + setup->rsep = avdtp_find_remote_sep(session, sep->lsep,
> + a2dp_vndcodec_matched);
> if (setup->rsep == NULL) {
> error("No matching ACP and INT SEPs found");
> goto failed;
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index f38188f..ab07128 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -1198,7 +1198,8 @@ static struct avdtp_local_sep *find_local_sep_by_seid(struct avdtp *session,
> }
>
> struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
> - struct avdtp_local_sep *lsep)
> + struct avdtp_local_sep *lsep,
> + avdtp_vndcodec_matched_cb cb)
> {
> GSList *l;
>
> @@ -1226,6 +1227,10 @@ struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
> if (codec_data->media_codec_type != lsep->codec)
> continue;
>
> + if (codec_data->media_codec_type == 0xff && cb)
> + if (cb(codec_data->data, lsep->user_data) == FALSE)
> + continue;
> +
> if (sep->stream == NULL)
> return sep;
> }
> diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
> index 1b02232..5886691 100644
> --- a/profiles/audio/avdtp.h
> +++ b/profiles/audio/avdtp.h
> @@ -277,9 +277,13 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> struct avdtp_sep_cfm *cfm,
> void *user_data);
>
> +typedef gboolean (*avdtp_vndcodec_matched_cb) (void *remote_vndcodec,
> + void *user_data);
> +
> /* Find a matching pair of local and remote SEP ID's */
> struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
> - struct avdtp_local_sep *lsep);
> + struct avdtp_local_sep *lsep,
> + avdtp_vndcodec_matched_cb cb);
>
> int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep);
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
From: Chan-yeol Park <[email protected]>
This function could be removed because A2DP vendor codec match is
already verified in avdtp_find_remote_sep().
---
profiles/audio/a2dp.c | 54 +++------------------------------------------------
1 file changed, 3 insertions(+), 51 deletions(-)
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 291be35..4ff5969 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1742,50 +1742,11 @@ done:
finalize_select(setup);
}
-static gboolean check_vendor_codec(struct a2dp_sep *sep, uint8_t *cap,
- size_t len)
-{
- uint8_t *capabilities;
- size_t length;
- a2dp_vendor_codec_t *local_codec;
- a2dp_vendor_codec_t *remote_codec;
-
- if (len < sizeof(a2dp_vendor_codec_t))
- return FALSE;
-
- remote_codec = (a2dp_vendor_codec_t *) cap;
-
- if (sep->endpoint == NULL)
- return FALSE;
-
- length = sep->endpoint->get_capabilities(sep,
- &capabilities, sep->user_data);
-
- if (length < sizeof(a2dp_vendor_codec_t))
- return FALSE;
-
- local_codec = (a2dp_vendor_codec_t *) capabilities;
-
- if (btohl(remote_codec->vendor_id) != btohl(local_codec->vendor_id))
- return FALSE;
-
- if (btohs(remote_codec->codec_id) != btohs(local_codec->codec_id))
- return FALSE;
-
- DBG("vendor 0x%08x codec 0x%04x", btohl(remote_codec->vendor_id),
- btohs(remote_codec->codec_id));
-
- return TRUE;
-}
-
static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
const char *sender)
{
for (; list; list = list->next) {
struct a2dp_sep *sep = list->data;
- struct avdtp_remote_sep *rsep;
- struct avdtp_media_codec_capability *cap;
- struct avdtp_service_capability *service;
/* Use sender's endpoint if available */
if (sender) {
@@ -1799,20 +1760,11 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
continue;
}
- rsep = avdtp_find_remote_sep(session, sep->lsep,
- a2dp_vndcodec_matched);
- if (rsep == NULL)
+ if (avdtp_find_remote_sep(session, sep->lsep,
+ a2dp_vndcodec_matched) == NULL)
continue;
- service = avdtp_get_codec(rsep);
- cap = (struct avdtp_media_codec_capability *) service->data;
-
- if (cap->media_codec_type != A2DP_CODEC_VENDOR)
- return sep;
-
- if (check_vendor_codec(sep, cap->data,
- service->length - sizeof(*cap)))
- return sep;
+ return sep;
}
return NULL;
--
2.1.0