* [Bluez PATCH v1] src/profile: Ensure class UUID matches before connecting profile
@ 2020-02-21 8:39 Archie Pusaka
2020-02-24 19:29 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 5+ messages in thread
From: Archie Pusaka @ 2020-02-21 8:39 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Archie Pusaka
From: Archie Pusaka <apusaka@chromium.org>
According to bluetooth spec Ver 5.1, Vol 3, Part B, 4.7.2, there
might be multiple service records returned in a SDP Service Search
Attribute Response. Also, according to 2.5.2, the service pattern
can match any UUID contained within the service record, it doesn't
have to match only some specific attributes of the record.
Therefore, before using the service record to connect to any
profile, first we must check that the service class ID of the
service record matches with whatever UUID specified in the service
pattern we are looking for.
---
src/profile.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/src/profile.c b/src/profile.c
index 192fd0245..1b481836e 100644
--- a/src/profile.c
+++ b/src/profile.c
@@ -1568,8 +1568,34 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
for (r = recs; r != NULL; r = r->next) {
sdp_record_t *rec = r->data;
+ sdp_list_t *svcclass;
+ sdp_list_t *svcclass_iter;
sdp_list_t *protos;
int port;
+ bool matches_class_uuid = false;
+
+ if (sdp_get_service_classes(rec, &svcclass) < 0) {
+ error("Unable to get svc class ID list from %s record",
+ ext->name);
+ continue;
+ }
+
+ for (svcclass_iter = svcclass; svcclass_iter != NULL;
+ svcclass_iter = svcclass_iter->next) {
+ char *uuid = bt_uuid2string(svcclass_iter->data);
+ int cmp_result = bt_uuid_strcmp(uuid, ext->uuid);
+
+ free(uuid);
+ if (cmp_result == 0) {
+ matches_class_uuid = true;
+ break;
+ }
+ }
+
+ sdp_list_free(svcclass, free);
+
+ if (!matches_class_uuid)
+ continue;
if (sdp_get_access_protos(rec, &protos) < 0) {
error("Unable to get proto list from %s record",
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Bluez PATCH v1] src/profile: Ensure class UUID matches before connecting profile
2020-02-21 8:39 [Bluez PATCH v1] src/profile: Ensure class UUID matches before connecting profile Archie Pusaka
@ 2020-02-24 19:29 ` Luiz Augusto von Dentz
2020-02-25 5:58 ` Archie Pusaka
0 siblings, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2020-02-24 19:29 UTC (permalink / raw)
To: Archie Pusaka; +Cc: linux-bluetooth, Archie Pusaka
Hi Archie,
On Fri, Feb 21, 2020 at 12:41 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> According to bluetooth spec Ver 5.1, Vol 3, Part B, 4.7.2, there
> might be multiple service records returned in a SDP Service Search
> Attribute Response. Also, according to 2.5.2, the service pattern
> can match any UUID contained within the service record, it doesn't
> have to match only some specific attributes of the record.
>
> Therefore, before using the service record to connect to any
> profile, first we must check that the service class ID of the
> service record matches with whatever UUID specified in the service
> pattern we are looking for.
Im surprised we were not doing this currently, Im fairly sure we do
that for the services/plugin though since there are only probed if the
service UUID matches
> ---
>
> src/profile.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/src/profile.c b/src/profile.c
> index 192fd0245..1b481836e 100644
> --- a/src/profile.c
> +++ b/src/profile.c
> @@ -1568,8 +1568,34 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
>
> for (r = recs; r != NULL; r = r->next) {
> sdp_record_t *rec = r->data;
> + sdp_list_t *svcclass;
> + sdp_list_t *svcclass_iter;
> sdp_list_t *protos;
> int port;
> + bool matches_class_uuid = false;
> +
> + if (sdp_get_service_classes(rec, &svcclass) < 0) {
> + error("Unable to get svc class ID list from %s record",
> + ext->name);
> + continue;
> + }
> +
> + for (svcclass_iter = svcclass; svcclass_iter != NULL;
> + svcclass_iter = svcclass_iter->next) {
> + char *uuid = bt_uuid2string(svcclass_iter->data);
> + int cmp_result = bt_uuid_strcmp(uuid, ext->uuid);
I think it would be probably more efficient to convert to data to
binary format (bt_uuid_t) and then do the comparision with
bt_uuid_cmp, also there might not be needed to iterate at all see
device.c:update_bredr_service which has the logic for updating
records:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/device.c#n4602
Btw, we should probably have bt_search_service doing the matching if
the uuid is set instead of returning all records like it seems to be
doing that way we don't have to maintain duplicate logic in both
device.c and profile.c
> + free(uuid);
> + if (cmp_result == 0) {
> + matches_class_uuid = true;
> + break;
> + }
> + }
> +
> + sdp_list_free(svcclass, free);
> +
> + if (!matches_class_uuid)
> + continue;
>
> if (sdp_get_access_protos(rec, &protos) < 0) {
> error("Unable to get proto list from %s record",
> --
> 2.25.0.265.gbab2e86ba0-goog
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Bluez PATCH v1] src/profile: Ensure class UUID matches before connecting profile
2020-02-24 19:29 ` Luiz Augusto von Dentz
@ 2020-02-25 5:58 ` Archie Pusaka
2020-02-25 6:59 ` Archie Pusaka
0 siblings, 1 reply; 5+ messages in thread
From: Archie Pusaka @ 2020-02-25 5:58 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
On Tue, 25 Feb 2020 at 03:30, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Fri, Feb 21, 2020 at 12:41 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > According to bluetooth spec Ver 5.1, Vol 3, Part B, 4.7.2, there
> > might be multiple service records returned in a SDP Service Search
> > Attribute Response. Also, according to 2.5.2, the service pattern
> > can match any UUID contained within the service record, it doesn't
> > have to match only some specific attributes of the record.
> >
> > Therefore, before using the service record to connect to any
> > profile, first we must check that the service class ID of the
> > service record matches with whatever UUID specified in the service
> > pattern we are looking for.
>
> Im surprised we were not doing this currently, Im fairly sure we do
> that for the services/plugin though since there are only probed if the
> service UUID matches
>
> > ---
> >
> > src/profile.c | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/src/profile.c b/src/profile.c
> > index 192fd0245..1b481836e 100644
> > --- a/src/profile.c
> > +++ b/src/profile.c
> > @@ -1568,8 +1568,34 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
> >
> > for (r = recs; r != NULL; r = r->next) {
> > sdp_record_t *rec = r->data;
> > + sdp_list_t *svcclass;
> > + sdp_list_t *svcclass_iter;
> > sdp_list_t *protos;
> > int port;
> > + bool matches_class_uuid = false;
> > +
> > + if (sdp_get_service_classes(rec, &svcclass) < 0) {
> > + error("Unable to get svc class ID list from %s record",
> > + ext->name);
> > + continue;
> > + }
> > +
> > + for (svcclass_iter = svcclass; svcclass_iter != NULL;
> > + svcclass_iter = svcclass_iter->next) {
> > + char *uuid = bt_uuid2string(svcclass_iter->data);
> > + int cmp_result = bt_uuid_strcmp(uuid, ext->uuid);
>
> I think it would be probably more efficient to convert to data to
> binary format (bt_uuid_t) and then do the comparision with
> bt_uuid_cmp, also there might not be needed to iterate at all see
> device.c:update_bredr_service which has the logic for updating
> records:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/device.c#n4602
I was uncertain whether we need to iterate or not, so I went for the safer side.
However if there is no need to do that then this shall be updated.
I also agree that comparison in binary should be faster.
>
> Btw, we should probably have bt_search_service doing the matching if
> the uuid is set instead of returning all records like it seems to be
> doing that way we don't have to maintain duplicate logic in both
> device.c and profile.c
Got it. somewhere in sdp-client.c is a good idea.
Aside from device.c and profile.c, is this also used somewhere else
though? We might also want to take them out if we are to implement
the check in sdp-client.c.
>
>
> > + free(uuid);
> > + if (cmp_result == 0) {
> > + matches_class_uuid = true;
> > + break;
> > + }
> > + }
> > +
> > + sdp_list_free(svcclass, free);
> > +
> > + if (!matches_class_uuid)
> > + continue;
> >
> > if (sdp_get_access_protos(rec, &protos) < 0) {
> > error("Unable to get proto list from %s record",
> > --
> > 2.25.0.265.gbab2e86ba0-goog
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Archie
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Bluez PATCH v1] src/profile: Ensure class UUID matches before connecting profile
2020-02-25 5:58 ` Archie Pusaka
@ 2020-02-25 6:59 ` Archie Pusaka
2020-02-25 8:04 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 5+ messages in thread
From: Archie Pusaka @ 2020-02-25 6:59 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
After looking at the code for device.c you pointed out,
apparently the function update_bredr_services() is called two times
upon connecting to a device, the first one for UUID 0x0100 (L2CAP),
then the second one for UUID 0x1200 (PNP).
I am not clear as to why BlueZ needs to search for those two records,
but this might impacted the decision to move the service class ID
filtering to sdp-client.c, because the service class ID of the records
will never match 0x0100, so it will all be filtered out.
Could you confirm that it is okay to filter all records based on the
service class ID, especially considering all records related to L2CAP
query will not execute this part of the code?
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/device.c#n4630
Thanks,
Archie
On Tue, 25 Feb 2020 at 13:58, Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> On Tue, 25 Feb 2020 at 03:30, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Fri, Feb 21, 2020 at 12:41 AM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > According to bluetooth spec Ver 5.1, Vol 3, Part B, 4.7.2, there
> > > might be multiple service records returned in a SDP Service Search
> > > Attribute Response. Also, according to 2.5.2, the service pattern
> > > can match any UUID contained within the service record, it doesn't
> > > have to match only some specific attributes of the record.
> > >
> > > Therefore, before using the service record to connect to any
> > > profile, first we must check that the service class ID of the
> > > service record matches with whatever UUID specified in the service
> > > pattern we are looking for.
> >
> > Im surprised we were not doing this currently, Im fairly sure we do
> > that for the services/plugin though since there are only probed if the
> > service UUID matches
> >
> > > ---
> > >
> > > src/profile.c | 26 ++++++++++++++++++++++++++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/src/profile.c b/src/profile.c
> > > index 192fd0245..1b481836e 100644
> > > --- a/src/profile.c
> > > +++ b/src/profile.c
> > > @@ -1568,8 +1568,34 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
> > >
> > > for (r = recs; r != NULL; r = r->next) {
> > > sdp_record_t *rec = r->data;
> > > + sdp_list_t *svcclass;
> > > + sdp_list_t *svcclass_iter;
> > > sdp_list_t *protos;
> > > int port;
> > > + bool matches_class_uuid = false;
> > > +
> > > + if (sdp_get_service_classes(rec, &svcclass) < 0) {
> > > + error("Unable to get svc class ID list from %s record",
> > > + ext->name);
> > > + continue;
> > > + }
> > > +
> > > + for (svcclass_iter = svcclass; svcclass_iter != NULL;
> > > + svcclass_iter = svcclass_iter->next) {
> > > + char *uuid = bt_uuid2string(svcclass_iter->data);
> > > + int cmp_result = bt_uuid_strcmp(uuid, ext->uuid);
> >
> > I think it would be probably more efficient to convert to data to
> > binary format (bt_uuid_t) and then do the comparision with
> > bt_uuid_cmp, also there might not be needed to iterate at all see
> > device.c:update_bredr_service which has the logic for updating
> > records:
> >
> > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/device.c#n4602
>
> I was uncertain whether we need to iterate or not, so I went for the safer side.
> However if there is no need to do that then this shall be updated.
> I also agree that comparison in binary should be faster.
>
> >
> > Btw, we should probably have bt_search_service doing the matching if
> > the uuid is set instead of returning all records like it seems to be
> > doing that way we don't have to maintain duplicate logic in both
> > device.c and profile.c
>
> Got it. somewhere in sdp-client.c is a good idea.
> Aside from device.c and profile.c, is this also used somewhere else
> though? We might also want to take them out if we are to implement
> the check in sdp-client.c.
>
> >
> >
> > > + free(uuid);
> > > + if (cmp_result == 0) {
> > > + matches_class_uuid = true;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + sdp_list_free(svcclass, free);
> > > +
> > > + if (!matches_class_uuid)
> > > + continue;
> > >
> > > if (sdp_get_access_protos(rec, &protos) < 0) {
> > > error("Unable to get proto list from %s record",
> > > --
> > > 2.25.0.265.gbab2e86ba0-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
> Thanks,
> Archie
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Bluez PATCH v1] src/profile: Ensure class UUID matches before connecting profile
2020-02-25 6:59 ` Archie Pusaka
@ 2020-02-25 8:04 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2020-02-25 8:04 UTC (permalink / raw)
To: Archie Pusaka; +Cc: linux-bluetooth
Hi Archie,
On Mon, Feb 24, 2020 at 10:59 PM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> After looking at the code for device.c you pointed out,
> apparently the function update_bredr_services() is called two times
> upon connecting to a device, the first one for UUID 0x0100 (L2CAP),
> then the second one for UUID 0x1200 (PNP).
>
> I am not clear as to why BlueZ needs to search for those two records,
> but this might impacted the decision to move the service class ID
> filtering to sdp-client.c, because the service class ID of the records
> will never match 0x0100, so it will all be filtered out.
Right that is looking for all records basically, since L2CAP is
included everytime, so we need to keep that working, so perhaps a
better alternative would be to have a second helper function that does
the filtering or extend bt_search_service with another parameter, but
I guess doing the former is cleaner, though I rename bt_search_service
to just bt_search and the later since that doesn't do any filtering
and leave bt_search_service to actually filter out base on service
class.
> Could you confirm that it is okay to filter all records based on the
> service class ID, especially considering all records related to L2CAP
> query will not execute this part of the code?
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/device.c#n4630
>
> Thanks,
> Archie
>
> On Tue, 25 Feb 2020 at 13:58, Archie Pusaka <apusaka@google.com> wrote:
> >
> > Hi Luiz,
> >
> > On Tue, 25 Feb 2020 at 03:30, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Fri, Feb 21, 2020 at 12:41 AM Archie Pusaka <apusaka@google.com> wrote:
> > > >
> > > > From: Archie Pusaka <apusaka@chromium.org>
> > > >
> > > > According to bluetooth spec Ver 5.1, Vol 3, Part B, 4.7.2, there
> > > > might be multiple service records returned in a SDP Service Search
> > > > Attribute Response. Also, according to 2.5.2, the service pattern
> > > > can match any UUID contained within the service record, it doesn't
> > > > have to match only some specific attributes of the record.
> > > >
> > > > Therefore, before using the service record to connect to any
> > > > profile, first we must check that the service class ID of the
> > > > service record matches with whatever UUID specified in the service
> > > > pattern we are looking for.
> > >
> > > Im surprised we were not doing this currently, Im fairly sure we do
> > > that for the services/plugin though since there are only probed if the
> > > service UUID matches
> > >
> > > > ---
> > > >
> > > > src/profile.c | 26 ++++++++++++++++++++++++++
> > > > 1 file changed, 26 insertions(+)
> > > >
> > > > diff --git a/src/profile.c b/src/profile.c
> > > > index 192fd0245..1b481836e 100644
> > > > --- a/src/profile.c
> > > > +++ b/src/profile.c
> > > > @@ -1568,8 +1568,34 @@ static void record_cb(sdp_list_t *recs, int err, gpointer user_data)
> > > >
> > > > for (r = recs; r != NULL; r = r->next) {
> > > > sdp_record_t *rec = r->data;
> > > > + sdp_list_t *svcclass;
> > > > + sdp_list_t *svcclass_iter;
> > > > sdp_list_t *protos;
> > > > int port;
> > > > + bool matches_class_uuid = false;
> > > > +
> > > > + if (sdp_get_service_classes(rec, &svcclass) < 0) {
> > > > + error("Unable to get svc class ID list from %s record",
> > > > + ext->name);
> > > > + continue;
> > > > + }
> > > > +
> > > > + for (svcclass_iter = svcclass; svcclass_iter != NULL;
> > > > + svcclass_iter = svcclass_iter->next) {
> > > > + char *uuid = bt_uuid2string(svcclass_iter->data);
> > > > + int cmp_result = bt_uuid_strcmp(uuid, ext->uuid);
> > >
> > > I think it would be probably more efficient to convert to data to
> > > binary format (bt_uuid_t) and then do the comparision with
> > > bt_uuid_cmp, also there might not be needed to iterate at all see
> > > device.c:update_bredr_service which has the logic for updating
> > > records:
> > >
> > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/device.c#n4602
> >
> > I was uncertain whether we need to iterate or not, so I went for the safer side.
> > However if there is no need to do that then this shall be updated.
> > I also agree that comparison in binary should be faster.
> >
> > >
> > > Btw, we should probably have bt_search_service doing the matching if
> > > the uuid is set instead of returning all records like it seems to be
> > > doing that way we don't have to maintain duplicate logic in both
> > > device.c and profile.c
> >
> > Got it. somewhere in sdp-client.c is a good idea.
> > Aside from device.c and profile.c, is this also used somewhere else
> > though? We might also want to take them out if we are to implement
> > the check in sdp-client.c.
> >
> > >
> > >
> > > > + free(uuid);
> > > > + if (cmp_result == 0) {
> > > > + matches_class_uuid = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + sdp_list_free(svcclass, free);
> > > > +
> > > > + if (!matches_class_uuid)
> > > > + continue;
> > > >
> > > > if (sdp_get_access_protos(rec, &protos) < 0) {
> > > > error("Unable to get proto list from %s record",
> > > > --
> > > > 2.25.0.265.gbab2e86ba0-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> >
> > Thanks,
> > Archie
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-25 8:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-21 8:39 [Bluez PATCH v1] src/profile: Ensure class UUID matches before connecting profile Archie Pusaka
2020-02-24 19:29 ` Luiz Augusto von Dentz
2020-02-25 5:58 ` Archie Pusaka
2020-02-25 6:59 ` Archie Pusaka
2020-02-25 8:04 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).