diff mbox series

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

Message ID 20201111011745.2016-7-sonnysasaka@chromium.org
State Superseded
Headers show
Series [BlueZ,v2,1/7] battery: Add the internal Battery API | expand

Commit Message

Sonny Sasaka Nov. 11, 2020, 1:17 a.m. UTC
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 <mcchou@chromium.org>

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

Comments

Bastien Nocera Nov. 17, 2020, 10:51 a.m. UTC | #1
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
Bastien Nocera Nov. 17, 2020, 6:01 p.m. UTC | #2
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.
Sonny Sasaka Nov. 17, 2020, 10:16 p.m. UTC | #3
Hi Bastien,

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

On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> 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

>
Sonny Sasaka Nov. 17, 2020, 10:22 p.m. UTC | #4
Hi Bastien,

More responses below.

On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <hadess@hadess.net> 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.

>
Sonny Sasaka Nov. 17, 2020, 10:26 p.m. UTC | #5
Hi Bastien,

On Tue, Nov 17, 2020 at 2:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>

> Hi Bastien,

>

> More responses below.

>

> On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <hadess@hadess.net> 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.

>

> >
Luiz Augusto von Dentz Nov. 17, 2020, 10:26 p.m. UTC | #6
Hi Sonny,

On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>

> Hi Bastien,

>

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

>

> On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> 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
Bastien Nocera Nov. 19, 2020, 10:44 a.m. UTC | #7
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 <hadess@hadess.net>

> 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

> >
Bastien Nocera Nov. 19, 2020, 10:53 a.m. UTC | #8
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 <hadess@hadess.net>

> 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.

> 

> >
Sonny Sasaka Nov. 19, 2020, 8:15 p.m. UTC | #9
Hi Luiz,


On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>

> Hi Sonny,

>

> On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:

> >

> > Hi Bastien,

> >

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

> >

> > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> 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
Sonny Sasaka Nov. 19, 2020, 8:20 p.m. UTC | #10
Hi Bastien,

On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <hadess@hadess.net> 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 <hadess@hadess.net>

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

> > >

>

>
Sonny Sasaka Nov. 19, 2020, 8:25 p.m. UTC | #11
Hi Bastien,

On Thu, Nov 19, 2020 at 2:53 AM Bastien Nocera <hadess@hadess.net> 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 <hadess@hadess.net>

> > 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.

>

> >

> > >

>

>
Luiz Augusto von Dentz Nov. 19, 2020, 11:56 p.m. UTC | #12
Hi Sonny,

On Thu, Nov 19, 2020 at 12:15 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
>
> On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sonny,
> >
> > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > >
> > > Hi Bastien,
> > >
> > > Thank you for the feedback. Please find my answers below.
> > >
> > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> 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
Bastien Nocera Nov. 20, 2020, 10:33 a.m. UTC | #13
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 <hadess@hadess.net>

> 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

> > > <hadess@hadess.net>

> > > 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 :)
Sonny Sasaka Nov. 20, 2020, 7:41 p.m. UTC | #14
Hi Bastien,

On Fri, Nov 20, 2020 at 2:33 AM Bastien Nocera <hadess@hadess.net> 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 <hadess@hadess.net>

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

> > > > <hadess@hadess.net>

> > > > 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 :)

>
Sonny Sasaka Nov. 20, 2020, 8:33 p.m. UTC | #15
Hi Luiz,

On Thu, Nov 19, 2020 at 3:56 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>

> Hi Sonny,

>

> On Thu, Nov 19, 2020 at 12:15 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:

> >

> > Hi Luiz,

> >

> >

> > On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz

> > <luiz.dentz@gmail.com> wrote:

> > >

> > > Hi Sonny,

> > >

> > > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:

> > > >

> > > > Hi Bastien,

> > > >

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

> > > >

> > > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> 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
diff mbox series

Patch

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);