[PATCH BlueZ 0/4] GATT over LE reverse service discovery fixes

[PATCH BlueZ 0/4] GATT over LE reverse service discovery fixes

[Anderson Lizardo]

Hi,

This series fixes issues when doing reverse GATT service discovery over LE.

There is still one open issue: If BlueZ creates a device without bonding (i.e.
CreateDevice() D-Bus method), but the remote requests SMP pairing with
"Security Request", at the end of the pairing a reverse GATT service discovery
is issued, *regardless* of BlueZ being the initiator or not. BlueZ should only
do reverse discovery if it is the acceptor (see comments on the
device_bonding_complete() function on src/device.c).

To fix this, I think we need to have a more robust way to check whether we are
initiator or acceptor, instead of simply checking for an active bonding
request. Currently, if BlueZ receives keys from kernel over mgmt,
device_bonding_complete() is eventually called, and if there is no active
bonding, BlueZ assumes to be an acceptor.

In any case, with these patches we make sure this spurious service discovery
will not crash BlueZ or create leaks. It is also applicable for "normal"
reverse service discovery as well.

Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia (INdT)
Manaus - Brazil

[PATCH BlueZ 1/4] device: Fix NULL pointer dereference during GATT service discovery

[Anderson Lizardo]

If reverse service discovery for GATT is triggered,
device_browse_primary() is called with a NULL "conn" parameter. This fix
is based on similar code found in device_browse_sdp().

This fixes errors like:

process 871: arguments to dbus_connection_ref() were incorrect,
assertion "connection != NULL" failed in file dbus-connection.c line
2549.
This is normally a bug in some application using the D-Bus library.
process 871: arguments to dbus_connection_get_object_path_data() were
incorrect, assertion "connection != NULL" failed in file
dbus-connection.c line 5639.
This is normally a bug in some application using the D-Bus library.
process 871: arguments to dbus_connection_register_object_path() were
incorrect, assertion "connection != NULL" failed in file
dbus-connection.c line 5461.
This is normally a bug in some application using the D-Bus library.
---
 src/device.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/device.c b/src/device.c
index ca7b15b..50ab339 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2011,8 +2011,10 @@ int device_browse_primary(struct btd_device *device, DBusConnection *conn,
 	}
 
 done:
-	if (conn)
-		req->conn = dbus_connection_ref(conn);
+	if (conn == NULL)
+		conn = get_dbus_connection();
+
+	req->conn = dbus_connection_ref(conn);
 
 	if (msg) {
 		const char *sender = dbus_message_get_sender(msg);
-- 
1.7.5.4

[PATCH BlueZ 2/4] device: Fix invalid memory read during GATT discovery

[Anderson Lizardo]

device->browse should be set to NULL before calling
browse_request_free(), otherwise it points to freed memory.
---
 src/device.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index 50ab339..b581564 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2005,6 +2005,7 @@ int device_browse_primary(struct btd_device *device, DBusConnection *conn,
 				BT_IO_OPT_INVALID);
 
 	if (device->att_io == NULL) {
+		device->browse = NULL;
 		browse_request_free(req);
 		g_free(attcb);
 		return -EIO;
-- 
1.7.5.4

[PATCH BlueZ 3/4] device: Fix invalid D-Bus calls during Reverse GATT Discovery

[Anderson Lizardo]

There is no D-Bus client active during Reverse GATT Discovery, therefore
BlueZ should not send any D-Bus replies. This fixes errors reported by
D-Bus internal checks:

process 453: arguments to dbus_message_new_method_return() were
incorrect, assertion "method_call != NULL" failed in file dbus-message.c
line 1111.
---
 src/device.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/device.c b/src/device.c
index b581564..b410a63 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1799,9 +1799,12 @@ static void primary_cb(GSList *services, guint8 status, gpointer user_data)
 	GSList *l, *uuids = NULL;
 
 	if (status) {
-		DBusMessage *reply;
-		reply = btd_error_failed(req->msg, att_ecode2str(status));
-		g_dbus_send_message(req->conn, reply);
+		if (req->msg) {
+			DBusMessage *reply;
+			reply = btd_error_failed(req->msg,
+							att_ecode2str(status));
+			g_dbus_send_message(req->conn, reply);
+		}
 		goto done;
 	}
 
@@ -1821,7 +1824,8 @@ static void primary_cb(GSList *services, guint8 status, gpointer user_data)
 	g_slist_free(uuids);
 
 	services_changed(device);
-	create_device_reply(device, req);
+	if (req->msg)
+		create_device_reply(device, req);
 
 	store_services(device);
 
-- 
1.7.5.4

[PATCH BlueZ 4/4] device: Fix memory leak during GATT service update

[Anderson Lizardo]

GATT service updates are not supported yet. This patch avoids a memory
leak and GATT service storage corruption due to trying to register GATT
services twice.

After adding support for Services Changed characteristic notification
(which is mandatory for GATT clients), this check can be removed.
---
 src/device.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index b410a63..dfc8e59 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1982,6 +1982,14 @@ int device_browse_primary(struct btd_device *device, DBusConnection *conn,
 	if (device->browse)
 		return -EBUSY;
 
+	/* FIXME: GATT service updates (implemented in update_services() for
+	 * SDP) are not supported yet. It will be supported once client side
+	 * "Services Changed" characteristic handling is implemented. */
+	if (device->primaries) {
+		error("Could not update GATT services");
+		return -ENOSYS;
+	}
+
 	req = g_new0(struct browse_req, 1);
 	req->device = btd_device_ref(device);
 	adapter_get_address(adapter, &src);
-- 
1.7.5.4

Re: [PATCH BlueZ 0/4] GATT over LE reverse service discovery fixes

[Johan Hedberg]

Hi Lizardo,

On Tue, Feb 14, 2012, Anderson Lizardo wrote:
> This series fixes issues when doing reverse GATT service discovery over LE.
> 
> There is still one open issue: If BlueZ creates a device without bonding (i.e.
> CreateDevice() D-Bus method), but the remote requests SMP pairing with
> "Security Request", at the end of the pairing a reverse GATT service discovery
> is issued, *regardless* of BlueZ being the initiator or not. BlueZ should only
> do reverse discovery if it is the acceptor (see comments on the
> device_bonding_complete() function on src/device.c).
> 
> To fix this, I think we need to have a more robust way to check whether we are
> initiator or acceptor, instead of simply checking for an active bonding
> request. Currently, if BlueZ receives keys from kernel over mgmt,
> device_bonding_complete() is eventually called, and if there is no active
> bonding, BlueZ assumes to be an acceptor.
> 
> In any case, with these patches we make sure this spurious service discovery
> will not crash BlueZ or create leaks. It is also applicable for "normal"
> reverse service discovery as well.

All four patches have been applied. Thanks.

Johan