Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1403131286-15028-1-git-send-email-lukasz.rymanowski@tieto.com> <1403131286-15028-5-git-send-email-lukasz.rymanowski@tieto.com> Date: Fri, 20 Jun 2014 01:18:32 +0200 Message-ID: Subject: Re: [PATCH 4/4] android/hidhost: Start encryption for HOG when bonded From: Lukasz Rymanowski To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" , Szymon Janc Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: HI Luiz, On 19 June 2014 15:25, Luiz Augusto von Dentz wrote: > Hi Lukasz, > > On Thu, Jun 19, 2014 at 1:41 AM, Lukasz Rymanowski > wrote: >> HOG requires encryption on connection, so make sure it is on. >> >> On the other hand we don't need medium security always when >> connecting LE device even device are bonded. It depends on permissions >> on characteristics. That's why we want security low in connect_le() >> --- >> android/gatt.c | 6 +----- >> android/hidhost.c | 4 ++++ >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/android/gatt.c b/android/gatt.c >> index 9471eaf..746316d 100644 >> --- a/android/gatt.c >> +++ b/android/gatt.c >> @@ -1419,7 +1419,6 @@ reply: >> >> static int connect_le(struct gatt_device *dev) >> { >> - BtIOSecLevel sec_level; >> GIOChannel *io; >> GError *gerr = NULL; >> char addr[18]; >> @@ -1434,9 +1433,6 @@ static int connect_le(struct gatt_device *dev) >> >> DBG("Connection attempt to: %s", addr); >> >> - sec_level = bt_device_is_bonded(&dev->bdaddr) ? BT_IO_SEC_MEDIUM : >> - BT_IO_SEC_LOW; >> - >> /* >> * This connection will help us catch any PDUs that comes before >> * pairing finishes >> @@ -1448,7 +1444,7 @@ static int connect_le(struct gatt_device *dev) >> BT_IO_OPT_DEST_BDADDR, &dev->bdaddr, >> BT_IO_OPT_DEST_TYPE, dev->bdaddr_type, >> BT_IO_OPT_CID, ATT_CID, >> - BT_IO_OPT_SEC_LEVEL, sec_level, >> + BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW, >> BT_IO_OPT_INVALID); >> if (!io) { >> error("gatt: Failed bt_io_connect(%s): %s", addr, >> diff --git a/android/hidhost.c b/android/hidhost.c >> index 846dd57..1f66454 100644 >> --- a/android/hidhost.c >> +++ b/android/hidhost.c >> @@ -777,6 +777,10 @@ static void hog_conn_cb(const bdaddr_t *addr, int err, void *attrib) >> bt_hid_notify_state(dev, HAL_HIDHOST_STATE_CONNECTING); >> } >> >> + /* If device is bonded lets enable encryption */ >> + if (bt_device_is_bonded(addr)) >> + bt_gatt_set_security(addr, BT_SECURITY_MEDIUM); >> + > > Don't we need to wait the encryption change to only then start sending > commands? Actually this gets more complicate since the specs says the > following: > > "If the HID Host receives the L2CAP Connection Parameter Update > request but has not > yet completed service discovery or has not completed encryption, the > HID Host may > send the L2CAP Connection Parameter Update Response with the Result field > indicating that the request has been rejected." > As far as I understand, socket will stop flow until the security level is updated. > So it seems HoG may have some requirements on L2CAP as well so it is > not per characteristics, so perhaps connect_le should take the initial > security level then we can add some security requirement to > bt_gatt_connect_app or something like that. Btw, Im not aware of any > drawback regarding encryption even if is not mandatory it will just > make the connection more secure, but perhaps this is fixing another > problem? One of the drawback is GATT PTS testcase TC_GAW_CL_BV_02_C which test signed wrtie. Scenario is that device do bonding and exchange CSRK, then disconnect and connect again. Once their are reconnected signed write should be performed. Note that signed write is no allowed on encrypted link. > >> if (!dev->hog) { >> /* TODO: Get device details and primary */ >> dev->hog = bt_hog_new("bluez-input-device", dev->vendor, >> -- >> 1.8.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Luiz Augusto von Dentz BR Lukasz