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;
Message-ID: <aa1c080e8a7813299e6a093608211684e074e427.camel@hadess.net>
Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API
From:   Bastien Nocera <hadess@hadess.net>
To:     Sonny Sasaka <sonnysasaka@chromium.org>,
        linux-bluetooth@vger.kernel.org
Cc:     Miao-chen Chou <mcchou@chromium.org>
Date:   Tue, 17 Nov 2020 11:51:34 +0100
In-Reply-To: <20201111011745.2016-7-sonnysasaka@chromium.org>
References: <20201111011745.2016-1-sonnysasaka@chromium.org>
         <20201111011745.2016-7-sonnysasaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.1 (3.38.1-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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