Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210802061250.170975-1-howardchung@google.com> <20210802141140.Bluez.v7.3.If0cf6e1feb9e9cc8106793bcaea60202852d7095@changeid>
In-Reply-To: <20210802141140.Bluez.v7.3.If0cf6e1feb9e9cc8106793bcaea60202852d7095@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 2 Aug 2021 11:49:50 -0700
Message-ID: <CABBYNZJTrym0dmQ0DnBsAcoaHeAqA3z8gxV7DJRbMQ+jdnk2VQ@mail.gmail.com>
Subject: Re: [Bluez PATCH v7 03/13] mcap: add adapter authorization
To:     Howard Chung <howardchung@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Yun-Hao Chung <howardchung@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Howard,

On Sun, Aug 1, 2021 at 11:13 PM Howard Chung <howardchung@google.com> wrote:
>
> From: Yun-Hao Chung <howardchung@chromium.org>
>
> Currently mcap is the only profile that doesn't request adatper
> authorization. This patch adds a argument when creating the mcap
> instance to set authorize method. The reason why we don't use
> btd_request_authorization directly like all other profiles is because
> tools/mcaptest includes the profile/health/mcap.h. If we add dependency
> to adapter.h in mcap.h, it will make mcaptest depend on adapter and be
> not able to build independently.
> ---
>
> (no changes since v1)
>
>  android/health.c       |  2 +-
>  profiles/health/hdp.c  |  1 +
>  profiles/health/mcap.c | 38 ++++++++++++++++++++++++++++++++++++--
>  profiles/health/mcap.h |  9 +++++++++
>  tools/mcaptest.c       |  2 +-
>  5 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/android/health.c b/android/health.c
> index 9a29964b1be2..de50db98e988 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -2008,7 +2008,7 @@ bool bt_health_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
>                                         mcl_connected, mcl_reconnected,
>                                         mcl_disconnected, mcl_uncached,
>                                         NULL, /* CSP is not used right now */
> -                                       NULL, &err);
> +                                       NULL, NULL, &err);
>         if (!mcap) {
>                 error("health: MCAP instance creation failed %s", err->message);
>                 g_error_free(err);
> diff --git a/profiles/health/hdp.c b/profiles/health/hdp.c
> index 6bc41946fef3..efa8955efaea 100644
> --- a/profiles/health/hdp.c
> +++ b/profiles/health/hdp.c
> @@ -1347,6 +1347,7 @@ static gboolean update_adapter(struct hdp_adapter *hdp_adapter)
>                                 mcl_connected, mcl_reconnected,
>                                 mcl_disconnected, mcl_uncached,
>                                 NULL, /* CSP is not used by now */
> +                               btd_request_authorization,
>                                 hdp_adapter, &err);
>         if (hdp_adapter->mi == NULL) {
>                 error("Error creating the MCAP instance: %s", err->message);
> diff --git a/profiles/health/mcap.c b/profiles/health/mcap.c
> index be13af37a0b8..a7eaba51693a 100644
> --- a/profiles/health/mcap.c
> +++ b/profiles/health/mcap.c
> @@ -14,6 +14,7 @@
>  #endif
>
>  #define _GNU_SOURCE
> +#include <dbus/dbus.h>
>  #include <netinet/in.h>
>  #include <stdlib.h>
>  #include <errno.h>
> @@ -23,6 +24,7 @@
>  #include <glib.h>
>
>  #include "lib/bluetooth.h"
> +#include "lib/uuid.h"
>  #include "bluetooth/l2cap.h"
>  #include "btio/btio.h"
>  #include "src/log.h"
> @@ -1980,7 +1982,6 @@ static void set_mcl_conf(GIOChannel *chan, struct mcap_mcl *mcl)
>         mcl->state = MCL_CONNECTED;
>         mcl->role = MCL_ACCEPTOR;
>         mcl->req = MCL_AVAILABLE;
> -       mcl->cc = g_io_channel_ref(chan);
>         mcl->ctrl |= MCAP_CTRL_STD_OP;
>
>         mcap_sync_init(mcl);
> @@ -2005,19 +2006,38 @@ static void set_mcl_conf(GIOChannel *chan, struct mcap_mcl *mcl)
>                 mcl->mi->mcl_connected_cb(mcl, mcl->mi->user_data);
>  }
>
> +static void auth_callback(DBusError *derr, void *user_data)
> +{
> +       struct mcap_mcl *mcl = user_data;
> +
> +       if (derr) {
> +               error("Access denied: %s", derr->message);
> +               goto reject;
> +       }
> +
> +       set_mcl_conf(mcl->cc, mcl);
> +       return;
> +
> +reject:
> +       g_io_channel_shutdown(mcl->cc, TRUE, NULL);
> +       g_io_channel_unref(mcl->cc);
> +}
> +
>  static void connect_mcl_event_cb(GIOChannel *chan, GError *gerr,
>                                                         gpointer user_data)
>  {
>         struct mcap_instance *mi = user_data;
>         struct mcap_mcl *mcl;
> -       bdaddr_t dst;
> +       bdaddr_t src, dst;
>         char address[18], srcstr[18];
>         GError *err = NULL;
> +       guint ret;
>
>         if (gerr)
>                 return;
>
>         bt_io_get(chan, &err,
> +                       BT_IO_OPT_SOURCE_BDADDR, &src,
>                         BT_IO_OPT_DEST_BDADDR, &dst,
>                         BT_IO_OPT_DEST, address,
>                         BT_IO_OPT_INVALID);
> @@ -2044,6 +2064,18 @@ static void connect_mcl_event_cb(GIOChannel *chan, GError *gerr,
>                 mcl->next_mdl = (rand() % MCAP_MDLID_FINAL) + 1;
>         }
>
> +       mcl->cc = g_io_channel_ref(chan);
> +       if (mi->authorize_cb) {
> +               ret = mi->authorize_cb(&src, &dst, HDP_UUID, auth_callback,
> +                                                                       mcl);
> +               if (ret != 0)
> +                       return;
> +
> +               error("HDP: authorization for device %s failed", address);
> +               g_io_channel_unref(mcl->cc);
> +               goto drop;
> +       }
> +
>         set_mcl_conf(chan, mcl);
>
>         return;
> @@ -2060,6 +2092,7 @@ struct mcap_instance *mcap_create_instance(const bdaddr_t *src,
>                                         mcap_mcl_event_cb mcl_disconnected,
>                                         mcap_mcl_event_cb mcl_uncached,
>                                         mcap_info_ind_event_cb mcl_sync_info_ind,
> +                                       mcap_authorize_cb authorize_cb,
>                                         gpointer user_data,
>                                         GError **gerr)
>  {
> @@ -2089,6 +2122,7 @@ struct mcap_instance *mcap_create_instance(const bdaddr_t *src,
>         mi->mcl_disconnected_cb = mcl_disconnected;
>         mi->mcl_uncached_cb = mcl_uncached;
>         mi->mcl_sync_infoind_cb = mcl_sync_info_ind;
> +       mi->authorize_cb = authorize_cb;
>         mi->user_data = user_data;
>         mi->csp_enabled = FALSE;
>
> diff --git a/profiles/health/mcap.h b/profiles/health/mcap.h
> index 5a94c8b63bea..48b7d7846c57 100644
> --- a/profiles/health/mcap.h
> +++ b/profiles/health/mcap.h
> @@ -9,6 +9,8 @@
>   *
>   */

These changes to mcap.h are causing a lot problems with CI, it seems
the comments are not going over 80 columns so if you really need to
touch this file you might as well move the comments above each field,
or perhaps don't bother changing the health plugin since it is
normally not enabled by default and we might as well consider it
deprecated given that has been many years without maintenance and
afaik these profiles have been moved to LE at some point.

> +#include <dbus/dbus.h>
> +
>  #define MCAP_VERSION   0x0100  /* current version 01.00 */
>
>  /* bytes to get MCAP Supported Procedures */
> @@ -249,6 +251,11 @@ typedef void (* mcap_sync_set_cb) (struct mcap_mcl *mcl,
>                                         GError *err,
>                                         gpointer data);
>
> +typedef void mcap_auth_cb(DBusError *derr, void *user_data);
> +typedef guint (* mcap_authorize_cb) (const bdaddr_t *src, const bdaddr_t *dst,
> +                                       const char *uuid, mcap_auth_cb cb,
> +                                       void *user_data);
> +
>  struct mcap_mdl_cb {
>         mcap_mdl_event_cb               mdl_connected;  /* Remote device has created a MDL */
>         mcap_mdl_event_cb               mdl_closed;     /* Remote device has closed a MDL */
> @@ -271,6 +278,7 @@ struct mcap_instance {
>         mcap_mcl_event_cb       mcl_disconnected_cb;    /* MCL disconnected */
>         mcap_mcl_event_cb       mcl_uncached_cb;        /* MCL has been removed from MCAP cache */
>         mcap_info_ind_event_cb  mcl_sync_infoind_cb;    /* (CSP Master) Received info indication */
> +       mcap_authorize_cb       authorize_cb;           /* Method to request authorization */
>         gpointer                user_data;              /* Data to be provided in callbacks */
>         int                     ref;                    /* Reference counter */
>
> @@ -404,6 +412,7 @@ struct mcap_instance *mcap_create_instance(const bdaddr_t *src,
>                                         mcap_mcl_event_cb mcl_disconnected,
>                                         mcap_mcl_event_cb mcl_uncached,
>                                         mcap_info_ind_event_cb mcl_sync_info_ind,
> +                                       mcap_authorize_cb authorize_cb,
>                                         gpointer user_data,
>                                         GError **gerr);
>  void mcap_release_instance(struct mcap_instance *mi);
> diff --git a/tools/mcaptest.c b/tools/mcaptest.c
> index dcef0b908ac8..63ee22149a40 100644
> --- a/tools/mcaptest.c
> +++ b/tools/mcaptest.c
> @@ -434,7 +434,7 @@ int main(int argc, char *argv[])
>                                         mcl_connected, mcl_reconnected,
>                                         mcl_disconnected, mcl_uncached,
>                                         NULL, /* CSP is not used right now */
> -                                       NULL, &err);
> +                                       NULL, NULL, &err);
>
>         if (!mcap) {
>                 printf("MCAP instance creation failed %s\n", err->message);
> --
> 2.32.0.554.ge1b32706d8-goog
>


-- 
Luiz Augusto von Dentz