* [PATCH v4 1/2] shared/gatt-db: Fix munmap_chunk invalid pointer
2024-01-11 13:39 [PATCH v4 0/2] Fix gatt-db munmap_chunk invalid pointer Frédéric Danis
@ 2024-01-11 13:39 ` Frédéric Danis
2024-01-11 15:17 ` Fix gatt-db " bluez.test.bot
2024-01-11 13:39 ` [PATCH v4 2/2] unit/test-gatt: Add unordered setup db test Frédéric Danis
2024-01-12 15:40 ` [PATCH v4 0/2] Fix gatt-db munmap_chunk invalid pointer patchwork-bot+bluetooth
2 siblings, 1 reply; 5+ messages in thread
From: Frédéric Danis @ 2024-01-11 13:39 UTC (permalink / raw)
To: linux-bluetooth
PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
and ending at 0xffff.
This resets the next_handle to 0 in gatt_db_insert_service() instead of
setting it to 0x10000. Other services are added later.
This could end-up by a crash in db_hash_update() if not enough space has
been allocated for hash.iov and some entries are overwritten.
Next_handle can be replaced by a last_handle variable which will not loop
over. This can be replaced by queue_peek_tail() and computing the value,
but keeping last_handle will avoid this sort of lookup.
---
src/shared/gatt-db.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 676f963ec..9559583d1 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -58,7 +58,7 @@ struct gatt_db {
struct bt_crypto *crypto;
uint8_t hash[16];
unsigned int hash_id;
- uint16_t next_handle;
+ uint16_t last_handle;
struct queue *services;
struct queue *notify_list;
@@ -255,7 +255,7 @@ struct gatt_db *gatt_db_new(void)
db->crypto = bt_crypto_new();
db->services = queue_new();
db->notify_list = queue_new();
- db->next_handle = 0x0001;
+ db->last_handle = 0x0000;
return gatt_db_ref(db);
}
@@ -356,14 +356,15 @@ static bool db_hash_update(void *user_data)
db->hash_id = 0;
- if (!db->next_handle)
+ if (gatt_db_isempty(db))
return false;
- hash.iov = new0(struct iovec, db->next_handle);
+ hash.iov = new0(struct iovec, db->last_handle + 1);
hash.i = 0;
gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);
- bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash);
+ bt_crypto_gatt_hash(db->crypto, hash.iov, db->last_handle + 1,
+ db->hash);
for (i = 0; i < hash.i; i++)
free(hash.iov[i].iov_base);
@@ -624,7 +625,7 @@ bool gatt_db_clear_range(struct gatt_db *db, uint16_t start_handle,
done:
if (gatt_db_isempty(db))
- db->next_handle = 0;
+ db->last_handle = 0;
return true;
}
@@ -700,7 +701,7 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
return NULL;
if (!handle)
- handle = db->next_handle;
+ handle = db->last_handle + 1;
if (num_handles < 1 || (handle + num_handles - 1) > UINT16_MAX)
return NULL;
@@ -747,8 +748,8 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
service->attributes[0]->handle = handle;
service->num_handles = num_handles;
- /* Fast-forward next_handle if the new service was added to the end */
- db->next_handle = MAX(handle + num_handles, db->next_handle);
+ /* Fast-forward last_handle if the new service was added to the end */
+ db->last_handle = MAX(handle + num_handles - 1, db->last_handle);
return service->attributes[0];
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v4 2/2] unit/test-gatt: Add unordered setup db test
2024-01-11 13:39 [PATCH v4 0/2] Fix gatt-db munmap_chunk invalid pointer Frédéric Danis
2024-01-11 13:39 ` [PATCH v4 1/2] shared/gatt-db: Fix " Frédéric Danis
@ 2024-01-11 13:39 ` Frédéric Danis
2024-01-12 15:40 ` [PATCH v4 0/2] Fix gatt-db munmap_chunk invalid pointer patchwork-bot+bluetooth
2 siblings, 0 replies; 5+ messages in thread
From: Frédéric Danis @ 2024-01-11 13:39 UTC (permalink / raw)
To: linux-bluetooth
Verify that gatt_db_get_hash() does not crash when services are added
in an unordered way.
---
unit/test-gatt.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 1 deletion(-)
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index f92d860c4..e536cd908 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -1908,6 +1908,60 @@ static struct gatt_db *make_test_spec_large_db_1(void)
return make_db(specs);
}
+/*
+ * Tiny database which fits into a single minimum sized-pdu with services
+ * added in the following order to check ability to create hash db:
+ * - one secondary service at handle 0x0003,
+ * - one primary service at the max handle,
+ * - one primary service at handle 0x0001.
+ */
+
+static struct gatt_db *make_test_unordered_setup_db(void)
+{
+ const struct att_handle_spec specs[] = {
+ SECONDARY_SERVICE(0x0003, DEVICE_INFORMATION_UUID, 16),
+ CHARACTERISTIC_STR(GATT_CHARAC_MANUFACTURER_NAME_STRING,
+ BT_ATT_PERM_READ |
+ BT_ATT_PERM_WRITE,
+ BT_GATT_CHRC_PROP_READ |
+ BT_GATT_CHRC_PROP_NOTIFY |
+ BT_GATT_CHRC_PROP_INDICATE |
+ BT_GATT_CHRC_PROP_EXT_PROP,
+ "BlueZ"),
+ DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ |
+ BT_ATT_PERM_WRITE, 0x00, 0x00),
+ DESCRIPTOR_STR(GATT_CHARAC_USER_DESC_UUID, BT_ATT_PERM_READ,
+ "Manufacturer Name"),
+ DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
+ 0x00),
+ CHARACTERISTIC_STR(GATT_CHARAC_SOFTWARE_REVISION_STRING,
+ BT_ATT_PERM_READ,
+ BT_GATT_CHRC_PROP_READ |
+ BT_GATT_CHRC_PROP_INDICATE,
+ "5.59"),
+ DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ
+ | BT_ATT_PERM_WRITE, 0x00, 0x00),
+
+ PRIMARY_SERVICE(0xFFFF - 9 + 1, GAP_UUID, 9),
+ INCLUDE(0x0003),
+ CHARACTERISTIC_STR(GATT_CHARAC_DEVICE_NAME, BT_ATT_PERM_READ,
+ BT_GATT_CHRC_PROP_READ,
+ "BlueZ Unit Tester"),
+ CHARACTERISTIC(0000B009-0000-0000-0123-456789abcdef,
+ BT_ATT_PERM_READ | BT_ATT_PERM_WRITE,
+ BT_GATT_CHRC_PROP_READ |
+ BT_GATT_CHRC_PROP_EXT_PROP, 0x09),
+ DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
+ 0x00),
+ CHARACTERISTIC(GATT_CHARAC_APPEARANCE, BT_ATT_PERM_READ,
+ BT_GATT_CHRC_PROP_READ, 0x00, 0x00),
+ PRIMARY_SERVICE(0x0001, DEVICE_INFORMATION_UUID, 1),
+ { }
+ };
+
+ return make_db(specs);
+}
+
static void test_client(gconstpointer data)
{
create_context(512, data);
@@ -2345,10 +2399,22 @@ static const struct test_step test_indication_server_1 = {
.length = 0x03,
};
+static void test_hash_db(gconstpointer data)
+{
+ struct context *context = create_context(512, data);
+
+ /* test that gatt_db_get_hash is able to manage unordered db and
+ * doesn't crash
+ */
+ gatt_db_get_hash(context->server_db);
+
+ context_quit(context);
+}
+
int main(int argc, char *argv[])
{
struct gatt_db *service_db_1, *service_db_2, *service_db_3;
- struct gatt_db *ts_small_db, *ts_large_db_1;
+ struct gatt_db *ts_small_db, *ts_large_db_1, *ts_tail_db;
tester_init(&argc, &argv);
@@ -2357,6 +2423,7 @@ int main(int argc, char *argv[])
service_db_3 = make_service_data_3_db();
ts_small_db = make_test_spec_small_db();
ts_large_db_1 = make_test_spec_large_db_1();
+ ts_tail_db = make_test_unordered_setup_db();
/*
* Server Configuration
@@ -4487,5 +4554,9 @@ int main(int argc, char *argv[])
raw_pdu(0xff, 0x00),
raw_pdu());
+ define_test_server("/robustness/hash-db",
+ test_hash_db, ts_tail_db, NULL,
+ {});
+
return tester_run();
}
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread