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: <20200226161257.Bluez.v2.1.I3c505b4307094eb7a6f2c5949125a17c89b2e099@changeid>
 <CABBYNZLK3Ct9kyikQiC4-QPC+xmg8cN30wN=fK8tm6STpmvDGA@mail.gmail.com>
In-Reply-To: <CABBYNZLK3Ct9kyikQiC4-QPC+xmg8cN30wN=fK8tm6STpmvDGA@mail.gmail.com>
From:   Archie Pusaka <apusaka@google.com>
Date:   Thu, 27 Feb 2020 11:32:26 +0800
Message-ID: <CAJQfnxFaQ2cJBVyktoBCA95TN9dhZf90R0G_DEWgsBvU=1uu9w@mail.gmail.com>
Subject: Re: [Bluez PATCH v2] src/profile: Ensure class UUID matches before
 connecting profile
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Luiz,

On Thu, 27 Feb 2020 at 01:55, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Wed, Feb 26, 2020 at 12:14 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.
> >
> > 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 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     | 17 ++++---------
> >  src/sdp-client.c | 62 ++++++++++++++++++++++++++++++++++++++++++++----
> >  src/sdp-client.h |  3 +++
> >  3 files changed, 64 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/device.c b/src/device.c
> > index a8f4c22f3..5ff381959 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -4590,17 +4590,8 @@ static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
> >                 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;
> > -               }
>
> This actually still need to be checked since there could be malformed
> records which don't set a service class, that said perhaps we could
> deal with that in sdp-client.c but it doesn't seem you have added
> these checks there.
>

Those malformed records which did not set a service class will have
the uuid stored in rec->svclass to be filled with all zeroes. This
will be NULL when converted into string, so we will skip that
effectively. But it would be better if I document this behaviour in
a comment.

> > +               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);

Actually I can just remove svcclass here (and everywhere else)

> >                         continue;
> > @@ -5352,9 +5343,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..fc8e9ec10 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;
> > @@ -157,6 +158,11 @@ static void search_context_cleanup(struct search_context *ctxt)
> >         g_free(ctxt);
> >  }
> >
> > +static gboolean check_record_uuid(sdp_record_t *rec, uuid_t *uuid)
> > +{
> > +       return sdp_uuid_cmp(uuid, &rec->svclass) == 0;
> > +}
>
> I wouldn't bother adding this function, instead just use sdp_uuid_cmp directly.

Yeah, not super useful. Will compare directly in the next patch.

>
> >  static void search_completed_cb(uint8_t type, uint16_t status,
> >                         uint8_t *rsp, size_t size, void *user_data)
> >  {
> > @@ -195,6 +201,12 @@ static void search_completed_cb(uint8_t type, uint16_t status,
> >                 rsp += recsize;
> >                 bytesleft -= recsize;
> >
> > +               if (ctxt->filter_svc_class &&
> > +                                       !check_record_uuid(rec, &ctxt->uuid)) {
> > +                       sdp_record_free(rec);
> > +                       continue;
> > +               }
> > +
> >                 recs = sdp_list_append(recs, rec);
> >         } while (scanned < (ssize_t) size && bytesleft > 0);
> >
> > @@ -338,7 +350,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 +381,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.1.481.gfbce0eb801-goog
>
> Other than that it looks pretty good.
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie