Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Reply-To: csonsino@gmail.com
Subject: Re: [PATCH 1/1] bluetooth: validate BLE connection interval updates
To:     Andreas Kemnade <andreas@kemnade.info>,
        Marcel Holtmann <marcel@holtmann.org>
Cc:     Johan Hedberg <johan.hedberg@gmail.com>,
        linux-bluetooth@vger.kernel.org, Sasha Levin <sashal@kernel.org>
References: <f9176553-d4d5-c2e0-ecd7-937cac0d8098@gmail.com>
 <138296C5-49FA-475D-9618-FC8D241B8823@holtmann.org>
 <20190815104419.161177fa@kemnade.info>
From:   Carey Sonsino <csonsino@gmail.com>
Message-ID: <0cda8242-304e-a073-90d8-63e656e3600c@gmail.com>
Date:   Mon, 19 Aug 2019 07:58:19 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190815104419.161177fa@kemnade.info>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

This seems like the exact "downside" situation that I described in the 
patch writeup.

I would still claim that as a Linux system administrator, I should have 
control over my system.? If I am operating in a low power environment, I 
do not want to allow a remote device to apply a setting which causes me 
to use more power without any say in the matter.

The connection min/max interval settings are configuration options that 
control how my bluetooth device operates.? Why are these down in debugfs 
anyway?? I think that a much more appropriate fix would be to migrate 
some of the BLE configuration options to sysconfdir (some place like 
/etc/bluetooth/ble.conf).? That would also help in the persistence of 
these configuration settings, which is kind of a pain with the debugfs 
mechanism that gets wiped out and recreated on bootup.

A quicker fix would be to simply set the default connection min interval 
and connection max interval values to the full range (6, 3200).? I 
*think* that this could be done by simply updating the values in 
hci_core.c, the hci_alloc_dev() function:

 ??? hdev->le_conn_min_interval = 0x0018;
 ??? hdev->le_conn_max_interval = 0x0028;

would become:

 ??? hdev->le_conn_min_interval = 0x0006;
 ??? hdev->le_conn_max_interval = 0x0c80;

This should allow all devices to negotiate whatever connection interval 
they want by default.? If I'm running on a device with debugfs (which I 
happen to be most of the time), then I can still override these defaults 
to control my system.

Please let me know if you would like me to do any further work towards 
resolving this issue.? I'd be happy to test and submit a patch that 
changes the default connection min/max interval values- I could probably 
get that done in the next day or two.? If you would like me to 
investigate migrating configuration settings to /etc then I'd be happy 
to do that as well, but it might take a bit more effort and time.

Regards,

Carey


On 8/15/19 2:44 AM, Andreas Kemnade wrote:
> On Sat, 6 Jul 2019 15:34:34 +0200
> Marcel Holtmann <marcel@holtmann.org> wrote:
>
>> Hi Carey,
>>
>>> Problem: The Linux bluetooth stack yields complete control over the BLE
>>> connection interval to the remote device.
>>>
>>> The Linux bluetooth stack provides access to the BLE connection interval
>>> min and max values through /sys/kernel/debug/bluetooth/hci0/
>>> conn_min_interval and /sys/kernel/debug/bluetooth/hci0/conn_max_interval.
>>> These values are used for initial BLE connections, but the remote device
>>> has the ability to request a connection parameter update. In the event
>>> that the remote side requests to change the connection interval, the Linux
>>> kernel currently only validates that the desired value is within the
>>> acceptable range in the bluetooth specification (6 - 3200, corresponding to
>>> 7.5ms - 4000ms). There is currently no validation that the desired value
>>> requested by the remote device is within the min/max limits specified in
>>> the conn_min_interval/conn_max_interval configurations. This essentially
>>> leads to Linux yielding complete control over the connection interval to
>>> the remote device.
>>>
>>> The proposed patch adds a verification step to the connection parameter
>>> update mechanism, ensuring that the desired value is within the min/max
>>> bounds of the current connection. If the desired value is outside of the
>>> current connection min/max values, then the connection parameter update
>>> request is rejected and the negative response is returned to the remote
>>> device. Recall that the initial connection is established using the local
>>> conn_min_interval/conn_max_interval values, so this allows the Linux
>>> administrator to retain control over the BLE connection interval.
>>>
>>> The one downside that I see is that the current default Linux values for
>>> conn_min_interval and conn_max_interval typically correspond to 30ms and
>>> 50ms respectively. If this change were accepted, then it is feasible that
>>> some devices would no longer be able to negotiate to their desired
>>> connection interval values. This might be remedied by setting the default
>>> Linux conn_min_interval and conn_max_interval values to the widest
>>> supported range (6 - 3200 / 7.5ms - 4000ms). This could lead to the same
>>> behavior as the current implementation, where the remote device could
>>> request to change the connection interval value to any value that is
>>> permitted by the bluetooth specification, and Linux would accept the
>>> desired value.
>>>
>>> Signed-off-by: Carey Sonsino <csonsino@gmail.com>
>> patch has been applied to bluetooth-next tree.
>>
> There are devices which require low connection intervals for usable operation,
> e.g. BLE smartcard readers. having 30ms instead of 7.5ms means speed four times
> lower.
>
> Other devices might want to set the connection interval  to high values to save
> power.
>
> So if the device is not allowed to set the connection interval to such values,
> how should the driver sitting on top of the gatt dbus interface of bluetoothd
> set such things?
>
> The debugfs nodes are not necessarily available in distro kernels. Anything
> sitting on top of gatt dbus interface does not have access to the connection handle
> and cannot call hci_le_conn_update() to set things to nice values.
>
> Using l2cap sockets instead of the dbus gatt interface is also not a viable altenative
> because it interferes with bluetoothd.
>
> So IMHO this patch causes regressions and should be reverted.
>
> Sorry for stepping out this late.
>
> Regards,
> Andreas