Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200221163911.Bluez.v1.1.I3c505b4307094eb7a6f2c5949125a17c89b2e099@changeid>
In-Reply-To: <20200221163911.Bluez.v1.1.I3c505b4307094eb7a6f2c5949125a17c89b2e099@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 24 Feb 2020 11:29:53 -0800
Message-ID: <CABBYNZKc6UPfYLbesAk0fUaGWFHP6GNtMQHFAOt+p+Yy0jNkWg@mail.gmail.com>
Subject: Re: [Bluez PATCH v1] src/profile: Ensure class UUID matches before
 connecting profile
To:     Archie Pusaka <apusaka@google.com>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Archie Pusaka <apusaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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