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: <20200302120658.Bluez.v3.1.I3c505b4307094eb7a6f2c5949125a17c89b2e099@changeid>
In-Reply-To: <20200302120658.Bluez.v3.1.I3c505b4307094eb7a6f2c5949125a17c89b2e099@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 2 Mar 2020 14:18:19 -0800
Message-ID: <CABBYNZLSzd4i6FFaTXBXHb+8QmNW8jR1sr9g3hKEOiNMQ4KZjQ@mail.gmail.com>
Subject: Re: [Bluez PATCH v3] 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 Sun, Mar 1, 2020 at 8:07 PM 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.
>
> This patch checks the service class ID of the records against the
> requested UUID whenever bt_search_service() is called and filter
> out the ones that don't match. For the alternative where filtering
> is not applied, use the method bt_search().
>
> ---
>
> Changes in v3:
> - Simplify sdp_client.c
> - Add comment for the behavior when SDP record is missing the
>   service class ID
>
> Changes in v2:
> - Move service class matching from profile.c to sdp_client.c
> - Create function bt_search for searching without matching uuid
> - Update device.c to use bt_search for L2CAP
>
>  src/device.c     | 26 +++++++--------------
>  src/sdp-client.c | 61 ++++++++++++++++++++++++++++++++++++++++++++----
>  src/sdp-client.h |  3 +++
>  3 files changed, 67 insertions(+), 23 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index a8f4c22f3..c351a1562 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -4584,27 +4584,18 @@ static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
>
>         for (seq = recs; seq; seq = seq->next) {
>                 sdp_record_t *rec = (sdp_record_t *) seq->data;
> -               sdp_list_t *svcclass = NULL;
>                 char *profile_uuid;
>
>                 if (!rec)
>                         break;
>
> -               if (sdp_get_service_classes(rec, &svcclass) < 0)
> -                       continue;
> -
> -               /* Check for empty service classes list */
> -               if (svcclass == NULL) {
> -                       DBG("Skipping record with no service classes");
> -                       continue;
> -               }
> +               /* If service class attribute is missing, svclass will be all
> +                * zero and the resulting uuid string will be NULL.
> +                */
> +               profile_uuid = bt_uuid2string(&rec->svclass);
>
> -               /* Extract the first element and skip the remainning */
> -               profile_uuid = bt_uuid2string(svcclass->data);
> -               if (!profile_uuid) {
> -                       sdp_list_free(svcclass, free);
> +               if (!profile_uuid)
>                         continue;
> -               }
>
>                 if (bt_uuid_strcmp(profile_uuid, PNP_UUID) == 0) {
>                         uint16_t source, vendor, product, version;
> @@ -4638,7 +4629,6 @@ static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
>
>  next:
>                 free(profile_uuid);
> -               sdp_list_free(svcclass, free);
>         }
>
>         if (sdp_key_file) {
> @@ -5352,9 +5342,9 @@ static int device_browse_sdp(struct btd_device *device, DBusMessage *msg)
>
>         req->sdp_flags = get_sdp_flags(device);
>
> -       err = bt_search_service(btd_adapter_get_address(adapter),
> -                               &device->bdaddr, &uuid, browse_cb, req, NULL,
> -                               req->sdp_flags);
> +       err = bt_search(btd_adapter_get_address(adapter),
> +                       &device->bdaddr, &uuid, browse_cb, req, NULL,
> +                       req->sdp_flags);
>         if (err < 0) {
>                 browse_request_free(req);
>                 return err;
> diff --git a/src/sdp-client.c b/src/sdp-client.c
> index 413cf30ec..b92a083af 100644
> --- a/src/sdp-client.c
> +++ b/src/sdp-client.c
> @@ -143,6 +143,7 @@ struct search_context {
>         gpointer                user_data;
>         uuid_t                  uuid;
>         guint                   io_id;
> +       gboolean                filter_svc_class;
>  };
>
>  static GSList *context_list = NULL;
> @@ -195,6 +196,16 @@ static void search_completed_cb(uint8_t type, uint16_t status,
>                 rsp += recsize;
>                 bytesleft -= recsize;
>
> +               /* Check whether service class ID matches some specified uuid.
> +                * If the record is missing service class ID, svclass will be
> +                * all zero, and thus will be unequal to the requested uuid.
> +                */
> +               if (ctxt->filter_svc_class &&
> +                               sdp_uuid_cmp(&ctxt->uuid, &rec->svclass) != 0) {
> +                       sdp_record_free(rec);
> +                       continue;
> +               }
> +
>                 recs = sdp_list_append(recs, rec);
>         } while (scanned < (ssize_t) size && bytesleft > 0);
>
> @@ -338,7 +349,28 @@ static int create_search_context(struct search_context **ctxt,
>         return 0;
>  }
>
> -int bt_search_service(const bdaddr_t *src, const bdaddr_t *dst,
> +static int create_search_context_full(struct search_context **ctxt,
> +                                       const bdaddr_t *src,
> +                                       const bdaddr_t *dst,
> +                                       uuid_t *uuid, uint16_t flags,
> +                                       void *user_data, bt_callback_t cb,
> +                                       bt_destroy_t destroy,
> +                                       gboolean filter_svc_class)
> +{
> +       int err = create_search_context(ctxt, src, dst, uuid, flags);
> +
> +       if (err < 0)
> +               return err;
> +
> +       (*ctxt)->cb = cb;
> +       (*ctxt)->destroy = destroy;
> +       (*ctxt)->user_data = user_data;
> +       (*ctxt)->filter_svc_class = filter_svc_class;
> +
> +       return 0;
> +}
> +
> +int bt_search(const bdaddr_t *src, const bdaddr_t *dst,
>                         uuid_t *uuid, bt_callback_t cb, void *user_data,
>                         bt_destroy_t destroy, uint16_t flags)
>  {
> @@ -348,13 +380,32 @@ int bt_search_service(const bdaddr_t *src, const bdaddr_t *dst,
>         if (!cb)
>                 return -EINVAL;
>
> -       err = create_search_context(&ctxt, src, dst, uuid, flags);
> +       /* The resulting service class ID doesn't have to match uuid */
> +       err = create_search_context_full(&ctxt, src, dst, uuid, flags,
> +                                       user_data, cb, destroy, FALSE);
>         if (err < 0)
>                 return err;
>
> -       ctxt->cb        = cb;
> -       ctxt->destroy   = destroy;
> -       ctxt->user_data = user_data;
> +       context_list = g_slist_append(context_list, ctxt);
> +
> +       return 0;
> +}
> +
> +int bt_search_service(const bdaddr_t *src, const bdaddr_t *dst,
> +                       uuid_t *uuid, bt_callback_t cb, void *user_data,
> +                       bt_destroy_t destroy, uint16_t flags)
> +{
> +       struct search_context *ctxt = NULL;
> +       int err;
> +
> +       if (!cb)
> +               return -EINVAL;
> +
> +       /* The resulting service class ID need to match uuid */
> +       err = create_search_context_full(&ctxt, src, dst, uuid, flags,
> +                                       user_data, cb, destroy, TRUE);
> +       if (err < 0)
> +               return err;
>
>         context_list = g_slist_append(context_list, ctxt);
>
> diff --git a/src/sdp-client.h b/src/sdp-client.h
> index 9aa5a4d98..3a7212fd2 100644
> --- a/src/sdp-client.h
> +++ b/src/sdp-client.h
> @@ -24,6 +24,9 @@
>  typedef void (*bt_callback_t) (sdp_list_t *recs, int err, gpointer user_data);
>  typedef void (*bt_destroy_t) (gpointer user_data);
>
> +int bt_search(const bdaddr_t *src, const bdaddr_t *dst,
> +                       uuid_t *uuid, bt_callback_t cb, void *user_data,
> +                       bt_destroy_t destroy, uint16_t flags);
>  int bt_search_service(const bdaddr_t *src, const bdaddr_t *dst,
>                         uuid_t *uuid, bt_callback_t cb, void *user_data,
>                         bt_destroy_t destroy, uint16_t flags);
> --
> 2.25.0.265.gbab2e86ba0-goog

Applied, thanks.

-- 
Luiz Augusto von Dentz