Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201111011745.2016-1-sonnysasaka@chromium.org>
 <20201111011745.2016-7-sonnysasaka@chromium.org> <aa1c080e8a7813299e6a093608211684e074e427.camel@hadess.net>
 <CAO271m=O3hyS6Pp4fQ1pnsir7wYbLFwDm7f-a5yd0o4NTUUewA@mail.gmail.com>
 <CABBYNZJjgdjsDXOQk=v4wJ7PLyHr-u2w6d+jgLKgwxV9J5OYdQ@mail.gmail.com>
 <CAO271m=L8yoD0JO70kVJqmH0+gBkCL=_dtpTKNDZ7ei5jfPz6w@mail.gmail.com> <CABBYNZ+FBdCzqqcOuaKou-A5S_QCZsgHvj0ygyZP3L3v90L_8A@mail.gmail.com>
In-Reply-To: <CABBYNZ+FBdCzqqcOuaKou-A5S_QCZsgHvj0ygyZP3L3v90L_8A@mail.gmail.com>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Fri, 20 Nov 2020 12:33:16 -0800
Message-ID: <CAO271mkGiN=qZozt9+yUAnqtz35GpkSWbW9A9pWicMSx6d3ZLw@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Bastien Nocera <hadess@hadess.net>,
        BlueZ <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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