2021-07-08 06:25:50

by Yun-hao Chung

[permalink] [raw]
Subject: [Bluez PATCH v1 00/14]

From: Yun-Hao Chung <[email protected]>

Hi manintainers,

This series is to
1. Implement a few methods in core so that a plugin can have control of
allowing / disallowing certain service connections.
2. Implement the AdminPolicy plugin. The plugin provides interfaces
AdminPolicySet and AdminPolicyStatus. For each policy, users should
set the value thorugh AdminPolicySet and query the current setting
through AdminPolicyStatus. We separeted these two interfaces so that
developers can assign different groups of users to these interfaces.
Currently the only policy is ServiceAllowList, which make bluez only
allow a list of service by specified their UUIDs, but the plugin is
also expected to provide more controls over other bluez behaviors.
Since the second part is a plugin, it might not be necessary to land in
upstream tree.

Thanks.


Howard Chung (2):
lib: add hash functions for bt_uuid_t
audio: Remove Media1 interface when a2dp source disallowed

Yun-Hao Chung (12):
unit: add uuid unit tests
core: add is_allowed property in btd_service
core: add adapter and device allowed_uuid functions
core: add device state and state callbacks
plugins: add a new plugin for admin_policy
plugins/admin_policy: add admin_policy adapter driver
plugins/admin_policy: add ServiceAllowList method
plugins/admin_policy: add ServiceAllowList property
plugins/admin_policy: add device state callback
plugins/admin_policy: add AffectedByPolicy property
plugins/admin_policy: persist policy settings
core: fix a possible crash when removing devices

Makefile.plugins | 5 +
bootstrap-configure | 1 +
configure.ac | 4 +
lib/uuid.c | 27 ++
lib/uuid.h | 3 +
plugins/admin_policy.c | 599 +++++++++++++++++++++++++++++++++++++++++
profiles/audio/a2dp.c | 2 +
profiles/audio/avrcp.c | 3 +
src/adapter.c | 90 +++++++
src/adapter.h | 8 +
src/device.c | 128 ++++++++-
src/device.h | 15 ++
src/service.c | 33 +++
src/service.h | 2 +
unit/test-uuid.c | 48 ++++
15 files changed, 966 insertions(+), 2 deletions(-)
create mode 100644 plugins/admin_policy.c

--
2.32.0.93.g670b81a890-goog


2021-07-08 06:25:50

by Yun-hao Chung

[permalink] [raw]
Subject: [Bluez PATCH v1 02/14] unit: add uuid unit tests

From: Yun-Hao Chung <[email protected]>

This adds uuid tests of bt_uuid_hash and bt_uuid_equal to
unit/test-uuid.c

Reviewed-by: Miao-chen Chou <[email protected]>
---

unit/test-uuid.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)

diff --git a/unit/test-uuid.c b/unit/test-uuid.c
index 0889630cfb34..ac613c5c2951 100644
--- a/unit/test-uuid.c
+++ b/unit/test-uuid.c
@@ -169,6 +169,34 @@ static void test_cmp(gconstpointer data)
tester_test_passed();
}

+static void test_hash(gconstpointer data)
+{
+ const struct uuid_test_data *test_data = data;
+ bt_uuid_t uuid1, uuid2;
+ guint uuid_h1, uuid_h2;
+
+ g_assert(bt_string_to_uuid(&uuid1, test_data->str) == 0);
+ g_assert(bt_string_to_uuid(&uuid2, test_data->str128) == 0);
+
+ uuid_h1 = bt_uuid_hash(&uuid1);
+ uuid_h2 = bt_uuid_hash(&uuid2);
+
+ g_assert(uuid_h1 == uuid_h2);
+ tester_test_passed();
+}
+
+static void test_equal(gconstpointer data)
+{
+ const struct uuid_test_data *test_data = data;
+ bt_uuid_t uuid1, uuid2;
+
+ g_assert(bt_string_to_uuid(&uuid1, test_data->str) == 0);
+ g_assert(bt_string_to_uuid(&uuid2, test_data->str128) == 0);
+
+ g_assert(bt_uuid_equal(&uuid1, &uuid2) == 1);
+ tester_test_passed();
+}
+
static const struct uuid_test_data compress[] = {
{
.str = "00001234-0000-1000-8000-00805f9b34fb",
@@ -226,26 +254,46 @@ int main(int argc, char *argv[])
tester_add("/uuid/base", &uuid_base, NULL, test_uuid, NULL);
tester_add("/uuid/base/str", &uuid_base, NULL, test_str, NULL);
tester_add("/uuid/base/cmp", &uuid_base, NULL, test_cmp, NULL);
+ tester_add("/uuid/base/hash", &uuid_base, NULL, test_hash, NULL);
+ tester_add("/uuid/base/equal", &uuid_base, NULL, test_equal, NULL);

tester_add("/uuid/sixteen1", &uuid_sixteen1, NULL, test_uuid, NULL);
tester_add("/uuid/sixteen1/str", &uuid_sixteen1, NULL, test_str, NULL);
tester_add("/uuid/sixteen1/cmp", &uuid_sixteen1, NULL, test_cmp, NULL);
+ tester_add("/uuid/sixteen1/hash", &uuid_sixteen1, NULL, test_hash,
+ NULL);
+ tester_add("/uuid/sixteen1/equal", &uuid_sixteen1, NULL, test_equal,
+ NULL);

tester_add("/uuid/sixteen2", &uuid_sixteen2, NULL, test_uuid, NULL);
tester_add("/uuid/sixteen2/str", &uuid_sixteen2, NULL, test_str, NULL);
tester_add("/uuid/sixteen2/cmp", &uuid_sixteen2, NULL, test_cmp, NULL);
+ tester_add("/uuid/sixteen2/hash", &uuid_sixteen2, NULL, test_hash,
+ NULL);
+ tester_add("/uuid/sixteen2/equal", &uuid_sixteen2, NULL, test_equal,
+ NULL);

tester_add("/uuid/thirtytwo1", &uuid_32_1, NULL, test_uuid, NULL);
tester_add("/uuid/thirtytwo1/str", &uuid_32_1, NULL, test_str, NULL);
tester_add("/uuid/thirtytwo1/cmp", &uuid_32_1, NULL, test_cmp, NULL);
+ tester_add("/uuid/thirtytwo1/hash", &uuid_32_1, NULL, test_hash, NULL);
+ tester_add("/uuid/thirtytwo1/equal", &uuid_32_1, NULL, test_equal,
+ NULL);

tester_add("/uuid/thirtytwo2", &uuid_32_2, NULL, test_uuid, NULL);
tester_add("/uuid/thritytwo2/str", &uuid_32_2, NULL, test_str, NULL);
tester_add("/uuid/thirtytwo2/cmp", &uuid_32_2, NULL, test_cmp, NULL);
+ tester_add("/uuid/thirtytwo2/hash", &uuid_32_2, NULL, test_hash, NULL);
+ tester_add("/uuid/thirtytwo2/equal", &uuid_32_2, NULL, test_equal,
+ NULL);

tester_add("/uuid/onetwentyeight", &uuid_128, NULL, test_uuid, NULL);
tester_add("/uuid/onetwentyeight/str", &uuid_128, NULL, test_str, NULL);
tester_add("/uuid/onetwentyeight/cmp", &uuid_128, NULL, test_cmp, NULL);
+ tester_add("/uuid/onetwentyeight/hash", &uuid_128, NULL, test_hash,
+ NULL);
+ tester_add("/uuid/onetwentyeight/equal", &uuid_128, NULL, test_equal,
+ NULL);

for (i = 0; malformed[i]; i++) {
char *testpath;
--
2.32.0.93.g670b81a890-goog

2021-07-08 06:25:50

by Yun-hao Chung

[permalink] [raw]
Subject: [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method

From: Yun-Hao Chung <[email protected]>

This adds code to register interface org.bluez.AdminPolicySet1.
The interface will provide methods to limit users to operate certain
functions of bluez, such as allow/disallow user to taggle adapter power,
or only allow users to connect services in the specified list, etc.

This patch also implements ServiceAllowlist in
org.bluez.AdminPolicySet1.

Reviewed-by: Miao-chen Chou <[email protected]>
---
The following test steps were performed:
1. Set ServiceAllowList to
["0x1108","0x110A","0x110B","0x110C","0x110D","0x110E",
"0x110F","0x1112","0x111E","0x111F","0x1203"]
( users are only allowed to connect headset )
2. Turn on paired WF1000XM3, and listen music on Youtube.
3. Turn on paired K830 (LE device), press any key on keyboard.
4. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
press any key on keyboard.
5. Set ServiceAllowList to
["1124","180A","180F","1812"]
( users are only allowed to connect HID devices )
6. Turn on paired WF1000XM3, and listen music on Youtube.
7. Turn on paired K830 (LE device), press any key on keyboard.
8. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
press any key on keyboard.
9. Set ServiceAllowList to []
( users are only allowed to connect any device. )
10. Turn on paired WF1000XM3, and listen music on Youtube.
11. Turn on paired K830 (LE device), press any key on keyboard.
12. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
press any key on keyboard.

Expected results:
Step 2,7,8,9,10,11 should success, and step 3,4,6 should fail.

plugins/admin_policy.c | 123 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 122 insertions(+), 1 deletion(-)

diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
index 2ece871564e6..242b8d5dacb0 100644
--- a/plugins/admin_policy.c
+++ b/plugins/admin_policy.c
@@ -12,19 +12,29 @@
#include <config.h>
#endif

+#include <dbus/dbus.h>
+#include <gdbus/gdbus.h>
+
#include "lib/bluetooth.h"
+#include "lib/uuid.h"

#include "src/adapter.h"
+#include "src/dbus-common.h"
#include "src/error.h"
#include "src/log.h"
#include "src/plugin.h"

#include "src/shared/queue.h"

+#define ADMIN_POLICY_SET_INTERFACE "org.bluez.AdminPolicySet1"
+
+static DBusConnection *dbus_conn;
+
/* |policy_data| has the same life cycle as btd_adapter */
static struct btd_admin_policy {
struct btd_adapter *adapter;
uint16_t adapter_id;
+ struct queue *service_allowlist;
} *policy_data = NULL;

static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
@@ -40,17 +50,116 @@ static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)

admin_policy->adapter = adapter;
admin_policy->adapter_id = btd_adapter_get_index(adapter);
+ admin_policy->service_allowlist = NULL;

return admin_policy;
}

+static void free_service_allowlist(struct queue *q)
+{
+ queue_destroy(q, g_free);
+}
+
static void admin_policy_free(void *data)
{
struct btd_admin_policy *admin_policy = data;

+ free_service_allowlist(admin_policy->service_allowlist);
g_free(admin_policy);
}

+static struct queue *parse_allow_service_list(struct btd_adapter *adapter,
+ DBusMessage *msg)
+{
+ DBusMessageIter iter, arr_iter;
+ struct queue *uuid_list = NULL;
+
+ dbus_message_iter_init(msg, &iter);
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY)
+ return NULL;
+
+ uuid_list = queue_new();
+ dbus_message_iter_recurse(&iter, &arr_iter);
+ do {
+ const int type = dbus_message_iter_get_arg_type(&arr_iter);
+ char *uuid_param;
+ bt_uuid_t *uuid;
+
+ if (type == DBUS_TYPE_INVALID)
+ break;
+
+ if (type != DBUS_TYPE_STRING)
+ goto failed;
+
+ dbus_message_iter_get_basic(&arr_iter, &uuid_param);
+
+ uuid = g_try_malloc(sizeof(*uuid));
+ if (!uuid)
+ goto failed;
+
+ if (bt_string_to_uuid(uuid, uuid_param)) {
+ g_free(uuid);
+ goto failed;
+ }
+
+ queue_push_head(uuid_list, uuid);
+
+ dbus_message_iter_next(&arr_iter);
+ } while (true);
+
+ return uuid_list;
+
+failed:
+ queue_destroy(uuid_list, g_free);
+ return NULL;
+}
+
+static bool service_allowlist_set(struct btd_admin_policy *admin_policy,
+ struct queue *uuid_list)
+{
+ struct btd_adapter *adapter = admin_policy->adapter;
+
+ if (!btd_adapter_set_allowed_uuids(adapter, uuid_list))
+ return false;
+
+ free_service_allowlist(admin_policy->service_allowlist);
+ admin_policy->service_allowlist = uuid_list;
+
+ return true;
+}
+
+static DBusMessage *set_service_allowlist(DBusConnection *conn,
+ DBusMessage *msg, void *user_data)
+{
+ struct btd_admin_policy *admin_policy = user_data;
+ struct btd_adapter *adapter = admin_policy->adapter;
+ struct queue *uuid_list = NULL;
+ const char *sender = dbus_message_get_sender(msg);
+
+ DBG("sender %s", sender);
+
+ /* Parse parameters */
+ uuid_list = parse_allow_service_list(adapter, msg);
+ if (!uuid_list) {
+ btd_error(admin_policy->adapter_id,
+ "Failed on parsing allowed service list");
+ return btd_error_invalid_args(msg);
+ }
+
+ if (!service_allowlist_set(admin_policy, uuid_list)) {
+ free_service_allowlist(uuid_list);
+ return btd_error_failed(msg, "service_allowlist_set failed");
+ }
+
+ return dbus_message_new_method_return(msg);
+}
+
+static const GDBusMethodTable admin_policy_adapter_methods[] = {
+ { GDBUS_METHOD("SetServiceAllowList", GDBUS_ARGS({ "UUIDs", "as" }),
+ NULL, set_service_allowlist) },
+ { }
+};
+
static int admin_policy_adapter_probe(struct btd_adapter *adapter)
{
if (policy_data) {
@@ -64,8 +173,18 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
if (!policy_data)
return -ENOMEM;

- btd_info(policy_data->adapter_id, "Admin Policy has been enabled");
+ if (!g_dbus_register_interface(dbus_conn, adapter_get_path(adapter),
+ ADMIN_POLICY_SET_INTERFACE,
+ admin_policy_adapter_methods, NULL,
+ NULL, policy_data, admin_policy_free)) {
+ btd_error(policy_data->adapter_id,
+ "Admin Policy Set interface init failed on path %s",
+ adapter_get_path(adapter));
+ return -EINVAL;
+ }

+ btd_info(policy_data->adapter_id,
+ "Admin Policy Set interface registered");
return 0;
}

@@ -79,6 +198,8 @@ static int admin_policy_init(void)
{
DBG("");

+ dbus_conn = btd_get_dbus_connection();
+
return btd_register_adapter_driver(&admin_policy_driver);
}

--
2.32.0.93.g670b81a890-goog

2021-07-08 06:25:50

by Yun-hao Chung

[permalink] [raw]
Subject: [Bluez PATCH v1 11/14] plugins/admin_policy: add device state callback

From: Yun-Hao Chung <[email protected]>

This registers a device state callback function. It will be used to
implement "AffectedByPolicy" property which indicates if there is any
service in a device that is being blocked by admin policy.

Reviewed-by: Miao-chen Chou <[email protected]>
---
The following test steps were performed:
1. start discovery using UI
2. verify device_data were added by checking system log
3. stop discovery
4. verify device_data were removed after a few seconds by checking
system log

plugins/admin_policy.c | 87 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 87 insertions(+)

diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
index c5ad31f761d9..852e79aaa07a 100644
--- a/plugins/admin_policy.c
+++ b/plugins/admin_policy.c
@@ -20,6 +20,7 @@

#include "src/adapter.h"
#include "src/dbus-common.h"
+#include "src/device.h"
#include "src/error.h"
#include "src/log.h"
#include "src/plugin.h"
@@ -30,6 +31,8 @@
#define ADMIN_POLICY_STATUS_INTERFACE "org.bluez.AdminPolicyStatus1"

static DBusConnection *dbus_conn;
+static struct queue *devices; /* List of struct device_data objects */
+static unsigned int device_cb_id;

/* |policy_data| has the same life cycle as btd_adapter */
static struct btd_admin_policy {
@@ -38,6 +41,11 @@ static struct btd_admin_policy {
struct queue *service_allowlist;
} *policy_data = NULL;

+struct device_data {
+ struct btd_device *device;
+ char *path;
+};
+
static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
{
struct btd_admin_policy *admin_policy = NULL;
@@ -245,6 +253,79 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
return 0;
}

+static bool device_data_match(const void *a, const void *b)
+{
+ const struct device_data *data = a;
+ const struct btd_device *dev = b;
+
+ if (!data) {
+ error("Unexpected NULL device_data");
+ return false;
+ }
+
+ return data->device == dev;
+}
+
+static void free_device_data(struct device_data *data)
+{
+ g_free(data->path);
+ g_free(data);
+}
+
+static void remove_device_data(void *data)
+{
+ struct device_data *device_data = data;
+
+ DBG("device_data for %s removing", device_data->path);
+
+ queue_remove(devices, device_data);
+ free_device_data(device_data);
+}
+
+static void add_device_data(struct btd_device *device)
+{
+ struct btd_adapter *adapter = device_get_adapter(device);
+ struct device_data *data;
+
+ if (queue_find(devices, device_data_match, device))
+ return;
+
+ data = g_new0(struct device_data, 1);
+ if (!data) {
+ btd_error(btd_adapter_get_index(adapter),
+ "Failed to allocate memory for device_data");
+ return;
+ }
+
+ data->device = device;
+ data->path = g_strdup(device_get_path(device));
+ queue_push_tail(devices, data);
+
+ DBG("device_data for %s added", data->path);
+}
+
+static void admin_policy_device_state_cb(struct btd_device *device,
+ enum btd_device_state_t new_state,
+ void *user_data)
+{
+ struct device_data *data = NULL;
+
+ switch (new_state) {
+ case BTD_DEVICE_STATE_INITIALIZING:
+ warn("Unexpected new state %d", new_state);
+ return;
+ case BTD_DEVICE_STATE_AVAILABLE:
+ add_device_data(device);
+ break;
+ case BTD_DEVICE_STATE_REMOVING:
+ data = queue_find(devices, device_data_match, device);
+
+ if (data)
+ remove_device_data(data);
+ break;
+ }
+}
+
static struct btd_adapter_driver admin_policy_driver = {
.name = "admin_policy",
.probe = admin_policy_adapter_probe,
@@ -256,6 +337,10 @@ static int admin_policy_init(void)
DBG("");

dbus_conn = btd_get_dbus_connection();
+ devices = queue_new();
+
+ device_cb_id = btd_device_add_state_cb(admin_policy_device_state_cb,
+ NULL);

return btd_register_adapter_driver(&admin_policy_driver);
}
@@ -265,6 +350,8 @@ static void admin_policy_exit(void)
DBG("");

btd_unregister_adapter_driver(&admin_policy_driver);
+ queue_destroy(devices, free_device_data);
+ btd_device_remove_state_cb(device_cb_id);

if (policy_data)
admin_policy_free(policy_data);
--
2.32.0.93.g670b81a890-goog

2021-07-08 06:27:50

by Yun-hao Chung

[permalink] [raw]
Subject: [Bluez PATCH v1 12/14] plugins/admin_policy: add AffectedByPolicy property

From: Yun-Hao Chung <[email protected]>

This adds property to indicate if a device has any service that is being
blocked by admin policy.

Reviewed-by: Miao-chen Chou <[email protected]>
---
The following test steps were performed:
1. Set ServiceAllowList to []
2. Verify AffectedByPolicy of K830 is False
3. Set ServiceAllowList to
["1800","1801","180A","180F","1812",
"00010000-0000-1000-8000-011f2000046d"
4. Verify AffectedByPolicy of K830 is False
5. Set ServiceAllowList to ["1800","1801","180A","180F","1812"]
6. Verify AffectedByPolicy of K830 is True

plugins/admin_policy.c | 78 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
index 852e79aaa07a..be4ba096a8b9 100644
--- a/plugins/admin_policy.c
+++ b/plugins/admin_policy.c
@@ -44,6 +44,7 @@ static struct btd_admin_policy {
struct device_data {
struct btd_device *device;
char *path;
+ bool affected;
};

static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
@@ -137,6 +138,27 @@ static bool service_allowlist_set(struct btd_admin_policy *admin_policy,
return true;
}

+static void update_device_affected(void *data, void *user_data)
+{
+ struct device_data *dev_data = data;
+ bool affected;
+
+ if (!dev_data) {
+ error("Unexpected NULL device_data when updating device");
+ return;
+ }
+
+ affected = btd_device_all_services_allowed(dev_data->device);
+
+ if (affected == dev_data->affected)
+ return;
+
+ dev_data->affected = affected;
+
+ g_dbus_emit_property_changed(dbus_conn, dev_data->path,
+ ADMIN_POLICY_STATUS_INTERFACE, "AffectedByPolicy");
+}
+
static DBusMessage *set_service_allowlist(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
@@ -164,6 +186,8 @@ static DBusMessage *set_service_allowlist(DBusConnection *conn,
adapter_get_path(policy_data->adapter),
ADMIN_POLICY_SET_INTERFACE, "ServiceAllowList");

+ queue_foreach(devices, update_device_affected, NULL);
+
return dbus_message_new_method_return(msg);
}

@@ -266,6 +290,29 @@ static bool device_data_match(const void *a, const void *b)
return data->device == dev;
}

+static gboolean property_get_affected_by_policy(
+ const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *user_data)
+{
+ struct device_data *data = user_data;
+ dbus_bool_t affected;
+
+ if (!data) {
+ error("Unexpected error: device_data is NULL");
+ return FALSE;
+ }
+
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
+ &data->affected);
+
+ return TRUE;
+}
+
+static const GDBusPropertyTable admin_policy_device_properties[] = {
+ { "AffectedByPolicy", "b", property_get_affected_by_policy },
+ { }
+};
+
static void free_device_data(struct device_data *data)
{
g_free(data->path);
@@ -299,11 +346,33 @@ static void add_device_data(struct btd_device *device)

data->device = device;
data->path = g_strdup(device_get_path(device));
+ data->affected = btd_device_all_services_allowed(data->device);
+
+ if (!g_dbus_register_interface(dbus_conn, data->path,
+ ADMIN_POLICY_STATUS_INTERFACE,
+ NULL, NULL,
+ admin_policy_device_properties,
+ data, remove_device_data)) {
+ btd_error(btd_adapter_get_index(adapter),
+ "Admin Policy Status interface init failed on path %s",
+ device_get_path(device));
+ free_device_data(data);
+ return;
+ }
+
queue_push_tail(devices, data);

DBG("device_data for %s added", data->path);
}

+static void unregister_device_data(void *data, void *user_data)
+{
+ struct device_data *dev_data = data;
+
+ g_dbus_unregister_interface(dbus_conn, dev_data->path,
+ ADMIN_POLICY_STATUS_INTERFACE);
+}
+
static void admin_policy_device_state_cb(struct btd_device *device,
enum btd_device_state_t new_state,
void *user_data)
@@ -321,7 +390,7 @@ static void admin_policy_device_state_cb(struct btd_device *device,
data = queue_find(devices, device_data_match, device);

if (data)
- remove_device_data(data);
+ unregister_device_data(data, NULL);
break;
}
}
@@ -339,6 +408,9 @@ static int admin_policy_init(void)
dbus_conn = btd_get_dbus_connection();
devices = queue_new();

+ device_cb_id = btd_device_add_state_cb(admin_policy_device_state_cb,
+ NULL);
+
device_cb_id = btd_device_add_state_cb(admin_policy_device_state_cb,
NULL);

@@ -350,9 +422,11 @@ static void admin_policy_exit(void)
DBG("");

btd_unregister_adapter_driver(&admin_policy_driver);
- queue_destroy(devices, free_device_data);
+ queue_foreach(devices, unregister_device_data, NULL);
+ queue_destroy(devices, g_free);
btd_device_remove_state_cb(device_cb_id);

+ btd_device_remove_state_cb(device_cb_id);
if (policy_data)
admin_policy_free(policy_data);
}
--
2.32.0.93.g670b81a890-goog

2021-07-08 06:27:50

by Yun-hao Chung

[permalink] [raw]
Subject: [Bluez PATCH v1 03/14] core: add is_allowed property in btd_service

From: Yun-Hao Chung <[email protected]>

This adds is_allowed property in btd_service. When is_allowed is set to
false, calling btd_service_connect and service_accept will fail and the
existing service connection gets disconnected.

Reviewed-by: Miao-chen Chou <[email protected]>
---

src/service.c | 33 +++++++++++++++++++++++++++++++++
src/service.h | 2 ++
2 files changed, 35 insertions(+)

diff --git a/src/service.c b/src/service.c
index 21a52762e637..84fbb208a7e9 100644
--- a/src/service.c
+++ b/src/service.c
@@ -41,6 +41,7 @@ struct btd_service {
void *user_data;
btd_service_state_t state;
int err;
+ bool is_allowed;
};

struct service_state_callback {
@@ -133,6 +134,7 @@ struct btd_service *service_create(struct btd_device *device,
service->device = device; /* Weak ref */
service->profile = profile;
service->state = BTD_SERVICE_STATE_UNAVAILABLE;
+ service->is_allowed = true;

return service;
}
@@ -186,6 +188,12 @@ int service_accept(struct btd_service *service)
if (!service->profile->accept)
return -ENOSYS;

+ if (!service->is_allowed) {
+ info("service %s is not allowed",
+ service->profile->remote_uuid);
+ return -ECONNABORTED;
+ }
+
err = service->profile->accept(service);
if (!err)
goto done;
@@ -245,6 +253,12 @@ int btd_service_connect(struct btd_service *service)
return -EBUSY;
}

+ if (!service->is_allowed) {
+ info("service %s is not allowed",
+ service->profile->remote_uuid);
+ return -ECONNABORTED;
+ }
+
err = profile->connect(service);
if (err == 0) {
change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
@@ -361,6 +375,25 @@ bool btd_service_remove_state_cb(unsigned int id)
return false;
}

+void btd_service_set_allowed(struct btd_service *service, bool allowed)
+{
+ if (allowed == service->is_allowed)
+ return;
+
+ service->is_allowed = allowed;
+
+ if (!allowed && (service->state == BTD_SERVICE_STATE_CONNECTING ||
+ service->state == BTD_SERVICE_STATE_CONNECTED)) {
+ btd_service_disconnect(service);
+ return;
+ }
+}
+
+bool btd_service_is_allowed(struct btd_service *service)
+{
+ return service->is_allowed;
+}
+
void btd_service_connecting_complete(struct btd_service *service, int err)
{
if (service->state != BTD_SERVICE_STATE_DISCONNECTED &&
diff --git a/src/service.h b/src/service.h
index 88530cc17d53..5a2a02447b24 100644
--- a/src/service.h
+++ b/src/service.h
@@ -51,6 +51,8 @@ int btd_service_get_error(const struct btd_service *service);
unsigned int btd_service_add_state_cb(btd_service_state_cb cb,
void *user_data);
bool btd_service_remove_state_cb(unsigned int id);
+void btd_service_set_allowed(struct btd_service *service, bool allowed);
+bool btd_service_is_allowed(struct btd_service *service);

/* Functions used by profile implementation */
void btd_service_connecting_complete(struct btd_service *service, int err);
--
2.32.0.93.g670b81a890-goog

2021-07-09 06:03:47

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method

Hi Howard,

On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <[email protected]> wrote:
>
> From: Yun-Hao Chung <[email protected]>
>
> This adds code to register interface org.bluez.AdminPolicySet1.
> The interface will provide methods to limit users to operate certain
> functions of bluez, such as allow/disallow user to taggle adapter power,
> or only allow users to connect services in the specified list, etc.
>
> This patch also implements ServiceAllowlist in
> org.bluez.AdminPolicySet1.
>
> Reviewed-by: Miao-chen Chou <[email protected]>
> ---
> The following test steps were performed:
> 1. Set ServiceAllowList to
> ["0x1108","0x110A","0x110B","0x110C","0x110D","0x110E",
> "0x110F","0x1112","0x111E","0x111F","0x1203"]
> ( users are only allowed to connect headset )
> 2. Turn on paired WF1000XM3, and listen music on Youtube.
> 3. Turn on paired K830 (LE device), press any key on keyboard.
> 4. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> press any key on keyboard.
> 5. Set ServiceAllowList to
> ["1124","180A","180F","1812"]
> ( users are only allowed to connect HID devices )
> 6. Turn on paired WF1000XM3, and listen music on Youtube.
> 7. Turn on paired K830 (LE device), press any key on keyboard.
> 8. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> press any key on keyboard.
> 9. Set ServiceAllowList to []
> ( users are only allowed to connect any device. )
> 10. Turn on paired WF1000XM3, and listen music on Youtube.
> 11. Turn on paired K830 (LE device), press any key on keyboard.
> 12. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> press any key on keyboard.
>
> Expected results:
> Step 2,7,8,9,10,11 should success, and step 3,4,6 should fail.

All this explanation is great but it would really help if you have
added support for bluetoothctl to control this, we also need to
document these interfaces in case someone else wants to use them (e.g:
other clients like bluetoothctl). For the bluetoothctl we could
perhaps an admin menu registered whenever the interfaces are available
and then a command allow [list/none/any] so the user can query when no
parameter is given or set a list of UUIDs. Btw, how is this supposed
to work with vendor UUIDs? I guess that would need to support UUID 128
bit format in order to support that.

>
> plugins/admin_policy.c | 123 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 122 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
> index 2ece871564e6..242b8d5dacb0 100644
> --- a/plugins/admin_policy.c
> +++ b/plugins/admin_policy.c
> @@ -12,19 +12,29 @@
> #include <config.h>
> #endif
>
> +#include <dbus/dbus.h>
> +#include <gdbus/gdbus.h>
> +
> #include "lib/bluetooth.h"
> +#include "lib/uuid.h"
>
> #include "src/adapter.h"
> +#include "src/dbus-common.h"
> #include "src/error.h"
> #include "src/log.h"
> #include "src/plugin.h"
>
> #include "src/shared/queue.h"
>
> +#define ADMIN_POLICY_SET_INTERFACE "org.bluez.AdminPolicySet1"
> +
> +static DBusConnection *dbus_conn;
> +
> /* |policy_data| has the same life cycle as btd_adapter */
> static struct btd_admin_policy {
> struct btd_adapter *adapter;
> uint16_t adapter_id;
> + struct queue *service_allowlist;
> } *policy_data = NULL;
>
> static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
> @@ -40,17 +50,116 @@ static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
>
> admin_policy->adapter = adapter;
> admin_policy->adapter_id = btd_adapter_get_index(adapter);
> + admin_policy->service_allowlist = NULL;
>
> return admin_policy;
> }
>
> +static void free_service_allowlist(struct queue *q)
> +{
> + queue_destroy(q, g_free);
> +}
> +
> static void admin_policy_free(void *data)
> {
> struct btd_admin_policy *admin_policy = data;
>
> + free_service_allowlist(admin_policy->service_allowlist);
> g_free(admin_policy);
> }
>
> +static struct queue *parse_allow_service_list(struct btd_adapter *adapter,
> + DBusMessage *msg)
> +{
> + DBusMessageIter iter, arr_iter;
> + struct queue *uuid_list = NULL;
> +
> + dbus_message_iter_init(msg, &iter);
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY)
> + return NULL;
> +
> + uuid_list = queue_new();
> + dbus_message_iter_recurse(&iter, &arr_iter);
> + do {
> + const int type = dbus_message_iter_get_arg_type(&arr_iter);
> + char *uuid_param;
> + bt_uuid_t *uuid;
> +
> + if (type == DBUS_TYPE_INVALID)
> + break;
> +
> + if (type != DBUS_TYPE_STRING)
> + goto failed;
> +
> + dbus_message_iter_get_basic(&arr_iter, &uuid_param);
> +
> + uuid = g_try_malloc(sizeof(*uuid));
> + if (!uuid)
> + goto failed;
> +
> + if (bt_string_to_uuid(uuid, uuid_param)) {
> + g_free(uuid);
> + goto failed;
> + }
> +
> + queue_push_head(uuid_list, uuid);
> +
> + dbus_message_iter_next(&arr_iter);
> + } while (true);
> +
> + return uuid_list;
> +
> +failed:
> + queue_destroy(uuid_list, g_free);
> + return NULL;
> +}
> +
> +static bool service_allowlist_set(struct btd_admin_policy *admin_policy,
> + struct queue *uuid_list)
> +{
> + struct btd_adapter *adapter = admin_policy->adapter;
> +
> + if (!btd_adapter_set_allowed_uuids(adapter, uuid_list))
> + return false;
> +
> + free_service_allowlist(admin_policy->service_allowlist);
> + admin_policy->service_allowlist = uuid_list;
> +
> + return true;
> +}
> +
> +static DBusMessage *set_service_allowlist(DBusConnection *conn,
> + DBusMessage *msg, void *user_data)
> +{
> + struct btd_admin_policy *admin_policy = user_data;
> + struct btd_adapter *adapter = admin_policy->adapter;
> + struct queue *uuid_list = NULL;
> + const char *sender = dbus_message_get_sender(msg);
> +
> + DBG("sender %s", sender);
> +
> + /* Parse parameters */
> + uuid_list = parse_allow_service_list(adapter, msg);
> + if (!uuid_list) {
> + btd_error(admin_policy->adapter_id,
> + "Failed on parsing allowed service list");
> + return btd_error_invalid_args(msg);
> + }
> +
> + if (!service_allowlist_set(admin_policy, uuid_list)) {
> + free_service_allowlist(uuid_list);
> + return btd_error_failed(msg, "service_allowlist_set failed");
> + }
> +
> + return dbus_message_new_method_return(msg);
> +}
> +
> +static const GDBusMethodTable admin_policy_adapter_methods[] = {
> + { GDBUS_METHOD("SetServiceAllowList", GDBUS_ARGS({ "UUIDs", "as" }),
> + NULL, set_service_allowlist) },
> + { }
> +};
> +
> static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> {
> if (policy_data) {
> @@ -64,8 +173,18 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> if (!policy_data)
> return -ENOMEM;
>
> - btd_info(policy_data->adapter_id, "Admin Policy has been enabled");
> + if (!g_dbus_register_interface(dbus_conn, adapter_get_path(adapter),
> + ADMIN_POLICY_SET_INTERFACE,
> + admin_policy_adapter_methods, NULL,
> + NULL, policy_data, admin_policy_free)) {
> + btd_error(policy_data->adapter_id,
> + "Admin Policy Set interface init failed on path %s",
> + adapter_get_path(adapter));
> + return -EINVAL;
> + }
>
> + btd_info(policy_data->adapter_id,
> + "Admin Policy Set interface registered");
> return 0;
> }
>
> @@ -79,6 +198,8 @@ static int admin_policy_init(void)
> {
> DBG("");
>
> + dbus_conn = btd_get_dbus_connection();
> +
> return btd_register_adapter_driver(&admin_policy_driver);
> }
>
> --
> 2.32.0.93.g670b81a890-goog
>


--
Luiz Augusto von Dentz

2021-07-12 09:16:35

by Yun-hao Chung

[permalink] [raw]
Subject: Re: [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method

On Fri, Jul 9, 2021 at 2:01 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Howard,
>
> On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <[email protected]> wrote:
> >
> > From: Yun-Hao Chung <[email protected]>
> >
> > This adds code to register interface org.bluez.AdminPolicySet1.
> > The interface will provide methods to limit users to operate certain
> > functions of bluez, such as allow/disallow user to taggle adapter power,
> > or only allow users to connect services in the specified list, etc.
> >
> > This patch also implements ServiceAllowlist in
> > org.bluez.AdminPolicySet1.
> >
> > Reviewed-by: Miao-chen Chou <[email protected]>
> > ---
> > The following test steps were performed:
> > 1. Set ServiceAllowList to
> > ["0x1108","0x110A","0x110B","0x110C","0x110D","0x110E",
> > "0x110F","0x1112","0x111E","0x111F","0x1203"]
> > ( users are only allowed to connect headset )
> > 2. Turn on paired WF1000XM3, and listen music on Youtube.
> > 3. Turn on paired K830 (LE device), press any key on keyboard.
> > 4. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> > press any key on keyboard.
> > 5. Set ServiceAllowList to
> > ["1124","180A","180F","1812"]
> > ( users are only allowed to connect HID devices )
> > 6. Turn on paired WF1000XM3, and listen music on Youtube.
> > 7. Turn on paired K830 (LE device), press any key on keyboard.
> > 8. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> > press any key on keyboard.
> > 9. Set ServiceAllowList to []
> > ( users are only allowed to connect any device. )
> > 10. Turn on paired WF1000XM3, and listen music on Youtube.
> > 11. Turn on paired K830 (LE device), press any key on keyboard.
> > 12. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> > press any key on keyboard.
> >
> > Expected results:
> > Step 2,7,8,9,10,11 should success, and step 3,4,6 should fail.
>
> All this explanation is great but it would really help if you have
> added support for bluetoothctl to control this, we also need to
Document it sounds good to me, but I notice that there is no document
for any plugin yet.
Where do you think we should put the document in?
> document these interfaces in case someone else wants to use them (e.g:
> other clients like bluetoothctl). For the bluetoothctl we could
> perhaps an admin menu registered whenever the interfaces are available
> and then a command allow [list/none/any] so the user can query when no
> parameter is given or set a list of UUIDs. Btw, how is this supposed
Adding commands in bluetoothctl sounds good to me as well. Can we
implement this in
a separate series?
> to work with vendor UUIDs? I guess that would need to support UUID 128
> bit format in order to support that.
Since we are using bt_string_to_uuid to parse the given string,
internally it checks if it can be parsed as UUID128/UUID32/UUID16.
>
> >
> > plugins/admin_policy.c | 123 ++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 122 insertions(+), 1 deletion(-)
> >
> > diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
> > index 2ece871564e6..242b8d5dacb0 100644
> > --- a/plugins/admin_policy.c
> > +++ b/plugins/admin_policy.c
> > @@ -12,19 +12,29 @@
> > #include <config.h>
> > #endif
> >
> > +#include <dbus/dbus.h>
> > +#include <gdbus/gdbus.h>
> > +
> > #include "lib/bluetooth.h"
> > +#include "lib/uuid.h"
> >
> > #include "src/adapter.h"
> > +#include "src/dbus-common.h"
> > #include "src/error.h"
> > #include "src/log.h"
> > #include "src/plugin.h"
> >
> > #include "src/shared/queue.h"
> >
> > +#define ADMIN_POLICY_SET_INTERFACE "org.bluez.AdminPolicySet1"
> > +
> > +static DBusConnection *dbus_conn;
> > +
> > /* |policy_data| has the same life cycle as btd_adapter */
> > static struct btd_admin_policy {
> > struct btd_adapter *adapter;
> > uint16_t adapter_id;
> > + struct queue *service_allowlist;
> > } *policy_data = NULL;
> >
> > static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
> > @@ -40,17 +50,116 @@ static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
> >
> > admin_policy->adapter = adapter;
> > admin_policy->adapter_id = btd_adapter_get_index(adapter);
> > + admin_policy->service_allowlist = NULL;
> >
> > return admin_policy;
> > }
> >
> > +static void free_service_allowlist(struct queue *q)
> > +{
> > + queue_destroy(q, g_free);
> > +}
> > +
> > static void admin_policy_free(void *data)
> > {
> > struct btd_admin_policy *admin_policy = data;
> >
> > + free_service_allowlist(admin_policy->service_allowlist);
> > g_free(admin_policy);
> > }
> >
> > +static struct queue *parse_allow_service_list(struct btd_adapter *adapter,
> > + DBusMessage *msg)
> > +{
> > + DBusMessageIter iter, arr_iter;
> > + struct queue *uuid_list = NULL;
> > +
> > + dbus_message_iter_init(msg, &iter);
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY)
> > + return NULL;
> > +
> > + uuid_list = queue_new();
> > + dbus_message_iter_recurse(&iter, &arr_iter);
> > + do {
> > + const int type = dbus_message_iter_get_arg_type(&arr_iter);
> > + char *uuid_param;
> > + bt_uuid_t *uuid;
> > +
> > + if (type == DBUS_TYPE_INVALID)
> > + break;
> > +
> > + if (type != DBUS_TYPE_STRING)
> > + goto failed;
> > +
> > + dbus_message_iter_get_basic(&arr_iter, &uuid_param);
> > +
> > + uuid = g_try_malloc(sizeof(*uuid));
> > + if (!uuid)
> > + goto failed;
> > +
> > + if (bt_string_to_uuid(uuid, uuid_param)) {
> > + g_free(uuid);
> > + goto failed;
> > + }
> > +
> > + queue_push_head(uuid_list, uuid);
> > +
> > + dbus_message_iter_next(&arr_iter);
> > + } while (true);
> > +
> > + return uuid_list;
> > +
> > +failed:
> > + queue_destroy(uuid_list, g_free);
> > + return NULL;
> > +}
> > +
> > +static bool service_allowlist_set(struct btd_admin_policy *admin_policy,
> > + struct queue *uuid_list)
> > +{
> > + struct btd_adapter *adapter = admin_policy->adapter;
> > +
> > + if (!btd_adapter_set_allowed_uuids(adapter, uuid_list))
> > + return false;
> > +
> > + free_service_allowlist(admin_policy->service_allowlist);
> > + admin_policy->service_allowlist = uuid_list;
> > +
> > + return true;
> > +}
> > +
> > +static DBusMessage *set_service_allowlist(DBusConnection *conn,
> > + DBusMessage *msg, void *user_data)
> > +{
> > + struct btd_admin_policy *admin_policy = user_data;
> > + struct btd_adapter *adapter = admin_policy->adapter;
> > + struct queue *uuid_list = NULL;
> > + const char *sender = dbus_message_get_sender(msg);
> > +
> > + DBG("sender %s", sender);
> > +
> > + /* Parse parameters */
> > + uuid_list = parse_allow_service_list(adapter, msg);
> > + if (!uuid_list) {
> > + btd_error(admin_policy->adapter_id,
> > + "Failed on parsing allowed service list");
> > + return btd_error_invalid_args(msg);
> > + }
> > +
> > + if (!service_allowlist_set(admin_policy, uuid_list)) {
> > + free_service_allowlist(uuid_list);
> > + return btd_error_failed(msg, "service_allowlist_set failed");
> > + }
> > +
> > + return dbus_message_new_method_return(msg);
> > +}
> > +
> > +static const GDBusMethodTable admin_policy_adapter_methods[] = {
> > + { GDBUS_METHOD("SetServiceAllowList", GDBUS_ARGS({ "UUIDs", "as" }),
> > + NULL, set_service_allowlist) },
> > + { }
> > +};
> > +
> > static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> > {
> > if (policy_data) {
> > @@ -64,8 +173,18 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> > if (!policy_data)
> > return -ENOMEM;
> >
> > - btd_info(policy_data->adapter_id, "Admin Policy has been enabled");
> > + if (!g_dbus_register_interface(dbus_conn, adapter_get_path(adapter),
> > + ADMIN_POLICY_SET_INTERFACE,
> > + admin_policy_adapter_methods, NULL,
> > + NULL, policy_data, admin_policy_free)) {
> > + btd_error(policy_data->adapter_id,
> > + "Admin Policy Set interface init failed on path %s",
> > + adapter_get_path(adapter));
> > + return -EINVAL;
> > + }
> >
> > + btd_info(policy_data->adapter_id,
> > + "Admin Policy Set interface registered");
> > return 0;
> > }
> >
> > @@ -79,6 +198,8 @@ static int admin_policy_init(void)
> > {
> > DBG("");
> >
> > + dbus_conn = btd_get_dbus_connection();
> > +
> > return btd_register_adapter_driver(&admin_policy_driver);
> > }
> >
> > --
> > 2.32.0.93.g670b81a890-goog
> >
>
>
> --
> Luiz Augusto von Dentz

2021-07-12 16:42:20

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method

Hi Howard,

On Mon, Jul 12, 2021 at 2:09 AM Yun-hao Chung <[email protected]> wrote:
>
> On Fri, Jul 9, 2021 at 2:01 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Howard,
> >
> > On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <[email protected]> wrote:
> > >
> > > From: Yun-Hao Chung <[email protected]>
> > >
> > > This adds code to register interface org.bluez.AdminPolicySet1.
> > > The interface will provide methods to limit users to operate certain
> > > functions of bluez, such as allow/disallow user to taggle adapter power,
> > > or only allow users to connect services in the specified list, etc.
> > >
> > > This patch also implements ServiceAllowlist in
> > > org.bluez.AdminPolicySet1.
> > >
> > > Reviewed-by: Miao-chen Chou <[email protected]>
> > > ---
> > > The following test steps were performed:
> > > 1. Set ServiceAllowList to
> > > ["0x1108","0x110A","0x110B","0x110C","0x110D","0x110E",
> > > "0x110F","0x1112","0x111E","0x111F","0x1203"]
> > > ( users are only allowed to connect headset )
> > > 2. Turn on paired WF1000XM3, and listen music on Youtube.
> > > 3. Turn on paired K830 (LE device), press any key on keyboard.
> > > 4. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> > > press any key on keyboard.
> > > 5. Set ServiceAllowList to
> > > ["1124","180A","180F","1812"]
> > > ( users are only allowed to connect HID devices )
> > > 6. Turn on paired WF1000XM3, and listen music on Youtube.
> > > 7. Turn on paired K830 (LE device), press any key on keyboard.
> > > 8. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> > > press any key on keyboard.
> > > 9. Set ServiceAllowList to []
> > > ( users are only allowed to connect any device. )
> > > 10. Turn on paired WF1000XM3, and listen music on Youtube.
> > > 11. Turn on paired K830 (LE device), press any key on keyboard.
> > > 12. Turn on paired Samsung Bluetooth Keyboard EE-BT550 (BREDR device),
> > > press any key on keyboard.
> > >
> > > Expected results:
> > > Step 2,7,8,9,10,11 should success, and step 3,4,6 should fail.
> >
> > All this explanation is great but it would really help if you have
> > added support for bluetoothctl to control this, we also need to
> Document it sounds good to me, but I notice that there is no document
> for any plugin yet.
> Where do you think we should put the document in?

Under doc (e.g. admin-policy.txt) should be fine since the plugin will
be in the tree anyway.

> > document these interfaces in case someone else wants to use them (e.g:
> > other clients like bluetoothctl). For the bluetoothctl we could
> > perhaps an admin menu registered whenever the interfaces are available
> > and then a command allow [list/none/any] so the user can query when no
> > parameter is given or set a list of UUIDs. Btw, how is this supposed
> Adding commands in bluetoothctl sounds good to me as well. Can we
> implement this in
> a separate series?

Sure.

> > to work with vendor UUIDs? I guess that would need to support UUID 128
> > bit format in order to support that.
> Since we are using bt_string_to_uuid to parse the given string,
> internally it checks if it can be parsed as UUID128/UUID32/UUID16.

Ok, that said in the comments you were using 0x so it sounded to me
the UUID type over D-Bus would be binary type not string, if we really
choose to support UUID 128 it is probably better to use string type
like we do with other APIs.

> >
> > >
> > > plugins/admin_policy.c | 123 ++++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 122 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
> > > index 2ece871564e6..242b8d5dacb0 100644
> > > --- a/plugins/admin_policy.c
> > > +++ b/plugins/admin_policy.c
> > > @@ -12,19 +12,29 @@
> > > #include <config.h>
> > > #endif
> > >
> > > +#include <dbus/dbus.h>
> > > +#include <gdbus/gdbus.h>
> > > +
> > > #include "lib/bluetooth.h"
> > > +#include "lib/uuid.h"
> > >
> > > #include "src/adapter.h"
> > > +#include "src/dbus-common.h"
> > > #include "src/error.h"
> > > #include "src/log.h"
> > > #include "src/plugin.h"
> > >
> > > #include "src/shared/queue.h"
> > >
> > > +#define ADMIN_POLICY_SET_INTERFACE "org.bluez.AdminPolicySet1"
> > > +
> > > +static DBusConnection *dbus_conn;
> > > +
> > > /* |policy_data| has the same life cycle as btd_adapter */
> > > static struct btd_admin_policy {
> > > struct btd_adapter *adapter;
> > > uint16_t adapter_id;
> > > + struct queue *service_allowlist;
> > > } *policy_data = NULL;
> > >
> > > static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
> > > @@ -40,17 +50,116 @@ static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
> > >
> > > admin_policy->adapter = adapter;
> > > admin_policy->adapter_id = btd_adapter_get_index(adapter);
> > > + admin_policy->service_allowlist = NULL;
> > >
> > > return admin_policy;
> > > }
> > >
> > > +static void free_service_allowlist(struct queue *q)
> > > +{
> > > + queue_destroy(q, g_free);
> > > +}
> > > +
> > > static void admin_policy_free(void *data)
> > > {
> > > struct btd_admin_policy *admin_policy = data;
> > >
> > > + free_service_allowlist(admin_policy->service_allowlist);
> > > g_free(admin_policy);
> > > }
> > >
> > > +static struct queue *parse_allow_service_list(struct btd_adapter *adapter,
> > > + DBusMessage *msg)
> > > +{
> > > + DBusMessageIter iter, arr_iter;
> > > + struct queue *uuid_list = NULL;
> > > +
> > > + dbus_message_iter_init(msg, &iter);
> > > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY)
> > > + return NULL;
> > > +
> > > + uuid_list = queue_new();
> > > + dbus_message_iter_recurse(&iter, &arr_iter);
> > > + do {
> > > + const int type = dbus_message_iter_get_arg_type(&arr_iter);
> > > + char *uuid_param;
> > > + bt_uuid_t *uuid;
> > > +
> > > + if (type == DBUS_TYPE_INVALID)
> > > + break;
> > > +
> > > + if (type != DBUS_TYPE_STRING)
> > > + goto failed;
> > > +
> > > + dbus_message_iter_get_basic(&arr_iter, &uuid_param);
> > > +
> > > + uuid = g_try_malloc(sizeof(*uuid));
> > > + if (!uuid)
> > > + goto failed;
> > > +
> > > + if (bt_string_to_uuid(uuid, uuid_param)) {
> > > + g_free(uuid);
> > > + goto failed;
> > > + }
> > > +
> > > + queue_push_head(uuid_list, uuid);
> > > +
> > > + dbus_message_iter_next(&arr_iter);
> > > + } while (true);
> > > +
> > > + return uuid_list;
> > > +
> > > +failed:
> > > + queue_destroy(uuid_list, g_free);
> > > + return NULL;
> > > +}
> > > +
> > > +static bool service_allowlist_set(struct btd_admin_policy *admin_policy,
> > > + struct queue *uuid_list)
> > > +{
> > > + struct btd_adapter *adapter = admin_policy->adapter;
> > > +
> > > + if (!btd_adapter_set_allowed_uuids(adapter, uuid_list))
> > > + return false;
> > > +
> > > + free_service_allowlist(admin_policy->service_allowlist);
> > > + admin_policy->service_allowlist = uuid_list;
> > > +
> > > + return true;
> > > +}
> > > +
> > > +static DBusMessage *set_service_allowlist(DBusConnection *conn,
> > > + DBusMessage *msg, void *user_data)
> > > +{
> > > + struct btd_admin_policy *admin_policy = user_data;
> > > + struct btd_adapter *adapter = admin_policy->adapter;
> > > + struct queue *uuid_list = NULL;
> > > + const char *sender = dbus_message_get_sender(msg);
> > > +
> > > + DBG("sender %s", sender);
> > > +
> > > + /* Parse parameters */
> > > + uuid_list = parse_allow_service_list(adapter, msg);
> > > + if (!uuid_list) {
> > > + btd_error(admin_policy->adapter_id,
> > > + "Failed on parsing allowed service list");
> > > + return btd_error_invalid_args(msg);
> > > + }
> > > +
> > > + if (!service_allowlist_set(admin_policy, uuid_list)) {
> > > + free_service_allowlist(uuid_list);
> > > + return btd_error_failed(msg, "service_allowlist_set failed");
> > > + }
> > > +
> > > + return dbus_message_new_method_return(msg);
> > > +}
> > > +
> > > +static const GDBusMethodTable admin_policy_adapter_methods[] = {
> > > + { GDBUS_METHOD("SetServiceAllowList", GDBUS_ARGS({ "UUIDs", "as" }),
> > > + NULL, set_service_allowlist) },
> > > + { }
> > > +};
> > > +
> > > static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> > > {
> > > if (policy_data) {
> > > @@ -64,8 +173,18 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
> > > if (!policy_data)
> > > return -ENOMEM;
> > >
> > > - btd_info(policy_data->adapter_id, "Admin Policy has been enabled");
> > > + if (!g_dbus_register_interface(dbus_conn, adapter_get_path(adapter),
> > > + ADMIN_POLICY_SET_INTERFACE,
> > > + admin_policy_adapter_methods, NULL,
> > > + NULL, policy_data, admin_policy_free)) {
> > > + btd_error(policy_data->adapter_id,
> > > + "Admin Policy Set interface init failed on path %s",
> > > + adapter_get_path(adapter));
> > > + return -EINVAL;
> > > + }
> > >
> > > + btd_info(policy_data->adapter_id,
> > > + "Admin Policy Set interface registered");
> > > return 0;
> > > }
> > >
> > > @@ -79,6 +198,8 @@ static int admin_policy_init(void)
> > > {
> > > DBG("");
> > >
> > > + dbus_conn = btd_get_dbus_connection();
> > > +
> > > return btd_register_adapter_driver(&admin_policy_driver);
> > > }
> > >
> > > --
> > > 2.32.0.93.g670b81a890-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



--
Luiz Augusto von Dentz