linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Bluez PATCH v1 00/14]
@ 2021-07-08  6:23 Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t Howard Chung
                   ` (13 more replies)
  0 siblings, 14 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung

From: Yun-Hao Chung <howardchung@chromium.org>

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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:36   ` [Bluez,v1,01/14] " bluez.test.bot
  2021-07-09  5:21   ` [Bluez PATCH v1 01/14] " Luiz Augusto von Dentz
  2021-07-08  6:23 ` [Bluez PATCH v1 02/14] unit: add uuid unit tests Howard Chung
                   ` (12 subsequent siblings)
  13 siblings, 2 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Howard Chung, Miao-chen Chou

This adds function GHashFunc and GEqualFunc for bt_uuid_t.
With these functions, we can add uuids into a GHashTable with bt_uuid_t
format.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 lib/uuid.c | 27 +++++++++++++++++++++++++++
 lib/uuid.h |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/lib/uuid.c b/lib/uuid.c
index 3d97dc8359c7..a09f5c428b87 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -16,6 +16,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <glib.h>
 
 #include "lib/bluetooth.h"
 #include "uuid.h"
@@ -120,6 +121,32 @@ int bt_uuid_cmp(const bt_uuid_t *uuid1, const bt_uuid_t *uuid2)
 	return bt_uuid128_cmp(&u1, &u2);
 }
 
+guint bt_uuid_hash(gconstpointer key)
+{
+	const bt_uuid_t *uuid = key;
+	bt_uuid_t uuid_128;
+	uint64_t *val;
+
+	if (!uuid)
+		return 0;
+
+	bt_uuid_to_uuid128(uuid, &uuid_128);
+	val = (uint64_t *)&uuid_128.value.u128;
+
+	return g_int64_hash(val) ^ g_int64_hash(val+1);
+}
+
+gboolean bt_uuid_equal(gconstpointer v1, gconstpointer v2)
+{
+	const bt_uuid_t *uuid1 = v1;
+	const bt_uuid_t *uuid2 = v2;
+
+	if (!uuid1 || !uuid2)
+		return !uuid1 && !uuid2;
+
+	return bt_uuid_cmp(uuid1, uuid2) == 0;
+}
+
 /*
  * convert the UUID to string, copying a maximum of n characters.
  */
diff --git a/lib/uuid.h b/lib/uuid.h
index 1a4029b68730..e47ccccb9fd2 100644
--- a/lib/uuid.h
+++ b/lib/uuid.h
@@ -17,6 +17,7 @@ extern "C" {
 #endif
 
 #include <stdint.h>
+#include <glib.h>
 
 #define GENERIC_AUDIO_UUID	"00001203-0000-1000-8000-00805f9b34fb"
 
@@ -167,6 +168,8 @@ int bt_uuid128_create(bt_uuid_t *btuuid, uint128_t value);
 
 int bt_uuid_cmp(const bt_uuid_t *uuid1, const bt_uuid_t *uuid2);
 void bt_uuid_to_uuid128(const bt_uuid_t *src, bt_uuid_t *dst);
+guint bt_uuid_hash(gconstpointer key);
+gboolean bt_uuid_equal(gconstpointer v1, gconstpointer v2);
 
 #define MAX_LEN_UUID_STR 37
 
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 02/14] unit: add uuid unit tests
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 03/14] core: add is_allowed property in btd_service Howard Chung
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

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

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 03/14] core: add is_allowed property in btd_service
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 02/14] unit: add uuid unit tests Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 04/14] core: add adapter and device allowed_uuid functions Howard Chung
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

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 <mcchou@chromium.org>
---

 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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 04/14] core: add adapter and device allowed_uuid functions
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (2 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 03/14] core: add is_allowed property in btd_service Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 05/14] core: add device state and state callbacks Howard Chung
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

This implements functions in src/adapter.c and src/device.c for
plugins setting a list of allowed services.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 src/adapter.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/adapter.h |  8 +++++
 src/device.c  | 59 ++++++++++++++++++++++++++++++++-
 src/device.h  |  2 ++
 4 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/src/adapter.c b/src/adapter.c
index 84bc5a1b09eb..f75b8acf1ffb 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -260,6 +260,8 @@ struct btd_adapter {
 
 	struct btd_battery_provider_manager *battery_provider_manager;
 
+	GHashTable *allowed_uuid_set;	/* Set of allowed service UUIDs */
+
 	gboolean initialized;
 
 	GSList *pin_callbacks;
@@ -3480,6 +3482,87 @@ static DBusMessage *connect_device(DBusConnection *conn,
 	return NULL;
 }
 
+static void update_adapter_profile(struct btd_profile *profile, void *data)
+{
+	struct btd_adapter *adapter = data;
+	bool is_allowed;
+
+	is_allowed = btd_adapter_is_uuid_allowed(adapter, profile->remote_uuid);
+
+	if (is_allowed && !g_slist_find(adapter->profiles, profile)) {
+		adapter_add_profile(adapter, profile);
+
+		info("service %s is allowed", profile->remote_uuid);
+
+	} else if (!is_allowed && g_slist_find(adapter->profiles, profile)) {
+		adapter_remove_profile(adapter, profile);
+
+		info("service %s is blocked", profile->remote_uuid);
+	}
+}
+
+static void update_device_allowed_services(void *data, void *user_data)
+{
+	struct btd_device *device = data;
+
+	btd_device_update_allowed_services(device);
+}
+
+static void add_uuid_to_uuid_set(void *data, void *user_data)
+{
+	bt_uuid_t *uuid = data;
+	GHashTable *uuid_set = user_data;
+
+	if (!uuid) {
+		error("Found NULL in UUID allowed list");
+		return;
+	}
+
+	g_hash_table_add(uuid_set, uuid);
+}
+
+bool btd_adapter_set_allowed_uuids(struct btd_adapter *adapter,
+							struct queue *uuids)
+{
+	if (!adapter)
+		return false;
+
+	if (adapter->allowed_uuid_set)
+		g_hash_table_destroy(adapter->allowed_uuid_set);
+
+	adapter->allowed_uuid_set = g_hash_table_new(bt_uuid_hash,
+								bt_uuid_equal);
+	if (!adapter->allowed_uuid_set) {
+		btd_error(adapter->dev_id,
+					"Failed to allocate allowed_uuid_set");
+		return false;
+	}
+
+	queue_foreach(uuids, add_uuid_to_uuid_set, adapter->allowed_uuid_set);
+	btd_profile_foreach(update_adapter_profile, adapter);
+	g_slist_foreach(adapter->devices, update_device_allowed_services, NULL);
+
+	return true;
+}
+
+bool btd_adapter_is_uuid_allowed(struct btd_adapter *adapter,
+							const char *uuid_str)
+{
+	bt_uuid_t uuid;
+
+	if (!adapter || !adapter->allowed_uuid_set)
+		return true;
+
+	if (bt_string_to_uuid(&uuid, uuid_str)) {
+		btd_error(adapter->dev_id,
+				"Failed to parse UUID string '%s'", uuid_str);
+		return false;
+	}
+
+	return !g_hash_table_size(adapter->allowed_uuid_set) ||
+		g_hash_table_contains(adapter->allowed_uuid_set, &uuid);
+}
+
 static const GDBusMethodTable adapter_methods[] = {
 	{ GDBUS_ASYNC_METHOD("StartDiscovery", NULL, NULL, start_discovery) },
 	{ GDBUS_METHOD("SetDiscoveryFilter",
@@ -4691,6 +4774,12 @@ static void probe_profile(struct btd_profile *profile, void *data)
 	if (profile->adapter_probe == NULL)
 		return;
 
+	if (!btd_adapter_is_uuid_allowed(adapter, profile->remote_uuid)) {
+		info("service %s is not allowed to probe",
+							profile->remote_uuid);
+		return;
+	}
+
 	err = profile->adapter_probe(profile, adapter);
 	if (err < 0) {
 		btd_error(adapter->dev_id, "%s: %s (%d)", profile->name,
@@ -5395,6 +5484,7 @@ static void adapter_free(gpointer user_data)
 	g_free(adapter->stored_alias);
 	g_free(adapter->current_alias);
 	free(adapter->modalias);
+	g_hash_table_destroy(adapter->allowed_uuid_set);
 	g_free(adapter);
 }
 
diff --git a/src/adapter.h b/src/adapter.h
index 60b5e3bcca34..7cac51451249 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -25,6 +25,7 @@
 
 struct btd_adapter;
 struct btd_device;
+struct queue;
 
 struct btd_adapter *btd_adapter_get_default(void);
 bool btd_adapter_is_default(struct btd_adapter *adapter);
@@ -97,6 +98,8 @@ void adapter_service_remove(struct btd_adapter *adapter, uint32_t handle);
 
 struct agent *adapter_get_agent(struct btd_adapter *adapter);
 
+bool btd_adapter_uuid_is_allowed(struct btd_adapter *adapter, const char *uuid);
+
 struct btd_adapter *btd_adapter_ref(struct btd_adapter *adapter);
 void btd_adapter_unref(struct btd_adapter *adapter);
 
@@ -240,3 +243,8 @@ enum kernel_features {
 };
 
 bool btd_has_kernel_features(uint32_t feature);
+
+bool btd_adapter_set_allowed_uuids(struct btd_adapter *adapter,
+							struct queue *uuids);
+bool btd_adapter_is_uuid_allowed(struct btd_adapter *adapter,
+							const char *uuid_str);
diff --git a/src/device.c b/src/device.c
index faf07ba22270..e1d82eab0988 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1929,6 +1929,51 @@ static int service_prio_cmp(gconstpointer a, gconstpointer b)
 	return p2->priority - p1->priority;
 }
 
+bool btd_device_all_services_allowed(struct btd_device *dev)
+{
+	GSList *l;
+	struct btd_adapter *adapter = dev->adapter;
+	char *uuid;
+
+	for (l = dev->uuids; l != NULL; l = g_slist_next(l)) {
+		uuid = l->data;
+
+		if (!btd_adapter_is_uuid_allowed(adapter, uuid))
+			return false;
+	}
+
+	return true;
+}
+
+void btd_device_update_allowed_services(struct btd_device *dev)
+{
+	struct btd_adapter *adapter = dev->adapter;
+	struct btd_service *service;
+	struct btd_profile *profile;
+	GSList *l;
+	bool is_allowed;
+	char addr[18];
+
+	/* If service discovery is ongoing, let the service discovery complete
+	 * callback call this function.
+	 */
+	if (dev->browse) {
+		ba2str(&dev->bdaddr, addr);
+		DBG("service discovery of %s is ongoing. Skip updating allowed "
+							"services", addr);
+		return;
+	}
+
+	for (l = dev->services; l != NULL; l = g_slist_next(l)) {
+		service = l->data;
+		profile = btd_service_get_profile(service);
+
+		is_allowed = btd_adapter_is_uuid_allowed(adapter,
+							profile->remote_uuid);
+		btd_service_set_allowed(service, is_allowed);
+	}
+}
+
 static GSList *create_pending_list(struct btd_device *dev, const char *uuid)
 {
 	struct btd_service *service;
@@ -1937,9 +1982,14 @@ static GSList *create_pending_list(struct btd_device *dev, const char *uuid)
 
 	if (uuid) {
 		service = find_connectable_service(dev, uuid);
-		if (service)
+
+		if (!service)
+			return dev->pending;
+
+		if (btd_service_is_allowed(service))
 			return g_slist_prepend(dev->pending, service);
 
+		info("service %s is blocked", uuid);
 		return dev->pending;
 	}
 
@@ -1950,6 +2000,11 @@ static GSList *create_pending_list(struct btd_device *dev, const char *uuid)
 		if (!p->auto_connect)
 			continue;
 
+		if (!btd_service_is_allowed(service)) {
+			info("service %s is blocked", p->remote_uuid);
+			continue;
+		}
+
 		if (g_slist_find(dev->pending, service))
 			continue;
 
@@ -2633,6 +2688,8 @@ static void device_svc_resolved(struct btd_device *dev, uint8_t browse_type,
 							dev->svc_callbacks);
 		g_free(cb);
 	}
+
+	btd_device_update_allowed_services(dev);
 }
 
 static struct bonding_req *bonding_request_new(DBusMessage *msg,
diff --git a/src/device.h b/src/device.h
index 4ae9abe0dbb4..5f615cb4b6b2 100644
--- a/src/device.h
+++ b/src/device.h
@@ -175,5 +175,7 @@ uint32_t btd_device_get_current_flags(struct btd_device *dev);
 void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
 			      uint32_t current_flags);
 
+bool btd_device_all_services_allowed(struct btd_device *dev);
+void btd_device_update_allowed_services(struct btd_device *dev);
 void btd_device_init(void);
 void btd_device_cleanup(void);
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 05/14] core: add device state and state callbacks
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (3 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 04/14] core: add adapter and device allowed_uuid functions Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-09  5:34   ` Luiz Augusto von Dentz
  2021-07-08  6:23 ` [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed Howard Chung
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

This implements functions to register/unregister state changed callback
functions, the functions will be called when a device's state changed.
Currently the state only shows initializing, available and removing,
which is enough for plugins to register dbus objects upon device
creation and unregister it upon device removal.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 src/device.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.h | 13 +++++++++++
 2 files changed, 77 insertions(+)

diff --git a/src/device.c b/src/device.c
index e1d82eab0988..0d7444706336 100644
--- a/src/device.c
+++ b/src/device.c
@@ -81,6 +81,13 @@
 
 static DBusConnection *dbus_conn = NULL;
 static unsigned service_state_cb_id;
+static GSList *device_state_callbacks;
+
+struct device_state_callback {
+	btd_device_state_cb	cb;
+	void			*user_data;
+	unsigned int		id;
+};
 
 struct btd_disconnect_data {
 	guint id;
@@ -272,6 +279,8 @@ struct btd_device {
 
 	GIOChannel	*att_io;
 	guint		store_id;
+
+	enum btd_device_state_t state;
 };
 
 static const uint16_t uuid_list[] = {
@@ -4095,6 +4104,23 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
 	gatt_services_changed(device);
 }
 
+static void device_change_state(struct btd_device *device,
+					enum btd_device_state_t new_state)
+{
+	GSList *l;
+	struct device_state_callback *cb_data;
+
+	if (device->state == new_state)
+		return;
+
+	for (l = device_state_callbacks; l != NULL; l = g_slist_next(l)) {
+		cb_data = l->data;
+		cb_data->cb(device, new_state, cb_data->user_data);
+	}
+
+	device->state = new_state;
+}
+
 static struct btd_device *device_new(struct btd_adapter *adapter,
 				const char *address)
 {
@@ -4158,6 +4184,8 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
 
 	device->refresh_discovery = btd_opts.refresh_discovery;
 
+	device_change_state(device, BTD_DEVICE_STATE_AVAILABLE);
+
 	return btd_device_ref(device);
 }
 
@@ -6839,6 +6867,7 @@ void btd_device_unref(struct btd_device *device)
 
 	DBG("Freeing device %s", device->path);
 
+	device_change_state(device, BTD_DEVICE_STATE_REMOVING);
 	g_dbus_unregister_interface(dbus_conn, device->path, DEVICE_INTERFACE);
 }
 
@@ -6980,3 +7009,38 @@ void btd_device_cleanup(void)
 {
 	btd_service_remove_state_cb(service_state_cb_id);
 }
+
+unsigned int btd_device_add_state_cb(btd_device_state_cb cb, void *user_data)
+{
+	struct device_state_callback *cb_data;
+	static unsigned int id;
+
+	cb_data = g_new0(struct device_state_callback, 1);
+	cb_data->cb = cb;
+	cb_data->user_data = user_data;
+	cb_data->id = ++id;
+
+	device_state_callbacks = g_slist_append(device_state_callbacks,
+								cb_data);
+
+	return cb_data->id;
+}
+
+bool btd_device_remove_state_cb(unsigned int id)
+{
+	GSList *l;
+
+	for (l = device_state_callbacks; l != NULL; l = g_slist_next(l)) {
+		struct device_state_callback *cb_data = l->data;
+
+		if (cb_data && cb_data->id == id) {
+			device_state_callbacks = g_slist_remove(
+							device_state_callbacks,
+							cb_data);
+			g_free(cb_data);
+			return true;
+		}
+	}
+
+	return false;
+}
diff --git a/src/device.h b/src/device.h
index 5f615cb4b6b2..a8424aa4f098 100644
--- a/src/device.h
+++ b/src/device.h
@@ -11,8 +11,18 @@
 
 #define DEVICE_INTERFACE	"org.bluez.Device1"
 
+enum btd_device_state_t {
+	BTD_DEVICE_STATE_INITIALIZING,	/* Device object is creating */
+	BTD_DEVICE_STATE_AVAILABLE,	/* Device object is registered */
+	BTD_DEVICE_STATE_REMOVING,	/* Device object is being removed */
+};
+
 struct btd_device;
 
+typedef void (*btd_device_state_cb) (struct btd_device *device,
+					enum btd_device_state_t new_state,
+					void *user_data);
+
 struct btd_device *device_create(struct btd_adapter *adapter,
 				const bdaddr_t *address, uint8_t bdaddr_type);
 struct btd_device *device_create_from_storage(struct btd_adapter *adapter,
@@ -179,3 +189,6 @@ bool btd_device_all_services_allowed(struct btd_device *dev);
 void btd_device_update_allowed_services(struct btd_device *dev);
 void btd_device_init(void);
 void btd_device_cleanup(void);
+
+unsigned int btd_device_add_state_cb(btd_device_state_cb cb, void *user_data);
+bool btd_device_remove_state_cb(unsigned int id);
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (4 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 05/14] core: add device state and state callbacks Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-09  5:49   ` Luiz Augusto von Dentz
  2021-07-08  6:23 ` [Bluez PATCH v1 07/14] plugins: add a new plugin for admin_policy Howard Chung
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Howard Chung, Miao-chen Chou

When A2DP source profile is removed from adapter, a2dp_server and
everything inside the object will be removed, which also releases all
MediaEndpoints and MediaPlayer. When A2DP source profile is re-added,
although a2dp_server will be created, clients are not able to know they
can register their endpoints and player by then.

This patch handles this case by unregistering Media1 interface
when we remove a2dp_server, and register it back when a2dp_source is
allowed.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---
perform following steps
1. SetServiceAllowList to empty list
2. pair with an LE headset, then turn off the headset
3. SetServiceAllowList to "0x1234"
4. SetServiceAllowList to empty list
5. turn on the headset and check if it is reconnected.

 profiles/audio/a2dp.c  | 2 ++
 profiles/audio/avrcp.c | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index d31ed845cbe7..26d4f365207e 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -3275,6 +3275,7 @@ static int a2dp_source_server_probe(struct btd_profile *p,
 {
 	struct a2dp_server *server;
 
+	media_register(adapter);
 	DBG("path %s", adapter_get_path(adapter));
 
 	server = find_server(servers, adapter);
@@ -3315,6 +3316,7 @@ static void a2dp_source_server_remove(struct btd_profile *p,
 		return;
 
 	a2dp_server_unregister(server);
+	media_unregister(adapter);
 }
 
 static int a2dp_sink_server_probe(struct btd_profile *p,
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index ccf34b2207a9..997a5be9a0f4 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -4751,6 +4751,8 @@ static void avrcp_controller_server_remove(struct btd_profile *p,
 
 	if (server->tg_record_id == 0)
 		avrcp_server_unregister(server);
+
+	media_unregister(adapter);
 }
 
 static int avrcp_controller_server_probe(struct btd_profile *p,
@@ -4761,6 +4763,7 @@ static int avrcp_controller_server_probe(struct btd_profile *p,
 
 	DBG("path %s", adapter_get_path(adapter));
 
+	media_register(adapter);
 	server = find_server(servers, adapter);
 	if (server != NULL)
 		goto done;
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 07/14] plugins: add a new plugin for admin_policy
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (5 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 08/14] plugins/admin_policy: add admin_policy adapter driver Howard Chung
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou, Shyh-In Hwang

From: Yun-Hao Chung <howardchung@chromium.org>

This adds an initial code for a new plugin admin_policy.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Shyh-In Hwang <josephsih@chromium.org>
---

 Makefile.plugins       |  5 +++++
 bootstrap-configure    |  1 +
 configure.ac           |  4 ++++
 plugins/admin_policy.c | 30 ++++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+)
 create mode 100644 plugins/admin_policy.c

diff --git a/Makefile.plugins b/Makefile.plugins
index 4e6a72b0bdf6..b6be0e0d559d 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -11,6 +11,11 @@ builtin_sources += plugins/autopair.c
 builtin_modules += policy
 builtin_sources += plugins/policy.c
 
+if ADMIN_POLICY
+builtin_modules += admin_policy
+builtin_sources += plugins/admin_policy.c
+endif
+
 if NFC
 builtin_modules += neard
 builtin_sources += plugins/neard.c
diff --git a/bootstrap-configure b/bootstrap-configure
index 0efd83abc2c4..89c0747b0256 100755
--- a/bootstrap-configure
+++ b/bootstrap-configure
@@ -30,4 +30,5 @@ fi
 		--enable-pie \
 		--enable-cups \
 		--enable-library \
+		--enable-admin_policy \
 		--disable-datafiles $*
diff --git a/configure.ac b/configure.ac
index be32782a641d..53ed8911f95c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -364,6 +364,10 @@ AC_ARG_ENABLE(logger, AC_HELP_STRING([--enable-logger],
 		[enable HCI logger service]), [enable_logger=${enableval}])
 AM_CONDITIONAL(LOGGER, test "${enable_logger}" = "yes")
 
+AC_ARG_ENABLE(admin_policy, AC_HELP_STRING([--enable-admin_policy],
+		[enable admin policy plugin]), [enable_admin_policy=${enableval}])
+AM_CONDITIONAL(ADMIN_POLICY, test "${enable_admin_policy}" = "yes")
+
 if (test "${prefix}" = "NONE"); then
 	dnl no prefix and no localstatedir, so default to /var
 	if (test "$localstatedir" = '${prefix}/var'); then
diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
new file mode 100644
index 000000000000..dd8d8973636f
--- /dev/null
+++ b/plugins/admin_policy.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2021 Google LLC
+ *
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include "src/log.h"
+#include "src/plugin.h"
+
+static int admin_policy_init(void)
+{
+	DBG("");
+}
+
+static void admin_policy_exit(void)
+{
+	DBG("");
+}
+
+BLUETOOTH_PLUGIN_DEFINE(admin_policy, VERSION,
+			BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
+			admin_policy_init, admin_policy_exit)
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 08/14] plugins/admin_policy: add admin_policy adapter driver
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (6 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 07/14] plugins: add a new plugin for admin_policy Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method Howard Chung
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

This adds code to register admin_policy driver to adapter when
admin_policy plugin is enabled.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---
The following test steps were performed:
1. restart bluetoothd
2. check if "Admin Policy is enabled" in system log

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

diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
index dd8d8973636f..2ece871564e6 100644
--- a/plugins/admin_policy.c
+++ b/plugins/admin_policy.c
@@ -12,17 +12,84 @@
 #include <config.h>
 #endif
 
+#include "lib/bluetooth.h"
+
+#include "src/adapter.h"
+#include "src/error.h"
 #include "src/log.h"
 #include "src/plugin.h"
 
+#include "src/shared/queue.h"
+
+/* |policy_data| has the same life cycle as btd_adapter */
+static struct btd_admin_policy {
+	struct btd_adapter *adapter;
+	uint16_t adapter_id;
+} *policy_data = NULL;
+
+static struct btd_admin_policy *admin_policy_new(struct btd_adapter *adapter)
+{
+	struct btd_admin_policy *admin_policy = NULL;
+
+	admin_policy = g_try_malloc(sizeof(*admin_policy));
+	if (!admin_policy) {
+		btd_error(btd_adapter_get_index(adapter),
+				"Failed to allocate memory for admin_policy");
+		return NULL;
+	}
+
+	admin_policy->adapter = adapter;
+	admin_policy->adapter_id = btd_adapter_get_index(adapter);
+
+	return admin_policy;
+}
+
+static void admin_policy_free(void *data)
+{
+	struct btd_admin_policy *admin_policy = data;
+
+	g_free(admin_policy);
+}
+
+static int admin_policy_adapter_probe(struct btd_adapter *adapter)
+{
+	if (policy_data) {
+		btd_warn(policy_data->adapter_id,
+						"Policy data already exists");
+		admin_policy_free(policy_data);
+		policy_data = NULL;
+	}
+
+	policy_data = admin_policy_new(adapter);
+	if (!policy_data)
+		return -ENOMEM;
+
+	btd_info(policy_data->adapter_id, "Admin Policy has been enabled");
+
+	return 0;
+}
+
+static struct btd_adapter_driver admin_policy_driver = {
+	.name	= "admin_policy",
+	.probe	= admin_policy_adapter_probe,
+	.resume = NULL,
+};
+
 static int admin_policy_init(void)
 {
 	DBG("");
+
+	return btd_register_adapter_driver(&admin_policy_driver);
 }
 
 static void admin_policy_exit(void)
 {
 	DBG("");
+
+	btd_unregister_adapter_driver(&admin_policy_driver);
+
+	if (policy_data)
+		admin_policy_free(policy_data);
 }
 
 BLUETOOTH_PLUGIN_DEFINE(admin_policy, VERSION,
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (7 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 08/14] plugins/admin_policy: add admin_policy adapter driver Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-09  6:01   ` Luiz Augusto von Dentz
  2021-07-08  6:23 ` [Bluez PATCH v1 10/14] plugins/admin_policy: add ServiceAllowList property Howard Chung
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

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 <mcchou@chromium.org>
---
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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 10/14] plugins/admin_policy: add ServiceAllowList property
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (8 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 11/14] plugins/admin_policy: add device state callback Howard Chung
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

This adds code to register interface org.bluez.AdminPolicyStatus.
The interface will provide read-only properties to indicate the current
settings of admin policies. We separate this from AdminPolicySet so that
normal clients can check current policy settings while only a few
clients can change policies.

This patch also adds readonly property ServiceAllowlist to
AdminPolicyStatus1, which indicates the current setting of service
allowlist.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---
The following test steps were performed:
1. Set ServiceAllowList to ["1124","180A","180F","1812"]
2. Verify ServiceAllowList is ["1124","180A","180F","1812"] in UUID-128
   form
3. Set ServiceAllowList to []
4. Verify ServiceAllowList is []

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

diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
index 242b8d5dacb0..c5ad31f761d9 100644
--- a/plugins/admin_policy.c
+++ b/plugins/admin_policy.c
@@ -27,6 +27,7 @@
 #include "src/shared/queue.h"
 
 #define ADMIN_POLICY_SET_INTERFACE	"org.bluez.AdminPolicySet1"
+#define ADMIN_POLICY_STATUS_INTERFACE	"org.bluez.AdminPolicyStatus1"
 
 static DBusConnection *dbus_conn;
 
@@ -151,6 +152,10 @@ static DBusMessage *set_service_allowlist(DBusConnection *conn,
 		return btd_error_failed(msg, "service_allowlist_set failed");
 	}
 
+	g_dbus_emit_property_changed(dbus_conn,
+				adapter_get_path(policy_data->adapter),
+				ADMIN_POLICY_SET_INTERFACE, "ServiceAllowList");
+
 	return dbus_message_new_method_return(msg);
 }
 
@@ -160,6 +165,43 @@ static const GDBusMethodTable admin_policy_adapter_methods[] = {
 	{ }
 };
 
+void append_service_uuid(void *data, void *user_data)
+{
+	bt_uuid_t *uuid = data;
+	DBusMessageIter *entry = user_data;
+	char uuid_str[MAX_LEN_UUID_STR];
+	const char *uuid_str_ptr = uuid_str;
+
+	if (!uuid) {
+		error("Unexpected NULL uuid data in service_allowlist");
+		return;
+	}
+
+	bt_uuid_to_string(uuid, uuid_str, MAX_LEN_UUID_STR);
+	dbus_message_iter_append_basic(entry, DBUS_TYPE_STRING, &uuid_str_ptr);
+}
+
+static gboolean property_get_service_allowlist(
+					const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *user_data)
+{
+	struct btd_admin_policy *admin_policy = user_data;
+	DBusMessageIter entry;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+					DBUS_TYPE_STRING_AS_STRING, &entry);
+	queue_foreach(admin_policy->service_allowlist, append_service_uuid,
+									&entry);
+	dbus_message_iter_close_container(iter, &entry);
+
+	return TRUE;
+}
+
+static const GDBusPropertyTable admin_policy_adapter_properties[] = {
+	{ "ServiceAllowList", "as", property_get_service_allowlist },
+	{ }
+};
+
 static int admin_policy_adapter_probe(struct btd_adapter *adapter)
 {
 	if (policy_data) {
@@ -185,6 +227,21 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
 
 	btd_info(policy_data->adapter_id,
 				"Admin Policy Set interface registered");
+
+	if (!g_dbus_register_interface(dbus_conn, adapter_get_path(adapter),
+					ADMIN_POLICY_STATUS_INTERFACE,
+					NULL, NULL,
+					admin_policy_adapter_properties,
+					policy_data, admin_policy_free)) {
+		btd_error(policy_data->adapter_id,
+			"Admin Policy Status interface init failed on path %s",
+						adapter_get_path(adapter));
+		return -EINVAL;
+	}
+
+	btd_info(policy_data->adapter_id,
+				"Admin Policy Status interface registered");
+
 	return 0;
 }
 
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 11/14] plugins/admin_policy: add device state callback
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (9 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 10/14] plugins/admin_policy: add ServiceAllowList property Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 12/14] plugins/admin_policy: add AffectedByPolicy property Howard Chung
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

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 <mcchou@chromium.org>
---
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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 12/14] plugins/admin_policy: add AffectedByPolicy property
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (10 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 11/14] plugins/admin_policy: add device state callback Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 13/14] plugins/admin_policy: persist policy settings Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 14/14] core: fix a possible crash when removing devices Howard Chung
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

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

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---
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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 13/14] plugins/admin_policy: persist policy settings
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (11 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 12/14] plugins/admin_policy: add AffectedByPolicy property Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  2021-07-08  6:23 ` [Bluez PATCH v1 14/14] core: fix a possible crash when removing devices Howard Chung
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Miao-chen Chou

From: Yun-Hao Chung <howardchung@chromium.org>

This adds code to store the ServiceAllowlist to file
/var/lib/bluetooth/{MAC_ADDR}/admin_policy
The stored settings will be loaded upon admin_policy initialized.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---
The following test steps were performed:
1. Set ServiceAllowlist to ["1124","180A","180F","1812", "1801"]
2. restart bluetoothd
3. Verify ServiceAllowlist is ["1124","180A","180F","1812","1801"] in
   UUID-128 form
4. Set ServiceAllowlist to []
5. restart bluetoothd
6. Verify ServiceAllowlist is []

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

diff --git a/plugins/admin_policy.c b/plugins/admin_policy.c
index be4ba096a8b9..168848c82a63 100644
--- a/plugins/admin_policy.c
+++ b/plugins/admin_policy.c
@@ -14,6 +14,8 @@
 
 #include <dbus/dbus.h>
 #include <gdbus/gdbus.h>
+#include <sys/file.h>
+#include <sys/stat.h>
 
 #include "lib/bluetooth.h"
 #include "lib/uuid.h"
@@ -24,12 +26,15 @@
 #include "src/error.h"
 #include "src/log.h"
 #include "src/plugin.h"
+#include "src/textfile.h"
 
 #include "src/shared/queue.h"
 
 #define ADMIN_POLICY_SET_INTERFACE	"org.bluez.AdminPolicySet1"
 #define ADMIN_POLICY_STATUS_INTERFACE	"org.bluez.AdminPolicyStatus1"
 
+#define ADMIN_POLICY_STORAGE		STORAGEDIR "/admin_policy_settings"
+
 static DBusConnection *dbus_conn;
 static struct queue *devices; /* List of struct device_data objects */
 static unsigned int device_cb_id;
@@ -159,6 +164,8 @@ static void update_device_affected(void *data, void *user_data)
 			ADMIN_POLICY_STATUS_INTERFACE, "AffectedByPolicy");
 }
 
+static void store_policy_settings(struct btd_admin_policy *admin_policy);
+
 static DBusMessage *set_service_allowlist(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
@@ -177,7 +184,9 @@ static DBusMessage *set_service_allowlist(DBusConnection *conn,
 		return btd_error_invalid_args(msg);
 	}
 
-	if (!service_allowlist_set(admin_policy, uuid_list)) {
+	if (service_allowlist_set(admin_policy, uuid_list)) {
+		store_policy_settings(admin_policy);
+	} else {
 		free_service_allowlist(uuid_list);
 		return btd_error_failed(msg, "service_allowlist_set failed");
 	}
@@ -234,6 +243,158 @@ static const GDBusPropertyTable admin_policy_adapter_properties[] = {
 	{ }
 };
 
+static void free_uuid_strings(char **uuid_strs, int num)
+{
+	gsize i;
+
+	for (i = 0; i < num; i++)
+		g_free(uuid_strs[i]);
+	g_free(uuid_strs);
+}
+
+static char **new_uuid_strings(struct queue *allowlist, gsize *num)
+{
+	const struct queue_entry *entry = NULL;
+	bt_uuid_t *uuid = NULL;
+	char **uuid_strs = NULL;
+	gsize i = 0, allowlist_num;
+
+	allowlist_num = queue_length(allowlist);
+	uuid_strs = g_try_malloc_n(allowlist_num, sizeof(char *));
+	if (!uuid_strs)
+		return NULL;
+
+	for (entry = queue_get_entries(allowlist); entry != NULL;
+							entry = entry->next) {
+		uuid = entry->data;
+		uuid_strs[i] = g_try_malloc0(MAX_LEN_UUID_STR * sizeof(char));
+
+		if (!uuid_strs[i])
+			goto failed;
+
+		bt_uuid_to_string(uuid, uuid_strs[i], MAX_LEN_UUID_STR);
+		i++;
+	}
+
+	*num = allowlist_num;
+	return uuid_strs;
+
+failed:
+	free_uuid_strings(uuid_strs, i);
+
+	return NULL;
+}
+
+static void store_policy_settings(struct btd_admin_policy *admin_policy)
+{
+	GKeyFile *key_file = NULL;
+	char *filename = ADMIN_POLICY_STORAGE;
+	char *key_file_data = NULL;
+	char **uuid_strs = NULL;
+	gsize length, num_uuids;
+
+	key_file = g_key_file_new();
+
+	if (num_uuids) {
+		uuid_strs = new_uuid_strings(admin_policy->service_allowlist,
+								&num_uuids);
+	}
+
+	if (!uuid_strs && num_uuids) {
+		btd_error(admin_policy->adapter_id,
+					"Failed to allocate uuid strings");
+		goto failed;
+	}
+
+	g_key_file_set_string_list(key_file, "General", "ServiceAllowlist",
+					(const gchar * const *)uuid_strs,
+					num_uuids);
+
+	if (create_file(ADMIN_POLICY_STORAGE, 0600) < 0) {
+		btd_error(admin_policy->adapter_id, "create %s failed, %s",
+						filename, strerror(errno));
+		goto failed;
+	}
+
+	key_file_data = g_key_file_to_data(key_file, &length, NULL);
+	g_file_set_contents(ADMIN_POLICY_STORAGE, key_file_data, length, NULL);
+
+	g_free(key_file_data);
+	free_uuid_strings(uuid_strs, num_uuids);
+
+failed:
+	g_key_file_free(key_file);
+}
+
+static void key_file_load_service_allowlist(GKeyFile *key_file,
+					struct btd_admin_policy *admin_policy)
+{
+	GError *gerr = NULL;
+	struct queue *uuid_list = NULL;
+	gchar **uuids = NULL;
+	gsize num, i;
+
+	uuids = g_key_file_get_string_list(key_file, "General",
+					"ServiceAllowlist", &num, &gerr);
+
+	if (gerr) {
+		btd_error(admin_policy->adapter_id,
+					"Failed to load ServiceAllowlist");
+		g_error_free(gerr);
+		return;
+	}
+
+	uuid_list = queue_new();
+	for (i = 0; i < num; i++) {
+		bt_uuid_t *uuid = g_try_malloc(sizeof(*uuid));
+
+		if (!uuid)
+			goto failed;
+
+		if (bt_string_to_uuid(uuid, *uuids)) {
+
+			btd_error(admin_policy->adapter_id,
+					"Failed to convert '%s' to uuid struct",
+					*uuids);
+
+			g_free(uuid);
+			goto failed;
+		}
+
+		queue_push_tail(uuid_list, uuid);
+		uuids++;
+	}
+
+	if (!service_allowlist_set(admin_policy, uuid_list))
+		goto failed;
+
+	return;
+failed:
+	free_service_allowlist(uuid_list);
+}
+
+static void load_policy_settings(struct btd_admin_policy *admin_policy)
+{
+	GKeyFile *key_file;
+	char *filename = ADMIN_POLICY_STORAGE;
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		btd_error(admin_policy->adapter_id,
+				"Failed to get file %s information",
+				filename);
+		return;
+	}
+
+	key_file = g_key_file_new();
+
+	g_key_file_load_from_file(key_file, filename, 0, NULL);
+
+	key_file_load_service_allowlist(key_file, admin_policy);
+
+	g_key_file_free(key_file);
+}
+
 static int admin_policy_adapter_probe(struct btd_adapter *adapter)
 {
 	if (policy_data) {
@@ -247,6 +408,8 @@ static int admin_policy_adapter_probe(struct btd_adapter *adapter)
 	if (!policy_data)
 		return -ENOMEM;
 
+	load_policy_settings(policy_data);
+
 	if (!g_dbus_register_interface(dbus_conn, adapter_get_path(adapter),
 					ADMIN_POLICY_SET_INTERFACE,
 					admin_policy_adapter_methods, NULL,
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [Bluez PATCH v1 14/14] core: fix a possible crash when removing devices
  2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
                   ` (12 preceding siblings ...)
  2021-07-08  6:23 ` [Bluez PATCH v1 13/14] plugins/admin_policy: persist policy settings Howard Chung
@ 2021-07-08  6:23 ` Howard Chung
  13 siblings, 0 replies; 26+ messages in thread
From: Howard Chung @ 2021-07-08  6:23 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung

From: Yun-Hao Chung <howardchung@chromium.org>

This patch changes the logic of probe_service so that the same service
will not be added to a device.
---
The crash can be reproduced in the following steps

1. set service allowlist to ['aaaa']
2. pair with any device
3. after the device is disconnected, set service allowlist to an empty
   list
4. remove the device from adapter

In step 3, when allowlist is set to empty, profile that was blocked
will be added to each devices. However, in step 2, profiles the device
provides had already been added. Due the logic of
device.c:probe_service, there will be 2 identical services in
device->services, which causes a double-free error when removing the
device.

 src/device.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/device.c b/src/device.c
index 0d7444706336..dba26f787066 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4709,8 +4709,11 @@ static struct btd_service *probe_service(struct btd_device *device,
 		return NULL;
 
 	l = find_service_with_profile(device->services, profile);
+	/* If the service already exists, return NULL so that it won't be added
+	 * to the device->services.
+	 */
 	if (l)
-		return l->data;
+		return NULL;
 
 	service = service_create(device, profile);
 
-- 
2.32.0.93.g670b81a890-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [Bluez,v1,01/14] lib: add hash functions for bt_uuid_t
  2021-07-08  6:23 ` [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t Howard Chung
@ 2021-07-08  6:36   ` bluez.test.bot
  2021-07-09  5:21   ` [Bluez PATCH v1 01/14] " Luiz Augusto von Dentz
  1 sibling, 0 replies; 26+ messages in thread
From: bluez.test.bot @ 2021-07-08  6:36 UTC (permalink / raw)
  To: linux-bluetooth, howardchung

[-- Attachment #1: Type: text/plain, Size: 6069 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=512335

---Test result---

Test Summary:
CheckPatch                    FAIL      3.75 seconds
GitLint                       PASS      1.46 seconds
Prep - Setup ELL              PASS      40.49 seconds
Build - Prep                  PASS      0.10 seconds
Build - Configure             PASS      7.11 seconds
Build - Make                  FAIL      65.27 seconds
Make Check                    FAIL      0.32 seconds
Make Distcheck                FAIL      84.18 seconds
Build w/ext ELL - Configure   PASS      7.17 seconds
Build w/ext ELL - Make        FAIL      54.38 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
plugins/admin_policy: add ServiceAllowList property
ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#62: FILE: plugins/admin_policy.c:185:
+					const GDBusPropertyTable *property,
 					                         ^

- total: 1 errors, 0 warnings, 81 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] plugins/admin_policy: add ServiceAllowList property" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

plugins/admin_policy: add AffectedByPolicy property
ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#65: FILE: plugins/admin_policy.c:294:
+					const GDBusPropertyTable *property,
 					                         ^

- total: 1 errors, 0 warnings, 133 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] plugins/admin_policy: add AffectedByPolicy property" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

plugins/admin_policy: persist policy settings
WARNING:LINE_SPACING: Missing a blank line after declarations
#153: FILE: plugins/admin_policy.c:334:
+	struct queue *uuid_list = NULL;
+	gchar **uuids = NULL;

- total: 0 errors, 1 warnings, 207 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] plugins/admin_policy: persist policy settings" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
/usr/bin/ld: lib/.libs/libbluetooth-internal.a(uuid.o): in function `bt_uuid_hash':
/github/workspace/src/lib/uuid.c:136: undefined reference to `g_int64_hash'
/usr/bin/ld: /github/workspace/src/lib/uuid.c:136: undefined reference to `g_int64_hash'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:5351: peripheral/btsensor] Error 1
make: *** [Makefile:4140: all] Error 2


##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
/usr/bin/ld: lib/.libs/libbluetooth-internal.a(uuid.o): in function `bt_uuid_hash':
/github/workspace/src/lib/uuid.c:136: undefined reference to `g_int64_hash'
/usr/bin/ld: /github/workspace/src/lib/uuid.c:136: undefined reference to `g_int64_hash'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:5351: peripheral/btsensor] Error 1
make: *** [Makefile:10429: check] Error 2


##############################
Test: Make Distcheck - FAIL
Desc: Run distcheck to check the distribution
Output:
/usr/bin/ld: lib/.libs/libbluetooth-internal.a(uuid.o): in function `bt_uuid_hash':
/github/workspace/src/bluez-5.60/_build/sub/../../lib/uuid.c:136: undefined reference to `g_int64_hash'
/usr/bin/ld: /github/workspace/src/bluez-5.60/_build/sub/../../lib/uuid.c:136: undefined reference to `g_int64_hash'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:5692: tools/btgatt-client] Error 1
make[1]: *** [Makefile:4140: all] Error 2
make: *** [Makefile:10350: distcheck] Error 1


##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
/usr/bin/ld: lib/.libs/libbluetooth-internal.a(uuid.o): in function `bt_uuid_hash':
/github/workspace/src2/lib/uuid.c:136: undefined reference to `g_int64_hash'
/usr/bin/ld: /github/workspace/src2/lib/uuid.c:136: undefined reference to `g_int64_hash'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:5351: peripheral/btsensor] Error 1
make: *** [Makefile:4140: all] Error 2




---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t
  2021-07-08  6:23 ` [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t Howard Chung
  2021-07-08  6:36   ` [Bluez,v1,01/14] " bluez.test.bot
@ 2021-07-09  5:21   ` Luiz Augusto von Dentz
  2021-07-12  3:20     ` Yun-hao Chung
  1 sibling, 1 reply; 26+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-09  5:21 UTC (permalink / raw)
  To: Howard Chung; +Cc: linux-bluetooth, Miao-chen Chou

Hi Howard,

On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
>
> This adds function GHashFunc and GEqualFunc for bt_uuid_t.
> With these functions, we can add uuids into a GHashTable with bt_uuid_t
> format.

We will likely move away from GLib dependency so I wouldn't want to
introduce a dependency to it specially as part of libbluetooth.

> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
>
>  lib/uuid.c | 27 +++++++++++++++++++++++++++
>  lib/uuid.h |  3 +++
>  2 files changed, 30 insertions(+)
>
> diff --git a/lib/uuid.c b/lib/uuid.c
> index 3d97dc8359c7..a09f5c428b87 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -16,6 +16,7 @@
>  #include <string.h>
>  #include <stdlib.h>
>  #include <errno.h>
> +#include <glib.h>
>
>  #include "lib/bluetooth.h"
>  #include "uuid.h"
> @@ -120,6 +121,32 @@ int bt_uuid_cmp(const bt_uuid_t *uuid1, const bt_uuid_t *uuid2)
>         return bt_uuid128_cmp(&u1, &u2);
>  }
>
> +guint bt_uuid_hash(gconstpointer key)
> +{
> +       const bt_uuid_t *uuid = key;
> +       bt_uuid_t uuid_128;
> +       uint64_t *val;
> +
> +       if (!uuid)
> +               return 0;
> +
> +       bt_uuid_to_uuid128(uuid, &uuid_128);
> +       val = (uint64_t *)&uuid_128.value.u128;
> +
> +       return g_int64_hash(val) ^ g_int64_hash(val+1);
> +}
> +
> +gboolean bt_uuid_equal(gconstpointer v1, gconstpointer v2)
> +{
> +       const bt_uuid_t *uuid1 = v1;
> +       const bt_uuid_t *uuid2 = v2;
> +
> +       if (!uuid1 || !uuid2)
> +               return !uuid1 && !uuid2;
> +
> +       return bt_uuid_cmp(uuid1, uuid2) == 0;
> +}
> +
>  /*
>   * convert the UUID to string, copying a maximum of n characters.
>   */
> diff --git a/lib/uuid.h b/lib/uuid.h
> index 1a4029b68730..e47ccccb9fd2 100644
> --- a/lib/uuid.h
> +++ b/lib/uuid.h
> @@ -17,6 +17,7 @@ extern "C" {
>  #endif
>
>  #include <stdint.h>
> +#include <glib.h>
>
>  #define GENERIC_AUDIO_UUID     "00001203-0000-1000-8000-00805f9b34fb"
>
> @@ -167,6 +168,8 @@ int bt_uuid128_create(bt_uuid_t *btuuid, uint128_t value);
>
>  int bt_uuid_cmp(const bt_uuid_t *uuid1, const bt_uuid_t *uuid2);
>  void bt_uuid_to_uuid128(const bt_uuid_t *src, bt_uuid_t *dst);
> +guint bt_uuid_hash(gconstpointer key);
> +gboolean bt_uuid_equal(gconstpointer v1, gconstpointer v2);
>
>  #define MAX_LEN_UUID_STR 37
>
> --
> 2.32.0.93.g670b81a890-goog
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 05/14] core: add device state and state callbacks
  2021-07-08  6:23 ` [Bluez PATCH v1 05/14] core: add device state and state callbacks Howard Chung
@ 2021-07-09  5:34   ` Luiz Augusto von Dentz
  2021-07-12  3:56     ` Yun-hao Chung
  0 siblings, 1 reply; 26+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-09  5:34 UTC (permalink / raw)
  To: Howard Chung; +Cc: linux-bluetooth, Yun-Hao Chung, Miao-chen Chou

Hi Howard,

On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
>
> From: Yun-Hao Chung <howardchung@chromium.org>
>
> This implements functions to register/unregister state changed callback
> functions, the functions will be called when a device's state changed.
> Currently the state only shows initializing, available and removing,
> which is enough for plugins to register dbus objects upon device
> creation and unregister it upon device removal.
>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
>
>  src/device.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/device.h | 13 +++++++++++
>  2 files changed, 77 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index e1d82eab0988..0d7444706336 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -81,6 +81,13 @@
>
>  static DBusConnection *dbus_conn = NULL;
>  static unsigned service_state_cb_id;
> +static GSList *device_state_callbacks;

Use a struct queue instead of GSList.

> +
> +struct device_state_callback {
> +       btd_device_state_cb     cb;
> +       void                    *user_data;
> +       unsigned int            id;
> +};
>
>  struct btd_disconnect_data {
>         guint id;
> @@ -272,6 +279,8 @@ struct btd_device {
>
>         GIOChannel      *att_io;
>         guint           store_id;
> +
> +       enum btd_device_state_t state;
>  };
>
>  static const uint16_t uuid_list[] = {
> @@ -4095,6 +4104,23 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
>         gatt_services_changed(device);
>  }
>
> +static void device_change_state(struct btd_device *device,
> +                                       enum btd_device_state_t new_state)
> +{
> +       GSList *l;
> +       struct device_state_callback *cb_data;
> +
> +       if (device->state == new_state)
> +               return;
> +
> +       for (l = device_state_callbacks; l != NULL; l = g_slist_next(l)) {
> +               cb_data = l->data;
> +               cb_data->cb(device, new_state, cb_data->user_data);
> +       }
> +
> +       device->state = new_state;
> +}
> +
>  static struct btd_device *device_new(struct btd_adapter *adapter,
>                                 const char *address)
>  {
> @@ -4158,6 +4184,8 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
>
>         device->refresh_discovery = btd_opts.refresh_discovery;
>
> +       device_change_state(device, BTD_DEVICE_STATE_AVAILABLE);
> +
>         return btd_device_ref(device);
>  }
>
> @@ -6839,6 +6867,7 @@ void btd_device_unref(struct btd_device *device)
>
>         DBG("Freeing device %s", device->path);
>
> +       device_change_state(device, BTD_DEVICE_STATE_REMOVING);
>         g_dbus_unregister_interface(dbus_conn, device->path, DEVICE_INTERFACE);
>  }
>
> @@ -6980,3 +7009,38 @@ void btd_device_cleanup(void)
>  {
>         btd_service_remove_state_cb(service_state_cb_id);
>  }
> +
> +unsigned int btd_device_add_state_cb(btd_device_state_cb cb, void *user_data)
> +{
> +       struct device_state_callback *cb_data;
> +       static unsigned int id;
> +
> +       cb_data = g_new0(struct device_state_callback, 1);
> +       cb_data->cb = cb;
> +       cb_data->user_data = user_data;
> +       cb_data->id = ++id;
> +
> +       device_state_callbacks = g_slist_append(device_state_callbacks,
> +                                                               cb_data);
> +
> +       return cb_data->id;
> +}
> +
> +bool btd_device_remove_state_cb(unsigned int id)
> +{
> +       GSList *l;
> +
> +       for (l = device_state_callbacks; l != NULL; l = g_slist_next(l)) {
> +               struct device_state_callback *cb_data = l->data;
> +
> +               if (cb_data && cb_data->id == id) {
> +                       device_state_callbacks = g_slist_remove(
> +                                                       device_state_callbacks,
> +                                                       cb_data);
> +                       g_free(cb_data);
> +                       return true;
> +               }
> +       }
> +
> +       return false;
> +}
> diff --git a/src/device.h b/src/device.h
> index 5f615cb4b6b2..a8424aa4f098 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -11,8 +11,18 @@
>
>  #define DEVICE_INTERFACE       "org.bluez.Device1"
>
> +enum btd_device_state_t {
> +       BTD_DEVICE_STATE_INITIALIZING,  /* Device object is creating */
> +       BTD_DEVICE_STATE_AVAILABLE,     /* Device object is registered */
> +       BTD_DEVICE_STATE_REMOVING,      /* Device object is being removed */
> +};

I got a bad feeling about adding this sort of state, are you trying to
cover some use case that we can't do with btd_service_add_state_cb? It
does seem a lot like it but at device level.

> +
>  struct btd_device;
>
> +typedef void (*btd_device_state_cb) (struct btd_device *device,
> +                                       enum btd_device_state_t new_state,
> +                                       void *user_data);
> +
>  struct btd_device *device_create(struct btd_adapter *adapter,
>                                 const bdaddr_t *address, uint8_t bdaddr_type);
>  struct btd_device *device_create_from_storage(struct btd_adapter *adapter,
> @@ -179,3 +189,6 @@ bool btd_device_all_services_allowed(struct btd_device *dev);
>  void btd_device_update_allowed_services(struct btd_device *dev);
>  void btd_device_init(void);
>  void btd_device_cleanup(void);
> +
> +unsigned int btd_device_add_state_cb(btd_device_state_cb cb, void *user_data);
> +bool btd_device_remove_state_cb(unsigned int id);
> --
> 2.32.0.93.g670b81a890-goog
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed
  2021-07-08  6:23 ` [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed Howard Chung
@ 2021-07-09  5:49   ` Luiz Augusto von Dentz
  2021-07-12  8:16     ` Yun-hao Chung
  0 siblings, 1 reply; 26+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-09  5:49 UTC (permalink / raw)
  To: Howard Chung; +Cc: linux-bluetooth, Miao-chen Chou

Hi Howard,

On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
>
> When A2DP source profile is removed from adapter, a2dp_server and
> everything inside the object will be removed, which also releases all
> MediaEndpoints and MediaPlayer. When A2DP source profile is re-added,
> although a2dp_server will be created, clients are not able to know they
> can register their endpoints and player by then.
>
> This patch handles this case by unregistering Media1 interface
> when we remove a2dp_server, and register it back when a2dp_source is
> allowed.

This sounds more like a bug fix for a regression introduced by the
very set, so Id recommend fixup the original patch that introduced the
problem, also Im afraid there could other instances like this perhaps
it would have been better to propagate the allow/block to the profiles
that way they don't have to be reprobed, I also have my doubts clients
would react properly to Media1 disappearing and appearing again so Id
leave it up if there is any endpoint/player registered to avoid having
them to re-register.

>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> perform following steps
> 1. SetServiceAllowList to empty list
> 2. pair with an LE headset, then turn off the headset
> 3. SetServiceAllowList to "0x1234"
> 4. SetServiceAllowList to empty list
> 5. turn on the headset and check if it is reconnected.
>
>  profiles/audio/a2dp.c  | 2 ++
>  profiles/audio/avrcp.c | 3 +++
>  2 files changed, 5 insertions(+)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index d31ed845cbe7..26d4f365207e 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -3275,6 +3275,7 @@ static int a2dp_source_server_probe(struct btd_profile *p,
>  {
>         struct a2dp_server *server;
>
> +       media_register(adapter);
>         DBG("path %s", adapter_get_path(adapter));
>
>         server = find_server(servers, adapter);
> @@ -3315,6 +3316,7 @@ static void a2dp_source_server_remove(struct btd_profile *p,
>                 return;
>
>         a2dp_server_unregister(server);
> +       media_unregister(adapter);
>  }
>
>  static int a2dp_sink_server_probe(struct btd_profile *p,
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index ccf34b2207a9..997a5be9a0f4 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -4751,6 +4751,8 @@ static void avrcp_controller_server_remove(struct btd_profile *p,
>
>         if (server->tg_record_id == 0)
>                 avrcp_server_unregister(server);
> +
> +       media_unregister(adapter);
>  }
>
>  static int avrcp_controller_server_probe(struct btd_profile *p,
> @@ -4761,6 +4763,7 @@ static int avrcp_controller_server_probe(struct btd_profile *p,
>
>         DBG("path %s", adapter_get_path(adapter));
>
> +       media_register(adapter);
>         server = find_server(servers, adapter);
>         if (server != NULL)
>                 goto done;
> --
> 2.32.0.93.g670b81a890-goog
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method
  2021-07-08  6:23 ` [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method Howard Chung
@ 2021-07-09  6:01   ` Luiz Augusto von Dentz
  2021-07-12  9:09     ` Yun-hao Chung
  0 siblings, 1 reply; 26+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-09  6:01 UTC (permalink / raw)
  To: Howard Chung; +Cc: linux-bluetooth, Yun-Hao Chung, Miao-chen Chou

Hi Howard,

On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
>
> From: Yun-Hao Chung <howardchung@chromium.org>
>
> 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 <mcchou@chromium.org>
> ---
> 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t
  2021-07-09  5:21   ` [Bluez PATCH v1 01/14] " Luiz Augusto von Dentz
@ 2021-07-12  3:20     ` Yun-hao Chung
  0 siblings, 0 replies; 26+ messages in thread
From: Yun-hao Chung @ 2021-07-12  3:20 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Miao-chen Chou

Got it. Will move bt_uuid_hash and bt_uuid_equal to src/adapter.c

On Fri, Jul 9, 2021 at 1:21 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
> >
> > This adds function GHashFunc and GEqualFunc for bt_uuid_t.
> > With these functions, we can add uuids into a GHashTable with bt_uuid_t
> > format.
>
> We will likely move away from GLib dependency so I wouldn't want to
> introduce a dependency to it specially as part of libbluetooth.
>
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > ---
> >
> >  lib/uuid.c | 27 +++++++++++++++++++++++++++
> >  lib/uuid.h |  3 +++
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/lib/uuid.c b/lib/uuid.c
> > index 3d97dc8359c7..a09f5c428b87 100644
> > --- a/lib/uuid.c
> > +++ b/lib/uuid.c
> > @@ -16,6 +16,7 @@
> >  #include <string.h>
> >  #include <stdlib.h>
> >  #include <errno.h>
> > +#include <glib.h>
> >
> >  #include "lib/bluetooth.h"
> >  #include "uuid.h"
> > @@ -120,6 +121,32 @@ int bt_uuid_cmp(const bt_uuid_t *uuid1, const bt_uuid_t *uuid2)
> >         return bt_uuid128_cmp(&u1, &u2);
> >  }
> >
> > +guint bt_uuid_hash(gconstpointer key)
> > +{
> > +       const bt_uuid_t *uuid = key;
> > +       bt_uuid_t uuid_128;
> > +       uint64_t *val;
> > +
> > +       if (!uuid)
> > +               return 0;
> > +
> > +       bt_uuid_to_uuid128(uuid, &uuid_128);
> > +       val = (uint64_t *)&uuid_128.value.u128;
> > +
> > +       return g_int64_hash(val) ^ g_int64_hash(val+1);
> > +}
> > +
> > +gboolean bt_uuid_equal(gconstpointer v1, gconstpointer v2)
> > +{
> > +       const bt_uuid_t *uuid1 = v1;
> > +       const bt_uuid_t *uuid2 = v2;
> > +
> > +       if (!uuid1 || !uuid2)
> > +               return !uuid1 && !uuid2;
> > +
> > +       return bt_uuid_cmp(uuid1, uuid2) == 0;
> > +}
> > +
> >  /*
> >   * convert the UUID to string, copying a maximum of n characters.
> >   */
> > diff --git a/lib/uuid.h b/lib/uuid.h
> > index 1a4029b68730..e47ccccb9fd2 100644
> > --- a/lib/uuid.h
> > +++ b/lib/uuid.h
> > @@ -17,6 +17,7 @@ extern "C" {
> >  #endif
> >
> >  #include <stdint.h>
> > +#include <glib.h>
> >
> >  #define GENERIC_AUDIO_UUID     "00001203-0000-1000-8000-00805f9b34fb"
> >
> > @@ -167,6 +168,8 @@ int bt_uuid128_create(bt_uuid_t *btuuid, uint128_t value);
> >
> >  int bt_uuid_cmp(const bt_uuid_t *uuid1, const bt_uuid_t *uuid2);
> >  void bt_uuid_to_uuid128(const bt_uuid_t *src, bt_uuid_t *dst);
> > +guint bt_uuid_hash(gconstpointer key);
> > +gboolean bt_uuid_equal(gconstpointer v1, gconstpointer v2);
> >
> >  #define MAX_LEN_UUID_STR 37
> >
> > --
> > 2.32.0.93.g670b81a890-goog
> >
>
>
> --
> Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 05/14] core: add device state and state callbacks
  2021-07-09  5:34   ` Luiz Augusto von Dentz
@ 2021-07-12  3:56     ` Yun-hao Chung
  0 siblings, 0 replies; 26+ messages in thread
From: Yun-hao Chung @ 2021-07-12  3:56 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Yun-Hao Chung, Miao-chen Chou

On Fri, Jul 9, 2021 at 1:34 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
> >
> > From: Yun-Hao Chung <howardchung@chromium.org>
> >
> > This implements functions to register/unregister state changed callback
> > functions, the functions will be called when a device's state changed.
> > Currently the state only shows initializing, available and removing,
> > which is enough for plugins to register dbus objects upon device
> > creation and unregister it upon device removal.
> >
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > ---
> >
> >  src/device.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/device.h | 13 +++++++++++
> >  2 files changed, 77 insertions(+)
> >
> > diff --git a/src/device.c b/src/device.c
> > index e1d82eab0988..0d7444706336 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -81,6 +81,13 @@
> >
> >  static DBusConnection *dbus_conn = NULL;
> >  static unsigned service_state_cb_id;
> > +static GSList *device_state_callbacks;
>
> Use a struct queue instead of GSList.
Will do.
>
> > +
> > +struct device_state_callback {
> > +       btd_device_state_cb     cb;
> > +       void                    *user_data;
> > +       unsigned int            id;
> > +};
> >
> >  struct btd_disconnect_data {
> >         guint id;
> > @@ -272,6 +279,8 @@ struct btd_device {
> >
> >         GIOChannel      *att_io;
> >         guint           store_id;
> > +
> > +       enum btd_device_state_t state;
> >  };
> >
> >  static const uint16_t uuid_list[] = {
> > @@ -4095,6 +4104,23 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
> >         gatt_services_changed(device);
> >  }
> >
> > +static void device_change_state(struct btd_device *device,
> > +                                       enum btd_device_state_t new_state)
> > +{
> > +       GSList *l;
> > +       struct device_state_callback *cb_data;
> > +
> > +       if (device->state == new_state)
> > +               return;
> > +
> > +       for (l = device_state_callbacks; l != NULL; l = g_slist_next(l)) {
> > +               cb_data = l->data;
> > +               cb_data->cb(device, new_state, cb_data->user_data);
> > +       }
> > +
> > +       device->state = new_state;
> > +}
> > +
> >  static struct btd_device *device_new(struct btd_adapter *adapter,
> >                                 const char *address)
> >  {
> > @@ -4158,6 +4184,8 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
> >
> >         device->refresh_discovery = btd_opts.refresh_discovery;
> >
> > +       device_change_state(device, BTD_DEVICE_STATE_AVAILABLE);
> > +
> >         return btd_device_ref(device);
> >  }
> >
> > @@ -6839,6 +6867,7 @@ void btd_device_unref(struct btd_device *device)
> >
> >         DBG("Freeing device %s", device->path);
> >
> > +       device_change_state(device, BTD_DEVICE_STATE_REMOVING);
> >         g_dbus_unregister_interface(dbus_conn, device->path, DEVICE_INTERFACE);
> >  }
> >
> > @@ -6980,3 +7009,38 @@ void btd_device_cleanup(void)
> >  {
> >         btd_service_remove_state_cb(service_state_cb_id);
> >  }
> > +
> > +unsigned int btd_device_add_state_cb(btd_device_state_cb cb, void *user_data)
> > +{
> > +       struct device_state_callback *cb_data;
> > +       static unsigned int id;
> > +
> > +       cb_data = g_new0(struct device_state_callback, 1);
> > +       cb_data->cb = cb;
> > +       cb_data->user_data = user_data;
> > +       cb_data->id = ++id;
> > +
> > +       device_state_callbacks = g_slist_append(device_state_callbacks,
> > +                                                               cb_data);
> > +
> > +       return cb_data->id;
> > +}
> > +
> > +bool btd_device_remove_state_cb(unsigned int id)
> > +{
> > +       GSList *l;
> > +
> > +       for (l = device_state_callbacks; l != NULL; l = g_slist_next(l)) {
> > +               struct device_state_callback *cb_data = l->data;
> > +
> > +               if (cb_data && cb_data->id == id) {
> > +                       device_state_callbacks = g_slist_remove(
> > +                                                       device_state_callbacks,
> > +                                                       cb_data);
> > +                       g_free(cb_data);
> > +                       return true;
> > +               }
> > +       }
> > +
> > +       return false;
> > +}
> > diff --git a/src/device.h b/src/device.h
> > index 5f615cb4b6b2..a8424aa4f098 100644
> > --- a/src/device.h
> > +++ b/src/device.h
> > @@ -11,8 +11,18 @@
> >
> >  #define DEVICE_INTERFACE       "org.bluez.Device1"
> >
> > +enum btd_device_state_t {
> > +       BTD_DEVICE_STATE_INITIALIZING,  /* Device object is creating */
> > +       BTD_DEVICE_STATE_AVAILABLE,     /* Device object is registered */
> > +       BTD_DEVICE_STATE_REMOVING,      /* Device object is being removed */
> > +};
>
> I got a bad feeling about adding this sort of state, are you trying to
> cover some use case that we can't do with btd_service_add_state_cb? It
> does seem a lot like it but at device level.
I added this so that the admin plugin can know whenever a device
object is created or removed.
I just learned that we can create a dbus client to listen for new
device objects. If it sounds better, I'll do it in this way.
>
> > +
> >  struct btd_device;
> >
> > +typedef void (*btd_device_state_cb) (struct btd_device *device,
> > +                                       enum btd_device_state_t new_state,
> > +                                       void *user_data);
> > +
> >  struct btd_device *device_create(struct btd_adapter *adapter,
> >                                 const bdaddr_t *address, uint8_t bdaddr_type);
> >  struct btd_device *device_create_from_storage(struct btd_adapter *adapter,
> > @@ -179,3 +189,6 @@ bool btd_device_all_services_allowed(struct btd_device *dev);
> >  void btd_device_update_allowed_services(struct btd_device *dev);
> >  void btd_device_init(void);
> >  void btd_device_cleanup(void);
> > +
> > +unsigned int btd_device_add_state_cb(btd_device_state_cb cb, void *user_data);
> > +bool btd_device_remove_state_cb(unsigned int id);
> > --
> > 2.32.0.93.g670b81a890-goog
> >
>
>
> --
> Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed
  2021-07-09  5:49   ` Luiz Augusto von Dentz
@ 2021-07-12  8:16     ` Yun-hao Chung
  2021-07-12 16:37       ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 26+ messages in thread
From: Yun-hao Chung @ 2021-07-12  8:16 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Miao-chen Chou

I agree this is a trick for CrOS and probably not suitable for
upstreaming. If we want to allow/disallow profiles without
reprobing/removing them, here is what we need to do:
For each profile in profiles/, reject the connection if its UUID is
not allowed. Note that checking the UUID in btd_request_authorization
is not enough since some profiles like profiles/health/mcap.c don't
call btd_request_authorization.
The same check will need to be added in src/profiles.c as well so that
we can also manage external profiles.
Does that make sense?

On Fri, Jul 9, 2021 at 1:49 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
> >
> > When A2DP source profile is removed from adapter, a2dp_server and
> > everything inside the object will be removed, which also releases all
> > MediaEndpoints and MediaPlayer. When A2DP source profile is re-added,
> > although a2dp_server will be created, clients are not able to know they
> > can register their endpoints and player by then.
> >
> > This patch handles this case by unregistering Media1 interface
> > when we remove a2dp_server, and register it back when a2dp_source is
> > allowed.
>
> This sounds more like a bug fix for a regression introduced by the
> very set, so Id recommend fixup the original patch that introduced the
> problem, also Im afraid there could other instances like this perhaps
> it would have been better to propagate the allow/block to the profiles
> that way they don't have to be reprobed, I also have my doubts clients
> would react properly to Media1 disappearing and appearing again so Id
> leave it up if there is any endpoint/player registered to avoid having
> them to re-register.
>
> >
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > ---
> > perform following steps
> > 1. SetServiceAllowList to empty list
> > 2. pair with an LE headset, then turn off the headset
> > 3. SetServiceAllowList to "0x1234"
> > 4. SetServiceAllowList to empty list
> > 5. turn on the headset and check if it is reconnected.
> >
> >  profiles/audio/a2dp.c  | 2 ++
> >  profiles/audio/avrcp.c | 3 +++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > index d31ed845cbe7..26d4f365207e 100644
> > --- a/profiles/audio/a2dp.c
> > +++ b/profiles/audio/a2dp.c
> > @@ -3275,6 +3275,7 @@ static int a2dp_source_server_probe(struct btd_profile *p,
> >  {
> >         struct a2dp_server *server;
> >
> > +       media_register(adapter);
> >         DBG("path %s", adapter_get_path(adapter));
> >
> >         server = find_server(servers, adapter);
> > @@ -3315,6 +3316,7 @@ static void a2dp_source_server_remove(struct btd_profile *p,
> >                 return;
> >
> >         a2dp_server_unregister(server);
> > +       media_unregister(adapter);
> >  }
> >
> >  static int a2dp_sink_server_probe(struct btd_profile *p,
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index ccf34b2207a9..997a5be9a0f4 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -4751,6 +4751,8 @@ static void avrcp_controller_server_remove(struct btd_profile *p,
> >
> >         if (server->tg_record_id == 0)
> >                 avrcp_server_unregister(server);
> > +
> > +       media_unregister(adapter);
> >  }
> >
> >  static int avrcp_controller_server_probe(struct btd_profile *p,
> > @@ -4761,6 +4763,7 @@ static int avrcp_controller_server_probe(struct btd_profile *p,
> >
> >         DBG("path %s", adapter_get_path(adapter));
> >
> > +       media_register(adapter);
> >         server = find_server(servers, adapter);
> >         if (server != NULL)
> >                 goto done;
> > --
> > 2.32.0.93.g670b81a890-goog
> >
>
>
> --
> Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method
  2021-07-09  6:01   ` Luiz Augusto von Dentz
@ 2021-07-12  9:09     ` Yun-hao Chung
  2021-07-12 16:41       ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 26+ messages in thread
From: Yun-hao Chung @ 2021-07-12  9:09 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Yun-Hao Chung, Miao-chen Chou

On Fri, Jul 9, 2021 at 2:01 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
> >
> > From: Yun-Hao Chung <howardchung@chromium.org>
> >
> > 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 <mcchou@chromium.org>
> > ---
> > 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed
  2021-07-12  8:16     ` Yun-hao Chung
@ 2021-07-12 16:37       ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 26+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-12 16:37 UTC (permalink / raw)
  To: Yun-hao Chung; +Cc: linux-bluetooth, Miao-chen Chou

Hi Howard,

On Mon, Jul 12, 2021 at 1:17 AM Yun-hao Chung <howardchung@google.com> wrote:
>
> I agree this is a trick for CrOS and probably not suitable for
> upstreaming. If we want to allow/disallow profiles without
> reprobing/removing them, here is what we need to do:
> For each profile in profiles/, reject the connection if its UUID is
> not allowed. Note that checking the UUID in btd_request_authorization
> is not enough since some profiles like profiles/health/mcap.c don't
> call btd_request_authorization.
> The same check will need to be added in src/profiles.c as well so that
> we can also manage external profiles.
> Does that make sense?

Yep, afaik when Ive implemented the service plugin I did the
blocking/allowing at the service level, not sure if you want to
replicate that.

> On Fri, Jul 9, 2021 at 1:49 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Howard,
> >
> > On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
> > >
> > > When A2DP source profile is removed from adapter, a2dp_server and
> > > everything inside the object will be removed, which also releases all
> > > MediaEndpoints and MediaPlayer. When A2DP source profile is re-added,
> > > although a2dp_server will be created, clients are not able to know they
> > > can register their endpoints and player by then.
> > >
> > > This patch handles this case by unregistering Media1 interface
> > > when we remove a2dp_server, and register it back when a2dp_source is
> > > allowed.
> >
> > This sounds more like a bug fix for a regression introduced by the
> > very set, so Id recommend fixup the original patch that introduced the
> > problem, also Im afraid there could other instances like this perhaps
> > it would have been better to propagate the allow/block to the profiles
> > that way they don't have to be reprobed, I also have my doubts clients
> > would react properly to Media1 disappearing and appearing again so Id
> > leave it up if there is any endpoint/player registered to avoid having
> > them to re-register.
> >
> > >
> > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > ---
> > > perform following steps
> > > 1. SetServiceAllowList to empty list
> > > 2. pair with an LE headset, then turn off the headset
> > > 3. SetServiceAllowList to "0x1234"
> > > 4. SetServiceAllowList to empty list
> > > 5. turn on the headset and check if it is reconnected.
> > >
> > >  profiles/audio/a2dp.c  | 2 ++
> > >  profiles/audio/avrcp.c | 3 +++
> > >  2 files changed, 5 insertions(+)
> > >
> > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > > index d31ed845cbe7..26d4f365207e 100644
> > > --- a/profiles/audio/a2dp.c
> > > +++ b/profiles/audio/a2dp.c
> > > @@ -3275,6 +3275,7 @@ static int a2dp_source_server_probe(struct btd_profile *p,
> > >  {
> > >         struct a2dp_server *server;
> > >
> > > +       media_register(adapter);
> > >         DBG("path %s", adapter_get_path(adapter));
> > >
> > >         server = find_server(servers, adapter);
> > > @@ -3315,6 +3316,7 @@ static void a2dp_source_server_remove(struct btd_profile *p,
> > >                 return;
> > >
> > >         a2dp_server_unregister(server);
> > > +       media_unregister(adapter);
> > >  }
> > >
> > >  static int a2dp_sink_server_probe(struct btd_profile *p,
> > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > index ccf34b2207a9..997a5be9a0f4 100644
> > > --- a/profiles/audio/avrcp.c
> > > +++ b/profiles/audio/avrcp.c
> > > @@ -4751,6 +4751,8 @@ static void avrcp_controller_server_remove(struct btd_profile *p,
> > >
> > >         if (server->tg_record_id == 0)
> > >                 avrcp_server_unregister(server);
> > > +
> > > +       media_unregister(adapter);
> > >  }
> > >
> > >  static int avrcp_controller_server_probe(struct btd_profile *p,
> > > @@ -4761,6 +4763,7 @@ static int avrcp_controller_server_probe(struct btd_profile *p,
> > >
> > >         DBG("path %s", adapter_get_path(adapter));
> > >
> > > +       media_register(adapter);
> > >         server = find_server(servers, adapter);
> > >         if (server != NULL)
> > >                 goto done;
> > > --
> > > 2.32.0.93.g670b81a890-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method
  2021-07-12  9:09     ` Yun-hao Chung
@ 2021-07-12 16:41       ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 26+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-12 16:41 UTC (permalink / raw)
  To: Yun-hao Chung; +Cc: linux-bluetooth, Yun-Hao Chung, Miao-chen Chou

Hi Howard,

On Mon, Jul 12, 2021 at 2:09 AM Yun-hao Chung <howardchung@google.com> wrote:
>
> On Fri, Jul 9, 2021 at 2:01 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Howard,
> >
> > On Wed, Jul 7, 2021 at 11:23 PM Howard Chung <howardchung@google.com> wrote:
> > >
> > > From: Yun-Hao Chung <howardchung@chromium.org>
> > >
> > > 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 <mcchou@chromium.org>
> > > ---
> > > 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-07-12 16:41 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08  6:23 [Bluez PATCH v1 00/14] Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 01/14] lib: add hash functions for bt_uuid_t Howard Chung
2021-07-08  6:36   ` [Bluez,v1,01/14] " bluez.test.bot
2021-07-09  5:21   ` [Bluez PATCH v1 01/14] " Luiz Augusto von Dentz
2021-07-12  3:20     ` Yun-hao Chung
2021-07-08  6:23 ` [Bluez PATCH v1 02/14] unit: add uuid unit tests Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 03/14] core: add is_allowed property in btd_service Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 04/14] core: add adapter and device allowed_uuid functions Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 05/14] core: add device state and state callbacks Howard Chung
2021-07-09  5:34   ` Luiz Augusto von Dentz
2021-07-12  3:56     ` Yun-hao Chung
2021-07-08  6:23 ` [Bluez PATCH v1 06/14] audio: Remove Media1 interface when a2dp source disallowed Howard Chung
2021-07-09  5:49   ` Luiz Augusto von Dentz
2021-07-12  8:16     ` Yun-hao Chung
2021-07-12 16:37       ` Luiz Augusto von Dentz
2021-07-08  6:23 ` [Bluez PATCH v1 07/14] plugins: add a new plugin for admin_policy Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 08/14] plugins/admin_policy: add admin_policy adapter driver Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 09/14] plugins/admin_policy: add ServiceAllowList method Howard Chung
2021-07-09  6:01   ` Luiz Augusto von Dentz
2021-07-12  9:09     ` Yun-hao Chung
2021-07-12 16:41       ` Luiz Augusto von Dentz
2021-07-08  6:23 ` [Bluez PATCH v1 10/14] plugins/admin_policy: add ServiceAllowList property Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 11/14] plugins/admin_policy: add device state callback Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 12/14] plugins/admin_policy: add AffectedByPolicy property Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 13/14] plugins/admin_policy: persist policy settings Howard Chung
2021-07-08  6:23 ` [Bluez PATCH v1 14/14] core: fix a possible crash when removing devices Howard Chung

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).