2020-11-11 01:19:50

by Sonny Sasaka

Subject: [PATCH BlueZ v2 1/7] battery: Add the internal Battery API

This patch adds an API for internal BlueZ code to expose battery
information to org.bluez.Battery1 interface. The motivation behind this
is that there is going to be other places than GATT BAS handler that
exposes battery information, for example internal plugins and the
BatteryProvider1 D-Bus API for external clients.

Reviewed-by: Daniel Winkler <[email protected]>

---
Makefile.am | 3 +-
src/battery.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++++++
src/battery.h | 15 ++++
3 files changed, 208 insertions(+), 1 deletion(-)
create mode 100644 src/battery.c
create mode 100644 src/battery.h

diff --git a/Makefile.am b/Makefile.am
index 56279c4ba..b19c1d40f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -294,7 +294,8 @@ src_bluetoothd_SOURCES = $(builtin_sources) \
src/device.h src/device.c \
src/dbus-common.c src/dbus-common.h \
src/eir.h src/eir.c \
- src/adv_monitor.h src/adv_monitor.c
+ src/adv_monitor.h src/adv_monitor.c \
+ src/battery.h src/battery.c
src_bluetoothd_LDADD = lib/libbluetooth-internal.la \
gdbus/libgdbus-internal.la \
src/libshared-glib.la \
diff --git a/src/battery.c b/src/battery.c
new file mode 100644
index 000000000..87a6b91fb
--- /dev/null
+++ b/src/battery.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2020 Google LLC
+ *
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <stdbool.h>
+#include <stdint.h>
+#include <glib.h>
+
+#include "gdbus/gdbus.h"
+#include "lib/bluetooth.h"
+#include "src/shared/queue.h"
+#include "src/shared/util.h"
+#include "battery.h"
+#include "dbus-common.h"
+#include "adapter.h"
+#include "log.h"
+
+#define BATTERY_INTERFACE "org.bluez.Battery1"
+
+#define BATTERY_MAX_PERCENTAGE 100
+
+struct btd_battery {
+ char *path; /* D-Bus object path, owns pointer */
+ uint8_t percentage; /* valid between 0 to 100 inclusively */
+};
+
+static struct queue *batteries = NULL;
+
+static void battery_add(struct btd_battery *battery)
+{
+ if (!batteries)
+ batteries = queue_new();
+
+ queue_push_head(batteries, battery);
+}
+
+static void battery_remove(struct btd_battery *battery)
+{
+ queue_remove(batteries, battery);
+ if (queue_isempty(batteries)) {
+ queue_destroy(batteries, NULL);
+ batteries = NULL;
+ }
+}
+
+static bool match_path(const void *data, const void *user_data)
+{
+ const struct btd_battery *battery = data;
+ const char *path = user_data;
+
+ return g_strcmp0(battery->path, path) == 0;
+}
+
+static struct btd_battery *battery_new(const char *path)
+{
+ struct btd_battery *battery;
+
+ battery = new0(struct btd_battery, 1);
+ battery->path = g_strdup(path);
+ battery->percentage = UINT8_MAX;
+
+ return battery;
+}
+
+static void battery_free(struct btd_battery *battery)
+{
+ if (battery->path)
+ g_free(battery->path);
+
+ free(battery);
+}
+
+static gboolean property_percentage_get(const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct btd_battery *battery = data;
+
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BYTE,
+ &battery->percentage);
+
+ return TRUE;
+}
+
+static gboolean property_percentage_exists(const GDBusPropertyTable *property,
+ void *data)
+{
+ struct btd_battery *battery = data;
+
+ return battery->percentage <= BATTERY_MAX_PERCENTAGE;
+}
+
+static const GDBusPropertyTable battery_properties[] = {
+ { "Percentage", "y", property_percentage_get, NULL,
+ property_percentage_exists },
+ {}
+};
+
+struct btd_battery *btd_battery_register(const char *path)
+{
+ struct btd_battery *battery;
+
+ DBG("path = %s", path);
+
+ if (queue_find(batteries, match_path, path)) {
+ error("error registering battery: path exists");
+ return NULL;
+ }
+
+ if (!g_str_has_prefix(path, "/")) {
+ error("error registering battery: invalid D-Bus object path");
+ return NULL;
+ }
+
+ battery = battery_new(path);
+ battery_add(battery);
+
+ if (!g_dbus_register_interface(btd_get_dbus_connection(), battery->path,
+ BATTERY_INTERFACE, NULL, NULL,
+ battery_properties, battery, NULL)) {
+ error("error registering D-Bus interface for %s",
+ battery->path);
+
+ battery_remove(battery);
+ battery_free(battery);
+
+ return NULL;
+ }
+
+ DBG("registered Battery object: %s", battery->path);
+
+ return battery;
+}
+
+bool btd_battery_unregister(struct btd_battery *battery)
+{
+ DBG("path = %s", battery->path);
+
+ if (!queue_find(batteries, NULL, battery)) {
+ error("error unregistering battery: "
+ "battery %s is not registered",
+ battery->path);
+ return false;
+ }
+
+ if (!g_dbus_unregister_interface(btd_get_dbus_connection(),
+ battery->path, BATTERY_INTERFACE)) {
+ error("error unregistering battery %s from D-Bus interface",
+ battery->path);
+ return false;
+ }
+
+ battery_remove(battery);
+ battery_free(battery);
+
+ return true;
+}
+
+bool btd_battery_update(struct btd_battery *battery, uint8_t percentage)
+{
+ DBG("path = %s", battery->path);
+
+ if (!queue_find(batteries, NULL, battery)) {
+ error("error updating battery: battery is not registered");
+ return false;
+ }
+
+ if (percentage > BATTERY_MAX_PERCENTAGE) {
+ error("error updating battery: percentage is not valid");
+ return false;
+ }
+
+ if (battery->percentage == percentage)
+ return true;
+
+ battery->percentage = percentage;
+ g_dbus_emit_property_changed(btd_get_dbus_connection(), battery->path,
+ BATTERY_INTERFACE, "Percentage");
+
+ return true;
+}
diff --git a/src/battery.h b/src/battery.h
new file mode 100644
index 000000000..9c69b7afa
--- /dev/null
+++ b/src/battery.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2020 Google LLC
+ *
+ *
+ */
+
+struct btd_battery;
+
+struct btd_battery *btd_battery_register(const char *path);
+bool btd_battery_unregister(struct btd_battery *battery);
+bool btd_battery_update(struct btd_battery *battery, uint8_t percentage);
--
2.26.2

2020-11-11 01:19:51

by Sonny Sasaka

Subject: [PATCH BlueZ v2 3/7] battery: Add Source property to Battery API

As Battery API will be generalized for other battery reporting
protocols, the Source property is useful for diagnostics purposes.

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

---
profiles/battery/battery.c | 3 ++-
src/battery.c | 35 +++++++++++++++++++++++++++++++----
src/battery.h | 2 +-
3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index e1a61dd0b..2478816a4 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -126,7 +126,8 @@ static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
return;
}

- batt->battery = btd_battery_register(device_get_path(batt->device));
+ batt->battery = btd_battery_register(device_get_path(batt->device),
+ "GATT Battery Service");

if (!batt->battery) {
batt_reset(batt);
diff --git a/src/battery.c b/src/battery.c
index 87a6b91fb..8613d6e23 100644
--- a/src/battery.c
+++ b/src/battery.c
@@ -31,8 +31,9 @@
#define BATTERY_MAX_PERCENTAGE 100

struct btd_battery {
- char *path; /* D-Bus object path, owns pointer */
+ char *path; /* D-Bus object path */
uint8_t percentage; /* valid between 0 to 100 inclusively */
+ char *source; /* Descriptive source of the battery info */
};

static struct queue *batteries = NULL;
@@ -62,13 +63,15 @@ static bool match_path(const void *data, const void *user_data)
return g_strcmp0(battery->path, path) == 0;
}

-static struct btd_battery *battery_new(const char *path)
+static struct btd_battery *battery_new(const char *path, const char *source)
{
struct btd_battery *battery;

battery = new0(struct btd_battery, 1);
battery->path = g_strdup(path);
battery->percentage = UINT8_MAX;
+ if (source)
+ battery->source = g_strdup(source);

return battery;
}
@@ -78,6 +81,9 @@ static void battery_free(struct btd_battery *battery)
if (battery->path)
g_free(battery->path);

+ if (battery->source)
+ g_free(battery->source);
+
free(battery);
}

@@ -100,13 +106,34 @@ static gboolean property_percentage_exists(const GDBusPropertyTable *property,
return battery->percentage <= BATTERY_MAX_PERCENTAGE;
}

+static gboolean property_source_get(const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct btd_battery *battery = data;
+
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING,
+ &battery->source);
+
+ return TRUE;
+}
+
+static gboolean property_source_exists(const GDBusPropertyTable *property,
+ void *data)
+{
+ struct btd_battery *battery = data;
+
+ return battery->source != NULL;
+}
+
static const GDBusPropertyTable battery_properties[] = {
{ "Percentage", "y", property_percentage_get, NULL,
property_percentage_exists },
+ { "Source", "s", property_source_get, NULL, property_source_exists,
+ G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
{}
};

-struct btd_battery *btd_battery_register(const char *path)
+struct btd_battery *btd_battery_register(const char *path, const char *source)
{
struct btd_battery *battery;

@@ -122,7 +149,7 @@ struct btd_battery *btd_battery_register(const char *path)
return NULL;
}

- battery = battery_new(path);
+ battery = battery_new(path, source);
battery_add(battery);

if (!g_dbus_register_interface(btd_get_dbus_connection(), battery->path,
diff --git a/src/battery.h b/src/battery.h
index 9c69b7afa..ff63454cd 100644
--- a/src/battery.h
+++ b/src/battery.h
@@ -10,6 +10,6 @@

struct btd_battery;

-struct btd_battery *btd_battery_register(const char *path);
+struct btd_battery *btd_battery_register(const char *path, const char *source);
bool btd_battery_unregister(struct btd_battery *battery);
bool btd_battery_update(struct btd_battery *battery, uint8_t percentage);
--
2.26.2

2020-11-11 01:19:53

by Sonny Sasaka

Subject: [PATCH BlueZ v2 2/7] profiles/battery: Refactor to use battery library

This refactors profiles/battery to use the internal battery library that
handles the D-Bus intricacies so that profiles/battery only handles the
GATT BAS concerns.

Reviewed-by: Daniel Winkler <[email protected]>

---
profiles/battery/battery.c | 51 +++++++++++---------------------------
1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 13c80d05c..e1a61dd0b 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -23,14 +23,11 @@

#include <glib.h>

-#include "gdbus/gdbus.h"
-
#include "lib/bluetooth.h"
#include "lib/hci.h"
#include "lib/sdp.h"
#include "lib/uuid.h"

-#include "src/dbus-common.h"
#include "src/shared/util.h"
#include "src/shared/att.h"
#include "src/shared/queue.h"
@@ -42,6 +39,7 @@
#include "src/profile.h"
#include "src/service.h"
#include "src/log.h"
+#include "src/battery.h"
#include "attrib/att.h"

#define BATTERY_INTERFACE "org.bluez.Battery1"
@@ -50,7 +48,7 @@

/* Generic Attribute/Access Service */
struct batt {
- char *path; /* D-Bus path of device */
+ struct btd_battery *battery;
struct btd_device *device;
struct gatt_db *db;
struct bt_gatt_client *client;
@@ -69,6 +67,8 @@ static void batt_free(struct batt *batt)
bt_gatt_client_unref(batt->client);
btd_device_unref(batt->device);
g_free (batt->initial_value);
+ if (batt->battery)
+ btd_battery_unregister(batt->battery);
g_free(batt);
}

@@ -81,11 +81,9 @@ static void batt_reset(struct batt *batt)
batt->client = NULL;
g_free (batt->initial_value);
batt->initial_value = NULL;
- if (batt->path) {
- g_dbus_unregister_interface(btd_get_dbus_connection(),
- batt->path, BATTERY_INTERFACE);
- g_free(batt->path);
- batt->path = NULL;
+ if (batt->battery) {
+ btd_battery_unregister(batt->battery);
+ batt->battery = NULL;
}
}

@@ -98,8 +96,11 @@ static void parse_battery_level(struct batt *batt,
if (batt->percentage != percentage) {
batt->percentage = percentage;
DBG("Battery Level updated: %d%%", percentage);
- g_dbus_emit_property_changed(btd_get_dbus_connection(), batt->path,
- BATTERY_INTERFACE, "Percentage");
+ if (!batt->battery) {
+ warn("Trying to update an unregistered battery");
+ return;
+ }
+ btd_battery_update(batt->battery, batt->percentage);
}
}

@@ -115,22 +116,6 @@ static void batt_io_value_cb(uint16_t value_handle, const uint8_t *value,
}
}

-static gboolean property_get_percentage(
- const GDBusPropertyTable *property,
- DBusMessageIter *iter, void *data)
-{
- struct batt *batt = data;
-
- dbus_message_iter_append_basic(iter, DBUS_TYPE_BYTE, &batt->percentage);
-
- return TRUE;
-}
-
-static const GDBusPropertyTable battery_properties[] = {
- { "Percentage", "y", property_get_percentage },
- { }
-};
-
static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
{
struct batt *batt = user_data;
@@ -141,13 +126,9 @@ static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
return;
}

- if (g_dbus_register_interface(btd_get_dbus_connection(),
- batt->path, BATTERY_INTERFACE,
- NULL, NULL,
- battery_properties, batt,
- NULL) == FALSE) {
- error("Unable to register %s interface for %s",
- BATTERY_INTERFACE, batt->path);
+ batt->battery = btd_battery_register(device_get_path(batt->device));
+
+ if (!batt->battery) {
batt_reset(batt);
return;
}
@@ -321,8 +302,6 @@ static int batt_accept(struct btd_service *service)
return -1;
}

- batt->path = g_strdup (device_get_path(device));
-
btd_service_connecting_complete(service, 0);

return 0;
--
2.26.2

2020-11-11 01:20:55

by Sonny Sasaka

Subject: [PATCH BlueZ v2 5/7] test: Add test app for Battery Provider API

The python test app simulates an application registering to BlueZ as a
Battery Provider providing three fake batteries drained periodically.

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

---
test/example-battery-provider | 230 ++++++++++++++++++++++++++++++++++
1 file changed, 230 insertions(+)
create mode 100755 test/example-battery-provider

diff --git a/test/example-battery-provider b/test/example-battery-provider
new file mode 100755
index 000000000..3dbb08563
--- /dev/null
+++ b/test/example-battery-provider
@@ -0,0 +1,230 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: LGPL-2.1-or-later
+
+import dbus
+import dbus.exceptions
+import dbus.mainloop.glib
+import dbus.service
+
+try:
+ from gi.repository import GObject
+except ImportError:
+ import gobject as GObject
+import sys
+
+mainloop = None
+app = None
+bus = None
+
+BLUEZ_SERVICE_NAME = 'org.bluez'
+DBUS_OM_IFACE = 'org.freedesktop.DBus.ObjectManager'
+DBUS_PROP_IFACE = 'org.freedesktop.DBus.Properties'
+
+BATTERY_PROVIDER_MANAGER_IFACE = 'org.bluez.BatteryProviderManager1'
+BATTERY_PROVIDER_IFACE = 'org.bluez.BatteryProvider1'
+BATTERY_PROVIDER_PATH = '/path/to/provider'
+
+BATTERY_PATH1 = '11_11_11_11_11_11'
+BATTERY_PATH2 = '22_22_22_22_22_22'
+BATTERY_PATH3 = '33_33_33_33_33_33'
+
+class InvalidArgsException(dbus.exceptions.DBusException):
+ _dbus_error_name = 'org.freedesktop.DBus.Error.InvalidArgs'
+
+
+class Application(dbus.service.Object):
+ def __init__(self, bus):
+ self.path = BATTERY_PROVIDER_PATH
+ self.services = []
+ self.batteries = []
+ dbus.service.Object.__init__(self, bus, self.path)
+
+ def get_path(self):
+ return dbus.ObjectPath(self.path)
+
+ def add_battery(self, battery):
+ self.batteries.append(battery)
+ self.InterfacesAdded(battery.get_path(), battery.get_properties())
+ GObject.timeout_add(1000, drain_battery, battery)
+
+ def remove_battery(self, battery):
+ self.batteries.remove(battery)
+ self.InterfacesRemoved(battery.get_path(), [BATTERY_PROVIDER_IFACE])
+
+ @dbus.service.method(DBUS_OM_IFACE, out_signature='a{oa{sa{sv}}}')
+ def GetManagedObjects(self):
+ response = {}
+ print('GetManagedObjects called')
+
+ for battery in self.batteries:
+ response[battery.get_path()] = battery.get_properties()
+
+ return response
+
+ @dbus.service.signal(DBUS_OM_IFACE, signature='oa{sa{sv}}')
+ def InterfacesAdded(self, object_path, interfaces_and_properties):
+ return
+
+ @dbus.service.signal(DBUS_OM_IFACE, signature='oas')
+ def InterfacesRemoved(self, object_path, interfaces):
+ return
+
+
+class Battery(dbus.service.Object):
+ """
+ org.bluez.BatteryProvider1 interface implementation
+ """
+ def __init__(self, bus, dev, percentage, source = None):
+ self.path = BATTERY_PROVIDER_PATH + '/org/bluez/hci0/dev_' + dev
+ self.bus = bus
+ self.percentage = percentage
+ self.source = source
+ dbus.service.Object.__init__(self, bus, self.path)
+
+ def get_battery_properties(self):
+ properties = {}
+ if self.percentage != None:
+ properties['Percentage'] = dbus.Byte(self.percentage)
+ if self.source != None:
+ properties['Source'] = self.source
+ return properties
+
+ def get_properties(self):
+ return { BATTERY_PROVIDER_IFACE: self.get_battery_properties() }
+
+ def get_path(self):
+ return dbus.ObjectPath(self.path)
+
+ def set_percentage(self, percentage):
+ if percentage < 0 or percentage > 100:
+ print('percentage not valid')
+ return
+
+ self.percentage = percentage
+ print('battery %s percentage %d' % (self.path, self.percentage))
+ self.PropertiesChanged(
+ BATTERY_PROVIDER_IFACE, self.get_battery_properties())
+
+ @dbus.service.method(DBUS_PROP_IFACE,
+ in_signature='s',
+ out_signature='a{sv}')
+ def GetAll(self, interface):
+ if interface != BATTERY_PROVIDER_IFACE:
+ raise InvalidArgsException()
+
+ return self.get_properties()[BATTERY_PROVIDER_IFACE]
+
+ @dbus.service.signal(DBUS_PROP_IFACE, signature='sa{sv}')
+ def PropertiesChanged(self, interface, properties):
+ return
+
+
+def add_late_battery():
+ app.add_battery(Battery(bus, BATTERY_PATH3, 70, 'Protocol 2'))
+
+
+def drain_battery(battery):
+ new_percentage = 100
+ if battery.percentage != None:
+ new_percentage = battery.percentage - 5
+ if new_percentage < 0:
+ new_percentage = 0
+
+ battery.set_percentage(new_percentage)
+
+ if new_percentage <= 0:
+ return False
+
+ return True
+
+def register_provider_cb():
+ print('Battery Provider registered')
+
+ # Battery added early right after RegisterBatteryProvider succeeds
+ app.add_battery(Battery(bus, BATTERY_PATH2, None))
+ # Battery added later
+ GObject.timeout_add(5000, add_late_battery)
+
+
+def register_provider_error_cb(error):
+ print('Failed to register Battery Provider: ' + str(error))
+ mainloop.quit()
+
+
+def find_manager(bus):
+ remote_om = dbus.Interface(bus.get_object(BLUEZ_SERVICE_NAME, '/'),
+ DBUS_OM_IFACE)
+ objects = remote_om.GetManagedObjects()
+
+ for o, props in objects.items():
+ if BATTERY_PROVIDER_MANAGER_IFACE in props.keys():
+ return o
+
+ return None
+
+
+def unregister_provider_cb():
+ print('Battery Provider unregistered')
+
+
+def unregister_provider_error_cb(error):
+ print('Failed to unregister Battery Provider: ' + str(error))
+
+
+def unregister_battery_provider(battery_provider_manager):
+ battery_provider_manager.UnregisterBatteryProvider(BATTERY_PROVIDER_PATH,
+ reply_handler=unregister_provider_cb,
+ error_handler=unregister_provider_error_cb)
+
+
+def remove_battery(app, battery):
+ app.remove_battery(battery)
+
+
+"""
+Simulates an application registering to BlueZ as a Battery Provider providing
+fake batteries drained periodically.
+"""
+def main():
+ global mainloop, bus, app
+
+ dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
+
+ bus = dbus.SystemBus()
+
+ manager_path = find_manager(bus)
+ if not manager_path:
+ print('BatteryProviderManager1 interface not found')
+ return
+
+ print('BatteryProviderManager1 path = ', manager_path)
+
+ battery_provider_manager = dbus.Interface(
+ bus.get_object(BLUEZ_SERVICE_NAME, manager_path),
+ BATTERY_PROVIDER_MANAGER_IFACE)
+
+ app = Application(bus)
+
+ # Battery pre-added before RegisterBatteryProvider
+ battery1 = Battery(bus, BATTERY_PATH1, 87, 'Protocol 1')
+ app.add_battery(battery1)
+
+ mainloop = GObject.MainLoop()
+
+ print('Registering Battery Provider...')
+
+ battery_provider_manager.RegisterBatteryProvider(BATTERY_PROVIDER_PATH,
+ reply_handler=register_provider_cb,
+ error_handler=register_provider_error_cb)
+
+ # Unregister the Battery Provider after an arbitrary amount of time
+ GObject.timeout_add(
+ 12000, unregister_battery_provider, battery_provider_manager)
+ # Simulate battery removal by a provider
+ GObject.timeout_add(8000, remove_battery, app, battery1)
+
+ mainloop.run()
+
+
+if __name__ == '__main__':
+ main()
--
2.26.2

2020-11-11 01:22:40

by Sonny Sasaka

Subject: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

This patch implements the BatteryProvider1 and BatteryProviderManager1
API. This is a means for external clients to feed battery information to
BlueZ if they handle some profile and can decode battery reporting.

The battery information is then exposed externally via the existing
Battery1 interface. UI components can consume this API to display
Bluetooth peripherals' battery via a unified BlueZ API.

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

---
profiles/battery/battery.c | 2 +-
src/adapter.c | 11 ++
src/battery.c | 371 ++++++++++++++++++++++++++++++++++++-
src/battery.h | 10 +-
4 files changed, 389 insertions(+), 5 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 2478816a4..81f849d57 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -127,7 +127,7 @@ static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
}

batt->battery = btd_battery_register(device_get_path(batt->device),
- "GATT Battery Service");
+ "GATT Battery Service", NULL);

if (!batt->battery) {
batt_reset(batt);
diff --git a/src/adapter.c b/src/adapter.c
index d27faaaa3..b791cdad2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -66,6 +66,7 @@
#include "advertising.h"
#include "adv_monitor.h"
#include "eir.h"
+#include "battery.h"

#define MODE_OFF 0x00
#define MODE_CONNECTABLE 0x01
@@ -254,6 +255,8 @@ struct btd_adapter {

struct btd_adv_monitor_manager *adv_monitor_manager;

+ struct btd_battery_provider_manager *battery_provider_manager;
+
gboolean initialized;

GSList *pin_callbacks;
@@ -6361,6 +6364,9 @@ static void adapter_remove(struct btd_adapter *adapter)
btd_adv_monitor_manager_destroy(adapter->adv_monitor_manager);
adapter->adv_monitor_manager = NULL;

+ btd_battery_provider_manager_destroy(adapter->battery_provider_manager);
+ adapter->battery_provider_manager = NULL;
+
g_slist_free(adapter->pin_callbacks);
adapter->pin_callbacks = NULL;

@@ -8651,6 +8657,11 @@ static int adapter_register(struct btd_adapter *adapter)
}
}

+ if (g_dbus_get_flags() & G_DBUS_FLAG_ENABLE_EXPERIMENTAL) {
+ adapter->battery_provider_manager =
+ btd_battery_provider_manager_create(adapter);
+ }
+
db = btd_gatt_database_get_db(adapter->database);
adapter->db_id = gatt_db_register(db, services_modified,
services_modified,
diff --git a/src/battery.c b/src/battery.c
index 8613d6e23..8594c8d77 100644
--- a/src/battery.c
+++ b/src/battery.c
@@ -25,8 +25,11 @@
#include "dbus-common.h"
#include "adapter.h"
#include "log.h"
+#include "error.h"

#define BATTERY_INTERFACE "org.bluez.Battery1"
+#define BATTERY_PROVIDER_INTERFACE "org.bluez.BatteryProvider1"
+#define BATTERY_PROVIDER_MANAGER_INTERFACE "org.bluez.BatteryProviderManager1"

#define BATTERY_MAX_PERCENTAGE 100

@@ -34,10 +37,27 @@ struct btd_battery {
char *path; /* D-Bus object path */
uint8_t percentage; /* valid between 0 to 100 inclusively */
char *source; /* Descriptive source of the battery info */
+ char *provider_path; /* The provider root path, if any */
+};
+
+struct btd_battery_provider_manager {
+ struct btd_adapter *adapter; /* Does not own pointer */
+ struct queue *battery_providers;
+};
+
+struct battery_provider {
+ struct btd_battery_provider_manager *manager; /* Does not own pointer */
+
+ char *owner; /* Owner D-Bus address */
+ char *path; /* D-Bus object path */
+
+ GDBusClient *client;
};

static struct queue *batteries = NULL;

+static void provider_disconnect_cb(DBusConnection *conn, void *user_data);
+
static void battery_add(struct btd_battery *battery)
{
if (!batteries)
@@ -63,7 +83,8 @@ static bool match_path(const void *data, const void *user_data)
return g_strcmp0(battery->path, path) == 0;
}

-static struct btd_battery *battery_new(const char *path, const char *source)
+static struct btd_battery *battery_new(const char *path, const char *source,
+ const char *provider_path)
{
struct btd_battery *battery;

@@ -72,6 +93,8 @@ static struct btd_battery *battery_new(const char *path, const char *source)
battery->percentage = UINT8_MAX;
if (source)
battery->source = g_strdup(source);
+ if (provider_path)
+ battery->provider_path = g_strdup(provider_path);

return battery;
}
@@ -133,7 +156,8 @@ static const GDBusPropertyTable battery_properties[] = {
{}
};

-struct btd_battery *btd_battery_register(const char *path, const char *source)
+struct btd_battery *btd_battery_register(const char *path, const char *source,
+ const char *provider_path)
{
struct btd_battery *battery;

@@ -149,7 +173,7 @@ struct btd_battery *btd_battery_register(const char *path, const char *source)
return NULL;
}

- battery = battery_new(path, source);
+ battery = battery_new(path, source, provider_path);
battery_add(battery);

if (!g_dbus_register_interface(btd_get_dbus_connection(), battery->path,
@@ -216,3 +240,344 @@ bool btd_battery_update(struct btd_battery *battery, uint8_t percentage)

return true;
}
+
+static struct btd_battery *find_battery_by_path(const char *path)
+{
+ return queue_find(batteries, match_path, path);
+}
+
+static void provided_battery_property_changed_cb(GDBusProxy *proxy,
+ const char *name,
+ DBusMessageIter *iter,
+ void *user_data)
+{
+ uint8_t percentage;
+ struct battery_provider *provider = user_data;
+ const char *path = g_dbus_proxy_get_path(proxy);
+ const char *export_path;
+
+ export_path = &path[strlen(provider->path)];
+
+ if (strcmp(name, "Percentage") != 0)
+ return;
+
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_BYTE)
+ return;
+
+ dbus_message_iter_get_basic(iter, &percentage);
+
+ DBG("battery percentage changed on %s, percentage = %d",
+ g_dbus_proxy_get_path(proxy), percentage);
+
+ btd_battery_update(find_battery_by_path(export_path), percentage);
+}
+
+static void provided_battery_added_cb(GDBusProxy *proxy, void *user_data)
+{
+ struct battery_provider *provider = user_data;
+ struct btd_battery *battery;
+ const char *path = g_dbus_proxy_get_path(proxy);
+ const char *export_path;
+ const char *source = NULL;
+ uint8_t percentage;
+ DBusMessageIter iter;
+
+ export_path = &path[strlen(provider->path)];
+
+ if (strcmp(g_dbus_proxy_get_interface(proxy),
+ BATTERY_PROVIDER_INTERFACE) != 0)
+ return;
+
+ if (!btd_adapter_find_device_by_path(provider->manager->adapter,
+ export_path)) {
+ warn("Ignoring non-existent device path for battery %s",
+ export_path);
+ return;
+ }
+
+ if (find_battery_by_path(export_path))
+ return;
+
+ g_dbus_proxy_set_property_watch(
+ proxy, provided_battery_property_changed_cb, provider);
+
+ if (g_dbus_proxy_get_property(proxy, "Source", &iter) == TRUE)
+ dbus_message_iter_get_basic(&iter, &source);
+
+ battery = btd_battery_register(export_path, source, provider->path);
+
+ DBG("provided battery added %s", path);
+
+ /* Percentage property may not be immediately available, that's okay
+ * since we monitor changes to this property.
+ */
+ if (g_dbus_proxy_get_property(proxy, "Percentage", &iter) == FALSE)
+ return;
+
+ dbus_message_iter_get_basic(&iter, &percentage);
+
+ btd_battery_update(battery, percentage);
+}
+
+static void provided_battery_removed_cb(GDBusProxy *proxy, void *user_data)
+{
+ struct battery_provider *provider = user_data;
+ struct btd_battery *battery;
+ const char *path = g_dbus_proxy_get_path(proxy);
+ const char *export_path;
+
+ export_path = &path[strlen(provider->path)];
+
+ if (strcmp(g_dbus_proxy_get_interface(proxy),
+ BATTERY_PROVIDER_INTERFACE) != 0)
+ return;
+
+ DBG("provided battery removed %s", g_dbus_proxy_get_path(proxy));
+
+ battery = find_battery_by_path(export_path);
+ if (!battery)
+ return;
+
+ if (g_strcmp0(battery->provider_path, provider->path) != 0)
+ return;
+
+ g_dbus_proxy_set_property_watch(proxy, NULL, NULL);
+
+ btd_battery_unregister(battery);
+}
+
+static bool match_provider_path(const void *data, const void *user_data)
+{
+ const struct battery_provider *provider = data;
+ const char *path = user_data;
+
+ return strcmp(provider->path, path) == 0;
+}
+
+static void unregister_if_path_has_prefix(void *data, void *user_data)
+{
+ struct btd_battery *battery = data;
+ struct battery_provider *provider = user_data;
+
+ if (g_strcmp0(battery->provider_path, provider->path) == 0)
+ btd_battery_unregister(battery);
+}
+
+static void battery_provider_free(gpointer data)
+{
+ struct battery_provider *provider = data;
+
+ /* Unregister batteries under the root path of provider->path */
+ queue_foreach(batteries, unregister_if_path_has_prefix, provider);
+
+ if (provider->owner)
+ g_free(provider->owner);
+
+ if (provider->path)
+ g_free(provider->path);
+
+ if (provider->client) {
+ g_dbus_client_set_disconnect_watch(provider->client, NULL,
+ NULL);
+ g_dbus_client_set_proxy_handlers(provider->client, NULL, NULL,
+ NULL, NULL);
+ g_dbus_client_unref(provider->client);
+ }
+
+ free(provider);
+}
+
+static struct battery_provider *
+battery_provider_new(DBusConnection *conn,
+ struct btd_battery_provider_manager *manager,
+ const char *path, const char *sender)
+{
+ struct battery_provider *provider;
+
+ provider = new0(struct battery_provider, 1);
+ provider->manager = manager;
+ provider->owner = g_strdup(sender);
+ provider->path = g_strdup(path);
+
+ provider->client = g_dbus_client_new_full(conn, sender, path, path);
+
+ if (!provider->client) {
+ error("error creating D-Bus client %s", path);
+ battery_provider_free(provider);
+ return NULL;
+ }
+
+ g_dbus_client_set_disconnect_watch(provider->client,
+ provider_disconnect_cb, provider);
+
+ g_dbus_client_set_proxy_handlers(provider->client,
+ provided_battery_added_cb,
+ provided_battery_removed_cb, NULL,
+ provider);
+
+ return provider;
+}
+
+static void provider_disconnect_cb(DBusConnection *conn, void *user_data)
+{
+ struct battery_provider *provider = user_data;
+ struct btd_battery_provider_manager *manager = provider->manager;
+
+ DBG("battery provider client disconnected %s root path %s",
+ provider->owner, provider->path);
+
+ if (!queue_find(manager->battery_providers, NULL, provider)) {
+ warn("Disconnection on a non-existing provider %s",
+ provider->path);
+ return;
+ }
+
+ queue_remove(manager->battery_providers, provider);
+ battery_provider_free(provider);
+}
+
+static DBusMessage *register_battery_provider(DBusConnection *conn,
+ DBusMessage *msg, void *user_data)
+{
+ struct btd_battery_provider_manager *manager = user_data;
+ const char *sender = dbus_message_get_sender(msg);
+ DBusMessageIter args;
+ const char *path;
+ struct battery_provider *provider;
+
+ if (!dbus_message_iter_init(msg, &args))
+ return btd_error_invalid_args(msg);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
+ return btd_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&args, &path);
+
+ DBG("register battery provider path = %s", path);
+
+ if (!g_str_has_prefix(path, "/"))
+ return btd_error_invalid_args(msg);
+
+ if (queue_find(manager->battery_providers, match_provider_path, path)) {
+ return dbus_message_new_error(msg,
+ ERROR_INTERFACE ".AlreadyExists",
+ "Provider already exists");
+ }
+
+ provider = battery_provider_new(conn, manager, path, sender);
+ queue_push_head(manager->battery_providers, provider);
+
+ return dbus_message_new_method_return(msg);
+}
+
+static DBusMessage *unregister_battery_provider(DBusConnection *conn,
+ DBusMessage *msg,
+ void *user_data)
+{
+ struct btd_battery_provider_manager *manager = user_data;
+ const char *sender = dbus_message_get_sender(msg);
+ DBusMessageIter args;
+ const char *path;
+ struct battery_provider *provider;
+
+ if (!dbus_message_iter_init(msg, &args))
+ return btd_error_invalid_args(msg);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
+ return btd_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&args, &path);
+
+ DBG("unregister battery provider path = %s", path);
+
+ provider = queue_find(manager->battery_providers, match_provider_path,
+ path);
+ if (!provider || strcmp(provider->owner, sender) != 0) {
+ return dbus_message_new_error(msg,
+ ERROR_INTERFACE ".DoesNotExist",
+ "Provider does not exist");
+ }
+
+ queue_remove(manager->battery_providers, provider);
+ battery_provider_free(provider);
+
+ return dbus_message_new_method_return(msg);
+}
+
+static const GDBusMethodTable methods[] = {
+ { GDBUS_EXPERIMENTAL_METHOD("RegisterBatteryProvider",
+ GDBUS_ARGS({ "provider", "o" }), NULL,
+ register_battery_provider) },
+ { GDBUS_EXPERIMENTAL_METHOD("UnregisterBatteryProvider",
+ GDBUS_ARGS({ "provider", "o" }), NULL,
+ unregister_battery_provider) },
+ {}
+};
+
+static struct btd_battery_provider_manager *
+manager_new(struct btd_adapter *adapter)
+{
+ struct btd_battery_provider_manager *manager;
+
+ DBG("");
+
+ manager = new0(struct btd_battery_provider_manager, 1);
+ manager->adapter = adapter;
+ manager->battery_providers = queue_new();
+
+ return manager;
+}
+
+static void manager_free(struct btd_battery_provider_manager *manager)
+{
+ if (!manager)
+ return;
+
+ DBG("");
+
+ queue_destroy(manager->battery_providers, battery_provider_free);
+
+ free(manager);
+}
+
+struct btd_battery_provider_manager *
+btd_battery_provider_manager_create(struct btd_adapter *adapter)
+{
+ struct btd_battery_provider_manager *manager;
+
+ if (!adapter)
+ return NULL;
+
+ manager = manager_new(adapter);
+ if (!manager)
+ return NULL;
+
+ if (!g_dbus_register_interface(btd_get_dbus_connection(),
+ adapter_get_path(manager->adapter),
+ BATTERY_PROVIDER_MANAGER_INTERFACE,
+ methods, NULL, NULL, manager, NULL)) {
+ error("error registering " BATTERY_PROVIDER_MANAGER_INTERFACE
+ " interface");
+ manager_free(manager);
+ return NULL;
+ }
+
+ info("Battery Provider Manager created");
+
+ return manager;
+}
+
+void btd_battery_provider_manager_destroy(
+ struct btd_battery_provider_manager *manager)
+{
+ if (!manager)
+ return;
+
+ g_dbus_unregister_interface(btd_get_dbus_connection(),
+ adapter_get_path(manager->adapter),
+ BATTERY_PROVIDER_MANAGER_INTERFACE);
+
+ info("Battery Provider Manager destroyed");
+
+ manager_free(manager);
+}
diff --git a/src/battery.h b/src/battery.h
index ff63454cd..271659474 100644
--- a/src/battery.h
+++ b/src/battery.h
@@ -8,8 +8,16 @@
*
*/

+struct btd_adapter;
struct btd_battery;
+struct btd_battery_provider_manager;

-struct btd_battery *btd_battery_register(const char *path, const char *source);
+struct btd_battery *btd_battery_register(const char *path, const char *source,
+ const char *provider_path);
bool btd_battery_unregister(struct btd_battery *battery);
bool btd_battery_update(struct btd_battery *battery, uint8_t percentage);
+
+struct btd_battery_provider_manager *
+btd_battery_provider_manager_create(struct btd_adapter *adapter);
+void btd_battery_provider_manager_destroy(
+ struct btd_battery_provider_manager *manager);
--
2.26.2

2020-11-11 01:22:41

by Sonny Sasaka

Subject: [PATCH BlueZ v2 6/7] adapter: Add a public function to find a device by path

The public function is motivated by the Battery Provider API code which
needs to probe whether a device exists.

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

---
src/adapter.c | 33 ++++++++++++++++++++++++---------
src/adapter.h | 2 ++
2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index c0053000a..d27faaaa3 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -872,6 +872,30 @@ struct btd_device *btd_adapter_find_device(struct btd_adapter *adapter,
return device;
}

+static int device_path_cmp(gconstpointer a, gconstpointer b)
+{
+ const struct btd_device *device = a;
+ const char *path = b;
+ const char *dev_path = device_get_path(device);
+
+ return strcasecmp(dev_path, path);
+}
+
+struct btd_device *btd_adapter_find_device_by_path(struct btd_adapter *adapter,
+ const char *path)
+{
+ GSList *list;
+
+ if (!adapter)
+ return NULL;
+
+ list = g_slist_find_custom(adapter->devices, path, device_path_cmp);
+ if (!list)
+ return NULL;
+
+ return list->data;
+}
+
static void uuid_to_uuid128(uuid_t *uuid128, const uuid_t *uuid)
{
if (uuid->type == SDP_UUID16)
@@ -3184,15 +3208,6 @@ static gboolean property_get_roles(const GDBusPropertyTable *property,
return TRUE;
}

-static int device_path_cmp(gconstpointer a, gconstpointer b)
-{
- const struct btd_device *device = a;
- const char *path = b;
- const char *dev_path = device_get_path(device);
-
- return strcasecmp(dev_path, path);
-}
-
static DBusMessage *remove_device(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
diff --git a/src/adapter.h b/src/adapter.h
index dcc574857..a77c7a61c 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -83,6 +83,8 @@ sdp_list_t *btd_adapter_get_services(struct btd_adapter *adapter);
struct btd_device *btd_adapter_find_device(struct btd_adapter *adapter,
const bdaddr_t *dst,
uint8_t dst_type);
+struct btd_device *btd_adapter_find_device_by_path(struct btd_adapter *adapter,
+ const char *path);

const char *adapter_get_path(struct btd_adapter *adapter);
const bdaddr_t *btd_adapter_get_address(struct btd_adapter *adapter);
--
2.26.2

2020-11-11 01:28:57

by bluez.test.bot

Subject: RE: [BlueZ,v2,1/7] battery: Add the internal Battery API

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=381631

---Test result---

##############################
Test: CheckPatch - FAIL
Output:
battery: Add the internal Battery API
ERROR:INITIALISED_STATIC: do not initialise statics to NULL
#71: FILE: src/battery.c:38:
+static struct queue *batteries = NULL;

- total: 1 errors, 0 warnings, 215 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] battery: Add the internal Battery API" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED 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: Add test app for Battery Provider API
ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
#12: FILE: test/example-battery-provider

- total: 1 errors, 0 warnings, 230 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] test: Add test app for Battery Provider API" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED 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: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

2020-11-17 11:00:18

by Bastien Nocera

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hey Sonny,

On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> This patch implements the BatteryProvider1 and
> BatteryProviderManager1
> API. This is a means for external clients to feed battery information
> to
> BlueZ if they handle some profile and can decode battery reporting.
>
> The battery information is then exposed externally via the existing
> Battery1 interface. UI components can consume this API to display
> Bluetooth peripherals' battery via a unified BlueZ API.

Was this patch reviewed for potential security problems? From the top
of my head, the possible problems would be:
- I don't see any filters on which user could register battery
providers, so on a multi user system, you could have a user logged in
via SSH squatting all the battery providers, while the user "at the
console" can't have their own providers. Also, what happens if the user
at the console changes (fast user switching)?
- It looks like battery providers don't check for paired, trusted or
even connected devices, so I would be able to create nearly unbound
number of battery providers depending on how big the cache for "seen"
devices is.

Given that the interface between upower and bluez is supposedly
trusted, it might be good to ensure that there are no fuzzing problems
on the bluez API side that could translate to causing problems in
upower itself.

I didn't review the code in depth, but, having written this mechanism
for Bluetooth battery reporting, I think that this is the right way to
go to allow daemons like pulseaudio to report battery status.

Cheers

2020-11-17 18:03:38

by Bastien Nocera

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> <
> <snip> didn't review the code in depth, but, having written this
> mechanism
> for Bluetooth battery reporting, I think that this is the right way
> to
> go to allow daemons like pulseaudio to report battery status.

It would also be useful to know what external components, or internal
plugins you expect to feed this API.

Apparently HSP/HFP, for example, provide more information that what can
be expressed with the Battery1 API, whether it is charging or not, or
whether a battery level is unknown, etc.

So I would say that, even if the current battery API is extended, we
need to make sure that the D-Bus API to feed new data is extensible
enough to allow new properties, and they don't need to be added
separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.

2020-11-17 22:17:36

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Bastien,

Thank you for the feedback. Please find my answers below.

On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <[email protected]> wrote:
>
> Hey Sonny,
>
> On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > This patch implements the BatteryProvider1 and
> > BatteryProviderManager1
> > API. This is a means for external clients to feed battery information
> > to
> > BlueZ if they handle some profile and can decode battery reporting.
> >
> > The battery information is then exposed externally via the existing
> > Battery1 interface. UI components can consume this API to display
> > Bluetooth peripherals' battery via a unified BlueZ API.
>
> Was this patch reviewed for potential security problems? From the top
> of my head, the possible problems would be:
> - I don't see any filters on which user could register battery
> providers, so on a multi user system, you could have a user logged in
> via SSH squatting all the battery providers, while the user "at the
> console" can't have their own providers. Also, what happens if the user
> at the console changes (fast user switching)?
> - It looks like battery providers don't check for paired, trusted or
> even connected devices, so I would be able to create nearly unbound
> number of battery providers depending on how big the cache for "seen"
> devices is.
For security, the API can be access-limited at D-Bus level using D-Bus
configuration files. For example, we can let only trusted UNIX users
as the callers for this API. This D-Bus config file would be
distribution-specific. In Chrome OS, for example, only the "audio" and
"power" users are allowed to call this API. This way we can make sure
that the callers do not abuse the API for denial-of-service kind of
attack.

>
> Given that the interface between upower and bluez is supposedly
> trusted, it might be good to ensure that there are no fuzzing problems
> on the bluez API side that could translate to causing problems in
> upower itself.
Could you give an example of what potential problems of upower can be
caused by communicating with BlueZ through this API?

>
> I didn't review the code in depth, but, having written this mechanism
> for Bluetooth battery reporting, I think that this is the right way to
> go to allow daemons like pulseaudio to report battery status.
>
> Cheers
>

2020-11-17 22:24:22

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Bastien,

More responses below.

On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <[email protected]> wrote:
>
> On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > <
> > <snip> didn't review the code in depth, but, having written this
> > mechanism
> > for Bluetooth battery reporting, I think that this is the right way
> > to
> > go to allow daemons like pulseaudio to report battery status.
>
> It would also be useful to know what external components, or internal
> plugins you expect to feed this API.
BlueZ could have internal plugins to read proprietary battery
reporting, e.g. JBL speakers and Bose QC35.

>
> Apparently HSP/HFP, for example, provide more information that what can
> be expressed with the Battery1 API, whether it is charging or not, or
> whether a battery level is unknown, etc.
>
> So I would say that, even if the current battery API is extended, we
> need to make sure that the D-Bus API to feed new data is extensible
> enough to allow new properties, and they don't need to be added
> separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
I proposed the API to start simple, but I believe that it can always
be extended as we need in the future so I don't think the additional
features need to be coded now. I documented how this API could evolve
in the future by extending other features as well in this design
document that I shared with Luiz and Marcel:
https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps.

>

2020-11-17 22:27:18

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Bastien,

On Tue, Nov 17, 2020 at 2:22 PM Sonny Sasaka <[email protected]> wrote:
>
> Hi Bastien,
>
> More responses below.
>
> On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <[email protected]> wrote:
> >
> > On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > > <
> > > <snip> didn't review the code in depth, but, having written this
> > > mechanism
> > > for Bluetooth battery reporting, I think that this is the right way
> > > to
> > > go to allow daemons like pulseaudio to report battery status.
> >
> > It would also be useful to know what external components, or internal
> > plugins you expect to feed this API.
> BlueZ could have internal plugins to read proprietary battery
> reporting, e.g. JBL speakers and Bose QC35.
Adding to mention that in Chrome OS we also plan to have a plugin to
decode Pixel Buds 2 multiple battery values (left, right, and case).
We have not yet decided whether this is going to be internal plugin or
external client though. But generally the API should allow this kind
of feature as well.


>
> >
> > Apparently HSP/HFP, for example, provide more information that what can
> > be expressed with the Battery1 API, whether it is charging or not, or
> > whether a battery level is unknown, etc.
> >
> > So I would say that, even if the current battery API is extended, we
> > need to make sure that the D-Bus API to feed new data is extensible
> > enough to allow new properties, and they don't need to be added
> > separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
> I proposed the API to start simple, but I believe that it can always
> be extended as we need in the future so I don't think the additional
> features need to be coded now. I documented how this API could evolve
> in the future by extending other features as well in this design
> document that I shared with Luiz and Marcel:
> https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps.
>
> >

2020-11-17 22:29:05

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Sonny,

On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <[email protected]> wrote:
>
> Hi Bastien,
>
> Thank you for the feedback. Please find my answers below.
>
> On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <[email protected]> wrote:
> >
> > Hey Sonny,
> >
> > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > This patch implements the BatteryProvider1 and
> > > BatteryProviderManager1
> > > API. This is a means for external clients to feed battery information
> > > to
> > > BlueZ if they handle some profile and can decode battery reporting.
> > >
> > > The battery information is then exposed externally via the existing
> > > Battery1 interface. UI components can consume this API to display
> > > Bluetooth peripherals' battery via a unified BlueZ API.
> >
> > Was this patch reviewed for potential security problems? From the top
> > of my head, the possible problems would be:
> > - I don't see any filters on which user could register battery
> > providers, so on a multi user system, you could have a user logged in
> > via SSH squatting all the battery providers, while the user "at the
> > console" can't have their own providers. Also, what happens if the user
> > at the console changes (fast user switching)?
> > - It looks like battery providers don't check for paired, trusted or
> > even connected devices, so I would be able to create nearly unbound
> > number of battery providers depending on how big the cache for "seen"
> > devices is.
> For security, the API can be access-limited at D-Bus level using D-Bus
> configuration files. For example, we can let only trusted UNIX users
> as the callers for this API. This D-Bus config file would be
> distribution-specific. In Chrome OS, for example, only the "audio" and
> "power" users are allowed to call this API. This way we can make sure
> that the callers do not abuse the API for denial-of-service kind of
> attack.

I guess there is still some the risk of conflicts even with the use of
D-Bus policy blocking roge applications, there could still be multiple
entities providing the battery status from the same source, which is
why I suggested we couple the Battery1 with the Profile1, so only the
entity that has registered to handle let say HFP can provide a battery
status and we automatically deduce the source is from HFP.

> >
> > Given that the interface between upower and bluez is supposedly
> > trusted, it might be good to ensure that there are no fuzzing problems
> > on the bluez API side that could translate to causing problems in
> > upower itself.
> Could you give an example of what potential problems of upower can be
> caused by communicating with BlueZ through this API?
>
> >
> > I didn't review the code in depth, but, having written this mechanism
> > for Bluetooth battery reporting, I think that this is the right way to
> > go to allow daemons like pulseaudio to report battery status.
> >
> > Cheers
> >



--
Luiz Augusto von Dentz

2020-11-19 10:45:34

by Bastien Nocera

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> Hi Bastien,
>
> Thank you for the feedback. Please find my answers below.
>
> On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <[email protected]>
> wrote:
> >
> > Hey Sonny,
> >
> > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > This patch implements the BatteryProvider1 and
> > > BatteryProviderManager1
> > > API. This is a means for external clients to feed battery
> > > information
> > > to
> > > BlueZ if they handle some profile and can decode battery
> > > reporting.
> > >
> > > The battery information is then exposed externally via the
> > > existing
> > > Battery1 interface. UI components can consume this API to display
> > > Bluetooth peripherals' battery via a unified BlueZ API.
> >
> > Was this patch reviewed for potential security problems? From the
> > top
> > of my head, the possible problems would be:
> > - I don't see any filters on which user could register battery
> > providers, so on a multi user system, you could have a user logged
> > in
> > via SSH squatting all the battery providers, while the user "at the
> > console" can't have their own providers. Also, what happens if the
> > user
> > at the console changes (fast user switching)?
> > - It looks like battery providers don't check for paired, trusted
> > or
> > even connected devices, so I would be able to create nearly unbound
> > number of battery providers depending on how big the cache for
> > "seen"
> > devices is.
> For security, the API can be access-limited at D-Bus level using D-
> Bus
> configuration files. For example, we can let only trusted UNIX users
> as the callers for this API. This D-Bus config file would be
> distribution-specific. In Chrome OS, for example, only the "audio"
> and
> "power" users are allowed to call this API. This way we can make sure
> that the callers do not abuse the API for denial-of-service kind of
> attack.

That wouldn't solve it, the point is to avoid one user causing problems
for another logged in user. If both users are in the audio group, which
they'd likely be to be able to use the computer, they'd be able to
cause problems to each other.

>
> >
> > Given that the interface between upower and bluez is supposedly
> > trusted, it might be good to ensure that there are no fuzzing
> > problems
> > on the bluez API side that could translate to causing problems in
> > upower itself.
> Could you give an example of what potential problems of upower can be
> caused by communicating with BlueZ through this API?

I haven't looked at the code in depth, but I would expect property
types to be checked before being exported, rather than relying on the
original dbus type matching the expected export type, this sort of
thing.

>
> >
> > I didn't review the code in depth, but, having written this
> > mechanism
> > for Bluetooth battery reporting, I think that this is the right way
> > to
> > go to allow daemons like pulseaudio to report battery status.
> >
> > Cheers
> >

2020-11-19 10:53:58

by Bastien Nocera

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

On Tue, 2020-11-17 at 14:22 -0800, Sonny Sasaka wrote:
> Hi Bastien,
>
> More responses below.
>
> On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <[email protected]>
> wrote:
> >
> > On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > > <
> > > <snip> didn't review the code in depth, but, having written this
> > > mechanism
> > > for Bluetooth battery reporting, I think that this is the right
> > > way
> > > to
> > > go to allow daemons like pulseaudio to report battery status.
> >
> > It would also be useful to know what external components, or
> > internal
> > plugins you expect to feed this API.
> BlueZ could have internal plugins to read proprietary battery
> reporting, e.g. JBL speakers and Bose QC35.

But you don't need to go over D-Bus to implement this.

>
> >
> > Apparently HSP/HFP, for example, provide more information that what
> > can
> > be expressed with the Battery1 API, whether it is charging or not,
> > or
> > whether a battery level is unknown, etc.
> >
> > So I would say that, even if the current battery API is extended,
> > we
> > need to make sure that the D-Bus API to feed new data is extensible
> > enough to allow new properties, and they don't need to be added
> > separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
> I proposed the API to start simple, but I believe that it can always
> be extended as we need in the future so I don't think the additional
> features need to be coded now. I documented how this API could evolve
> in the future by extending other features as well in this design
> document that I shared with Luiz and Marcel:
>
> https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps
> .

Right. My advice would have been to say "the properties exported by
BatteryProvider1 API match the properties exported by the Battery1
API", so you don't need to update 2 APIs separately when the API is
extended.

>
> >

2020-11-19 20:16:31

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Luiz,


On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Sonny,
>
> On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <[email protected]> wrote:
> >
> > Hi Bastien,
> >
> > Thank you for the feedback. Please find my answers below.
> >
> > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <[email protected]> wrote:
> > >
> > > Hey Sonny,
> > >
> > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > This patch implements the BatteryProvider1 and
> > > > BatteryProviderManager1
> > > > API. This is a means for external clients to feed battery information
> > > > to
> > > > BlueZ if they handle some profile and can decode battery reporting.
> > > >
> > > > The battery information is then exposed externally via the existing
> > > > Battery1 interface. UI components can consume this API to display
> > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > >
> > > Was this patch reviewed for potential security problems? From the top
> > > of my head, the possible problems would be:
> > > - I don't see any filters on which user could register battery
> > > providers, so on a multi user system, you could have a user logged in
> > > via SSH squatting all the battery providers, while the user "at the
> > > console" can't have their own providers. Also, what happens if the user
> > > at the console changes (fast user switching)?
> > > - It looks like battery providers don't check for paired, trusted or
> > > even connected devices, so I would be able to create nearly unbound
> > > number of battery providers depending on how big the cache for "seen"
> > > devices is.
> > For security, the API can be access-limited at D-Bus level using D-Bus
> > configuration files. For example, we can let only trusted UNIX users
> > as the callers for this API. This D-Bus config file would be
> > distribution-specific. In Chrome OS, for example, only the "audio" and
> > "power" users are allowed to call this API. This way we can make sure
> > that the callers do not abuse the API for denial-of-service kind of
> > attack.
>
> I guess there is still some the risk of conflicts even with the use of
> D-Bus policy blocking roge applications, there could still be multiple
> entities providing the battery status from the same source, which is
> why I suggested we couple the Battery1 with the Profile1, so only the
> entity that has registered to handle let say HFP can provide a battery
> status and we automatically deduce the source is from HFP.

These are two different issues. The issue with bad applications can be
overcome with D-Bus policy. The issue with multiple providers is
inherent: we have to have a duplicate resolution because one device
may report battery from different sources, e.g. via HFP and A2DP at
the same time. The latter case is rare in practice but still possible,
so I propose the simplest duplication resolution which is accept the
first provider registered (of that specific device) and ignore the
rest.

Coupling Battery1 with Profile1 will limit the flexibility of this
feature. For example, some speakers report battery status via a
separate LE channel (GATT). If we require Battery provider to be also
a registered Profile, we won't be able to support these cases since
GATT clients do not register any profile.


>
> > >
> > > Given that the interface between upower and bluez is supposedly
> > > trusted, it might be good to ensure that there are no fuzzing problems
> > > on the bluez API side that could translate to causing problems in
> > > upower itself.
> > Could you give an example of what potential problems of upower can be
> > caused by communicating with BlueZ through this API?
> >
> > >
> > > I didn't review the code in depth, but, having written this mechanism
> > > for Bluetooth battery reporting, I think that this is the right way to
> > > go to allow daemons like pulseaudio to report battery status.
> > >
> > > Cheers
> > >
>
>
>
> --
> Luiz Augusto von Dentz

2020-11-19 20:21:35

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Bastien,

On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <[email protected]> wrote:
>
> On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> > Hi Bastien,
> >
> > Thank you for the feedback. Please find my answers below.
> >
> > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <[email protected]>
> > wrote:
> > >
> > > Hey Sonny,
> > >
> > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > This patch implements the BatteryProvider1 and
> > > > BatteryProviderManager1
> > > > API. This is a means for external clients to feed battery
> > > > information
> > > > to
> > > > BlueZ if they handle some profile and can decode battery
> > > > reporting.
> > > >
> > > > The battery information is then exposed externally via the
> > > > existing
> > > > Battery1 interface. UI components can consume this API to display
> > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > >
> > > Was this patch reviewed for potential security problems? From the
> > > top
> > > of my head, the possible problems would be:
> > > - I don't see any filters on which user could register battery
> > > providers, so on a multi user system, you could have a user logged
> > > in
> > > via SSH squatting all the battery providers, while the user "at the
> > > console" can't have their own providers. Also, what happens if the
> > > user
> > > at the console changes (fast user switching)?
> > > - It looks like battery providers don't check for paired, trusted
> > > or
> > > even connected devices, so I would be able to create nearly unbound
> > > number of battery providers depending on how big the cache for
> > > "seen"
> > > devices is.
> > For security, the API can be access-limited at D-Bus level using D-
> > Bus
> > configuration files. For example, we can let only trusted UNIX users
> > as the callers for this API. This D-Bus config file would be
> > distribution-specific. In Chrome OS, for example, only the "audio"
> > and
> > "power" users are allowed to call this API. This way we can make sure
> > that the callers do not abuse the API for denial-of-service kind of
> > attack.
>
> That wouldn't solve it, the point is to avoid one user causing problems
> for another logged in user. If both users are in the audio group, which
> they'd likely be to be able to use the computer, they'd be able to
> cause problems to each other.

If I understand your case correctly, both users being in "audio" group
still won't allow them both to become battery providers because the
D-Bus policy only allows "audio" user and not "audio" group.


>
> >
> > >
> > > Given that the interface between upower and bluez is supposedly
> > > trusted, it might be good to ensure that there are no fuzzing
> > > problems
> > > on the bluez API side that could translate to causing problems in
> > > upower itself.
> > Could you give an example of what potential problems of upower can be
> > caused by communicating with BlueZ through this API?
>
> I haven't looked at the code in depth, but I would expect property
> types to be checked before being exported, rather than relying on the
> original dbus type matching the expected export type, this sort of
> thing.
Yes, the code does not just forward information. The code processes
the input and exports it as a clearly defined output type.
>
> >
> > >
> > > I didn't review the code in depth, but, having written this
> > > mechanism
> > > for Bluetooth battery reporting, I think that this is the right way
> > > to
> > > go to allow daemons like pulseaudio to report battery status.
> > >
> > > Cheers
> > >
>
>

2020-11-19 20:27:06

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Bastien,

On Thu, Nov 19, 2020 at 2:53 AM Bastien Nocera <[email protected]> wrote:
>
> On Tue, 2020-11-17 at 14:22 -0800, Sonny Sasaka wrote:
> > Hi Bastien,
> >
> > More responses below.
> >
> > On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <[email protected]>
> > wrote:
> > >
> > > On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > > > <
> > > > <snip> didn't review the code in depth, but, having written this
> > > > mechanism
> > > > for Bluetooth battery reporting, I think that this is the right
> > > > way
> > > > to
> > > > go to allow daemons like pulseaudio to report battery status.
> > >
> > > It would also be useful to know what external components, or
> > > internal
> > > plugins you expect to feed this API.
> > BlueZ could have internal plugins to read proprietary battery
> > reporting, e.g. JBL speakers and Bose QC35.
>
> But you don't need to go over D-Bus to implement this.
Some proprietary protocols may not want to become an internal BlueZ
plugin, for example because it is developed closed source. D-Bus API
is useful to support these cases.
>
> >
> > >
> > > Apparently HSP/HFP, for example, provide more information that what
> > > can
> > > be expressed with the Battery1 API, whether it is charging or not,
> > > or
> > > whether a battery level is unknown, etc.
> > >
> > > So I would say that, even if the current battery API is extended,
> > > we
> > > need to make sure that the D-Bus API to feed new data is extensible
> > > enough to allow new properties, and they don't need to be added
> > > separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
> > I proposed the API to start simple, but I believe that it can always
> > be extended as we need in the future so I don't think the additional
> > features need to be coded now. I documented how this API could evolve
> > in the future by extending other features as well in this design
> > document that I shared with Luiz and Marcel:
> >
> > https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps
> > .
>
> Right. My advice would have been to say "the properties exported by
> BatteryProvider1 API match the properties exported by the Battery1
> API", so you don't need to update 2 APIs separately when the API is
> extended.
I am considering doing this. Let me think it through to make sure we
don't stumble on anything in the future if we do it this way.

>
> >
> > >
>
>

2020-11-19 23:58:22

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Sonny,

On Thu, Nov 19, 2020 at 12:15 PM Sonny Sasaka <[email protected]> wrote:
>
> Hi Luiz,
>
>
> On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Sonny,
> >
> > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <[email protected]> wrote:
> > >
> > > Hi Bastien,
> > >
> > > Thank you for the feedback. Please find my answers below.
> > >
> > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <[email protected]> wrote:
> > > >
> > > > Hey Sonny,
> > > >
> > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > This patch implements the BatteryProvider1 and
> > > > > BatteryProviderManager1
> > > > > API. This is a means for external clients to feed battery information
> > > > > to
> > > > > BlueZ if they handle some profile and can decode battery reporting.
> > > > >
> > > > > The battery information is then exposed externally via the existing
> > > > > Battery1 interface. UI components can consume this API to display
> > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > >
> > > > Was this patch reviewed for potential security problems? From the top
> > > > of my head, the possible problems would be:
> > > > - I don't see any filters on which user could register battery
> > > > providers, so on a multi user system, you could have a user logged in
> > > > via SSH squatting all the battery providers, while the user "at the
> > > > console" can't have their own providers. Also, what happens if the user
> > > > at the console changes (fast user switching)?
> > > > - It looks like battery providers don't check for paired, trusted or
> > > > even connected devices, so I would be able to create nearly unbound
> > > > number of battery providers depending on how big the cache for "seen"
> > > > devices is.
> > > For security, the API can be access-limited at D-Bus level using D-Bus
> > > configuration files. For example, we can let only trusted UNIX users
> > > as the callers for this API. This D-Bus config file would be
> > > distribution-specific. In Chrome OS, for example, only the "audio" and
> > > "power" users are allowed to call this API. This way we can make sure
> > > that the callers do not abuse the API for denial-of-service kind of
> > > attack.
> >
> > I guess there is still some the risk of conflicts even with the use of
> > D-Bus policy blocking roge applications, there could still be multiple
> > entities providing the battery status from the same source, which is
> > why I suggested we couple the Battery1 with the Profile1, so only the
> > entity that has registered to handle let say HFP can provide a battery
> > status and we automatically deduce the source is from HFP.
>
> These are two different issues. The issue with bad applications can be
> overcome with D-Bus policy. The issue with multiple providers is
> inherent: we have to have a duplicate resolution because one device
> may report battery from different sources, e.g. via HFP and A2DP at
> the same time. The latter case is rare in practice but still possible,
> so I propose the simplest duplication resolution which is accept the
> first provider registered (of that specific device) and ignore the
> rest.
>
> Coupling Battery1 with Profile1 will limit the flexibility of this
> feature. For example, some speakers report battery status via a
> separate LE channel (GATT). If we require Battery provider to be also
> a registered Profile, we won't be able to support these cases since
> GATT clients do not register any profile.

Actually it is a good policy to provide GattProfile1 if you are
interested in enabling auto-connect, which I suppose most third-party
services would like to enable:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt#n367

Note that we are doing to avoid complicate conflict resolution since
profiles by design can only be registered once that means Battery
sources will also be limited to one per profile, Im fine if you choose
not to have this integration in the first version .

>
> >
> > > >
> > > > Given that the interface between upower and bluez is supposedly
> > > > trusted, it might be good to ensure that there are no fuzzing problems
> > > > on the bluez API side that could translate to causing problems in
> > > > upower itself.
> > > Could you give an example of what potential problems of upower can be
> > > caused by communicating with BlueZ through this API?
> > >
> > > >
> > > > I didn't review the code in depth, but, having written this mechanism
> > > > for Bluetooth battery reporting, I think that this is the right way to
> > > > go to allow daemons like pulseaudio to report battery status.
> > > >
> > > > Cheers
> > > >
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



--
Luiz Augusto von Dentz

2020-11-20 10:37:12

by Bastien Nocera

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

On Thu, 2020-11-19 at 12:20 -0800, Sonny Sasaka wrote:
> Hi Bastien,
>
> On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <[email protected]>
> wrote:
> >
> > On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> > > Hi Bastien,
> > >
> > > Thank you for the feedback. Please find my answers below.
> > >
> > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera
> > > <[email protected]>
> > > wrote:
> > > >
> > > > Hey Sonny,
> > > >
> > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > This patch implements the BatteryProvider1 and
> > > > > BatteryProviderManager1
> > > > > API. This is a means for external clients to feed battery
> > > > > information
> > > > > to
> > > > > BlueZ if they handle some profile and can decode battery
> > > > > reporting.
> > > > >
> > > > > The battery information is then exposed externally via the
> > > > > existing
> > > > > Battery1 interface. UI components can consume this API to
> > > > > display
> > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > >
> > > > Was this patch reviewed for potential security problems? From
> > > > the
> > > > top
> > > > of my head, the possible problems would be:
> > > > - I don't see any filters on which user could register battery
> > > > providers, so on a multi user system, you could have a user
> > > > logged
> > > > in
> > > > via SSH squatting all the battery providers, while the user "at
> > > > the
> > > > console" can't have their own providers. Also, what happens if
> > > > the
> > > > user
> > > > at the console changes (fast user switching)?
> > > > - It looks like battery providers don't check for paired,
> > > > trusted
> > > > or
> > > > even connected devices, so I would be able to create nearly
> > > > unbound
> > > > number of battery providers depending on how big the cache for
> > > > "seen"
> > > > devices is.
> > > For security, the API can be access-limited at D-Bus level using
> > > D-
> > > Bus
> > > configuration files. For example, we can let only trusted UNIX
> > > users
> > > as the callers for this API. This D-Bus config file would be
> > > distribution-specific. In Chrome OS, for example, only the
> > > "audio"
> > > and
> > > "power" users are allowed to call this API. This way we can make
> > > sure
> > > that the callers do not abuse the API for denial-of-service kind
> > > of
> > > attack.
> >
> > That wouldn't solve it, the point is to avoid one user causing
> > problems
> > for another logged in user. If both users are in the audio group,
> > which
> > they'd likely be to be able to use the computer, they'd be able to
> > cause problems to each other.
>
> If I understand your case correctly, both users being in "audio"
> group
> still won't allow them both to become battery providers because the
> D-Bus policy only allows "audio" user and not "audio" group.

OK, I guess that means that this is a separate daemon running as a
different user, not, say, PulseAudio running in the user's session
feeding information. Is that right?

Either way, I guess we'll need to test this out once the feature is
merged.

Apart from the concern about having to duplicate the exported
properties, the rest looks good. I've made some additional comments
about the architecture in the design document you shared, but those
should not have any impact on the implementation.

Good job :)

2020-11-20 19:43:38

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Bastien,

On Fri, Nov 20, 2020 at 2:33 AM Bastien Nocera <[email protected]> wrote:
>
> On Thu, 2020-11-19 at 12:20 -0800, Sonny Sasaka wrote:
> > Hi Bastien,
> >
> > On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <[email protected]>
> > wrote:
> > >
> > > On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> > > > Hi Bastien,
> > > >
> > > > Thank you for the feedback. Please find my answers below.
> > > >
> > > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera
> > > > <[email protected]>
> > > > wrote:
> > > > >
> > > > > Hey Sonny,
> > > > >
> > > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > > This patch implements the BatteryProvider1 and
> > > > > > BatteryProviderManager1
> > > > > > API. This is a means for external clients to feed battery
> > > > > > information
> > > > > > to
> > > > > > BlueZ if they handle some profile and can decode battery
> > > > > > reporting.
> > > > > >
> > > > > > The battery information is then exposed externally via the
> > > > > > existing
> > > > > > Battery1 interface. UI components can consume this API to
> > > > > > display
> > > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > > >
> > > > > Was this patch reviewed for potential security problems? From
> > > > > the
> > > > > top
> > > > > of my head, the possible problems would be:
> > > > > - I don't see any filters on which user could register battery
> > > > > providers, so on a multi user system, you could have a user
> > > > > logged
> > > > > in
> > > > > via SSH squatting all the battery providers, while the user "at
> > > > > the
> > > > > console" can't have their own providers. Also, what happens if
> > > > > the
> > > > > user
> > > > > at the console changes (fast user switching)?
> > > > > - It looks like battery providers don't check for paired,
> > > > > trusted
> > > > > or
> > > > > even connected devices, so I would be able to create nearly
> > > > > unbound
> > > > > number of battery providers depending on how big the cache for
> > > > > "seen"
> > > > > devices is.
> > > > For security, the API can be access-limited at D-Bus level using
> > > > D-
> > > > Bus
> > > > configuration files. For example, we can let only trusted UNIX
> > > > users
> > > > as the callers for this API. This D-Bus config file would be
> > > > distribution-specific. In Chrome OS, for example, only the
> > > > "audio"
> > > > and
> > > > "power" users are allowed to call this API. This way we can make
> > > > sure
> > > > that the callers do not abuse the API for denial-of-service kind
> > > > of
> > > > attack.
> > >
> > > That wouldn't solve it, the point is to avoid one user causing
> > > problems
> > > for another logged in user. If both users are in the audio group,
> > > which
> > > they'd likely be to be able to use the computer, they'd be able to
> > > cause problems to each other.
> >
> > If I understand your case correctly, both users being in "audio"
> > group
> > still won't allow them both to become battery providers because the
> > D-Bus policy only allows "audio" user and not "audio" group.
>
> OK, I guess that means that this is a separate daemon running as a
> different user, not, say, PulseAudio running in the user's session
> feeding information. Is that right?
Yes, that's right.

>
> Either way, I guess we'll need to test this out once the feature is
> merged.
It will first be tested in Chrome OS with CRAS as the battery provider
from HFP (I am working on it too). I guess PulseAudio can then follow
along so Linux desktops can get headphones batteries in the UI. Then,
as Luiz suggested, batteries from HID will be parsed directly from
within bluetoothd (a TODO, not blocking the progress of the API).

>
> Apart from the concern about having to duplicate the exported
> properties, the rest looks good. I've made some additional comments
> about the architecture in the design document you shared, but those
> should not have any impact on the implementation.

Thanks for the review. I will update the patches based on your and
Luiz's feedback.
>
> Good job :)
>

2020-11-20 20:33:59

by Sonny Sasaka

Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

Hi Luiz,

On Thu, Nov 19, 2020 at 3:56 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Sonny,
>
> On Thu, Nov 19, 2020 at 12:15 PM Sonny Sasaka <[email protected]> wrote:
> >
> > Hi Luiz,
> >
> >
> > On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi Sonny,
> > >
> > > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <[email protected]> wrote:
> > > >
> > > > Hi Bastien,
> > > >
> > > > Thank you for the feedback. Please find my answers below.
> > > >
> > > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <[email protected]> wrote:
> > > > >
> > > > > Hey Sonny,
> > > > >
> > > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > > This patch implements the BatteryProvider1 and
> > > > > > BatteryProviderManager1
> > > > > > API. This is a means for external clients to feed battery information
> > > > > > to
> > > > > > BlueZ if they handle some profile and can decode battery reporting.
> > > > > >
> > > > > > The battery information is then exposed externally via the existing
> > > > > > Battery1 interface. UI components can consume this API to display
> > > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > > >
> > > > > Was this patch reviewed for potential security problems? From the top
> > > > > of my head, the possible problems would be:
> > > > > - I don't see any filters on which user could register battery
> > > > > providers, so on a multi user system, you could have a user logged in
> > > > > via SSH squatting all the battery providers, while the user "at the
> > > > > console" can't have their own providers. Also, what happens if the user
> > > > > at the console changes (fast user switching)?
> > > > > - It looks like battery providers don't check for paired, trusted or
> > > > > even connected devices, so I would be able to create nearly unbound
> > > > > number of battery providers depending on how big the cache for "seen"
> > > > > devices is.
> > > > For security, the API can be access-limited at D-Bus level using D-Bus
> > > > configuration files. For example, we can let only trusted UNIX users
> > > > as the callers for this API. This D-Bus config file would be
> > > > distribution-specific. In Chrome OS, for example, only the "audio" and
> > > > "power" users are allowed to call this API. This way we can make sure
> > > > that the callers do not abuse the API for denial-of-service kind of
> > > > attack.
> > >
> > > I guess there is still some the risk of conflicts even with the use of
> > > D-Bus policy blocking roge applications, there could still be multiple
> > > entities providing the battery status from the same source, which is
> > > why I suggested we couple the Battery1 with the Profile1, so only the
> > > entity that has registered to handle let say HFP can provide a battery
> > > status and we automatically deduce the source is from HFP.
> >
> > These are two different issues. The issue with bad applications can be
> > overcome with D-Bus policy. The issue with multiple providers is
> > inherent: we have to have a duplicate resolution because one device
> > may report battery from different sources, e.g. via HFP and A2DP at
> > the same time. The latter case is rare in practice but still possible,
> > so I propose the simplest duplication resolution which is accept the
> > first provider registered (of that specific device) and ignore the
> > rest.
> >
> > Coupling Battery1 with Profile1 will limit the flexibility of this
> > feature. For example, some speakers report battery status via a
> > separate LE channel (GATT). If we require Battery provider to be also
> > a registered Profile, we won't be able to support these cases since
> > GATT clients do not register any profile.
>
> Actually it is a good policy to provide GattProfile1 if you are
> interested in enabling auto-connect, which I suppose most third-party
> services would like to enable:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt#n367
>
> Note that we are doing to avoid complicate conflict resolution since
> profiles by design can only be registered once that means Battery
> sources will also be limited to one per profile, Im fine if you choose
> not to have this integration in the first version .
Thanks, Luiz. I will proceed without profile integration in the first
iteration, since battery sources will be limited to one per profile
anyway.

>
> >
> > >
> > > > >
> > > > > Given that the interface between upower and bluez is supposedly
> > > > > trusted, it might be good to ensure that there are no fuzzing problems
> > > > > on the bluez API side that could translate to causing problems in
> > > > > upower itself.
> > > > Could you give an example of what potential problems of upower can be
> > > > caused by communicating with BlueZ through this API?
> > > >
> > > > >
> > > > > I didn't review the code in depth, but, having written this mechanism
> > > > > for Bluetooth battery reporting, I think that this is the right way to
> > > > > go to allow daemons like pulseaudio to report battery status.
> > > > >
> > > > > Cheers
> > > > >
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz