Skip to contentSkip to conversation

client: Fix uninitialized read in attribute handle

PATCH In review Alicia Boya Garcia 3 participants 3 messages Aug 8, 2022 Bluetooth drivers

lore
Share & tools
lore

Alicia Boya Garcia

author
#0 lore raw

PATCH 1/1 client: Fix uninitialized read in attribute handle

When services, characteristics and descriptors were parsed from DBus proxies the client code was calling the print code without initializing the handle field, which the print functions use.

This resulted in semi-random or zero handles in all attributes when using gatt.list-attributes in bluetoothctl, depending on compilation flags.

This patch fixes the problem by parsing the handle from the DBus proxy path.

---
 client/gatt.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
client/gatt.c +13 -0
index 4c1efaf75..aaad786b2 100644
@@ -158,6 +158,16 @@ static void print_inc_service(struct service *service, const char *description)
 					service->uuid, text);
 }
 
+static uint16_t handle_from_path(const char *path)
+{
+	const char *number = path + strlen(path) - 4;
+
+	if (number < path)
+		return 0;
+
+	return (uint16_t) strtol(number, NULL, 16);
+}
+
 static void print_service_proxy(GDBusProxy *proxy, const char *description)
 {
 	struct service service;

@@ -179,6 +189,7 @@ static void print_service_proxy(GDBusProxy *proxy, const char *description)
 	service.path = (char *) g_dbus_proxy_get_path(proxy);
 	service.uuid = (char *) uuid;
 	service.primary = primary;
+	service.handle = handle_from_path(service.path);
 
 	print_service(&service, description);
 }

@@ -261,6 +272,7 @@ static void print_characteristic(GDBusProxy *proxy, const char *description)
 	memset(&chrc, 0, sizeof(chrc));
 	chrc.path = (char *) g_dbus_proxy_get_path(proxy);
 	chrc.uuid = (char *) uuid;
+	chrc.handle = handle_from_path(chrc.path);
 
 	print_chrc(&chrc, description);
 }

@@ -355,6 +367,7 @@ static void print_descriptor(GDBusProxy *proxy, const char *description)
 	memset(&desc, 0, sizeof(desc));
 	desc.path = (char *) g_dbus_proxy_get_path(proxy);
 	desc.uuid = (char *) uuid;
+	desc.handle = handle_from_path(desc.path);
 
 	print_desc(&desc, description);
 }
Signature · 1 line
2.37.1
#1 lore raw

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=666005

---Test result---

Test Summary:

CheckPatch                    PASS      2.06 seconds
GitLint                       PASS      1.07 seconds
Prep - Setup ELL              PASS      29.52 seconds
Build - Prep                  PASS      0.83 seconds
Build - Configure             PASS      8.97 seconds
Build - Make                  PASS      879.44 seconds
Make Check                    PASS      12.32 seconds
Make Check w/Valgrind         PASS      288.51 seconds
Make Distcheck                PASS      241.98 seconds
Build w/ext ELL - Configure   PASS      8.85 seconds
Build w/ext ELL - Make        PASS      82.41 seconds
Incremental Build w/ patches  PASS      0.00 seconds
Scan Build                    PASS      497.89 seconds

---
Regards,
Linux Bluetooth

maintainer
#2 lore raw
In reply to Alicia Boya Garcia

Hi Alicia,

Quoted 22 lines · Alicia Boya Garcia

When services, characteristics and descriptors were parsed from DBus proxies the client code was calling the print code without initializing the handle field, which the print functions use.

This resulted in semi-random or zero handles in all attributes when using gatt.list-attributes in bluetoothctl, depending on compilation flags.

This patch fixes the problem by parsing the handle from the DBus proxy path.

---
 client/gatt.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
client/gatt.c +0 -0
index 4c1efaf75..aaad786b2 100644
@@ -158,6 +158,16 @@ static void print_inc_service(struct service *service, const char *description)
                                        service->uuid, text);
 }

client/gatt.c static void print_inc_service(struct service *service, const char *description) · line 169

Quoted 10 lines
client/gatt.c +9 -0
 
+static uint16_t handle_from_path(const char *path)
+{
+       const char *number = path + strlen(path) - 4;
+
+       if (number < path)
+               return 0;
+
+       return (uint16_t) strtol(number, NULL, 16);
+}

Object paths are subject to change so we shouldn't consider them as APIs, so what we should aim to do if we really want to expose handles of remote attributes is to have it as a D-Bus property Handle like we do for the servers:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt#n53

Quoted 30 lines
client/gatt.c +3 -1
 static void print_service_proxy(GDBusProxy *proxy, const char *description)
 {
        struct service service;

@@ -179,6 +189,7 @@ static void print_service_proxy(GDBusProxy *proxy, const char *description)
        service.path = (char *) g_dbus_proxy_get_path(proxy);
        service.uuid = (char *) uuid;
        service.primary = primary;
+       service.handle = handle_from_path(service.path);
 
        print_service(&service, description);
 }

@@ -261,6 +272,7 @@ static void print_characteristic(GDBusProxy *proxy, const char *description)
        memset(&chrc, 0, sizeof(chrc));
        chrc.path = (char *) g_dbus_proxy_get_path(proxy);
        chrc.uuid = (char *) uuid;
+       chrc.handle = handle_from_path(chrc.path);
 
        print_chrc(&chrc, description);
 }

@@ -355,6 +367,7 @@ static void print_descriptor(GDBusProxy *proxy, const char *description)
        memset(&desc, 0, sizeof(desc));
        desc.path = (char *) g_dbus_proxy_get_path(proxy);
        desc.uuid = (char *) uuid;
+       desc.handle = handle_from_path(desc.path);
 
        print_desc(&desc, description);
 }
--

2.37.1

Signature · 1 line
Luiz Augusto von Dentz