| Message ID | 20210708062314.245754-1-howardchung@google.com |
|---|---|
| Headers | show |
| Series | [Bluez,v1,01/14] lib: add hash functions for bt_uuid_t | expand |
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
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
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
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
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
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
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
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