Hi Luiz, On Thu, Nov 19, 2020 at 3:56 PM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > > Hi Sonny, > > On Thu, Nov 19, 2020 at 12:15 PM Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx> wrote: > > > > Hi Luiz, > > > > > > On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > Hi Sonny, > > > > > > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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
