Fix gatt-db munmap_chunk invalid pointer
v4 cover Fix gatt-db munmap_chunk invalid pointer
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.
Add a unit test to check regression.
v1 -> v2: Replace next_handle by last_handle
Check empty db using gatt_db_isempty(db) instead of
next_handle == 0
Add robustness unit test to check that gatt_db_get_hash()
doesn't crash
v2 -> v3: Fix line length checkpatch errors
v3 -> v4: Update commit comment to explain reason for keeping last_handle
Split unit test to its own commit
Rephrase db setup comment in unit test
Frédéric Danis (2):
shared/gatt-db: Fix munmap_chunk invalid pointer unit/test-gatt: Add unordered setup db test
src/shared/gatt-db.c | 19 ++++++------ unit/test-gatt.c | 73 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 10 deletions(-)
Signature · 1 line
2.34.1
PATCH v4 1/2 shared/gatt-db: Fix munmap_chunk invalid pointer
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(-)
src/shared/gatt-db.c +10 -9
@@ -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 ];
Signature · 1 line
2.34.1
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=816181
---Test result---
Test Summary:
CheckPatch PASS 1.10 seconds GitLint PASS 0.83 seconds BuildEll PASS 23.87 seconds BluezMake PASS 727.83 seconds MakeCheck PASS 11.38 seconds MakeDistcheck PASS 159.36 seconds CheckValgrind PASS 221.84 seconds CheckSmatch PASS 327.53 seconds bluezmakeextell PASS 105.96 seconds IncrementalBuild PASS 1352.37 seconds ScanBuild PASS 930.29 seconds
---
Regards,
Linux Bluetooth
PATCH v4 2/2 unit/test-gatt: Add unordered setup db test
In reply to Frédéric DanisVerify 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(-)
unit/test-gatt.c +72 -1
@@ -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 (); }
Signature · 1 line
2.34.1
This series was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <[email protected]>:
Hello:
This series was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <[email protected]>:
Quoted 8 lines · On Thu, 11 Jan 2024 14:39:53 +0100 you wrote:
PTS test GATT/CL/GAD/BV-03-C published a service starting at handle
0xfffdand ending at0xffff.
This resets thenext_handleto 0 ingatt_db_insert_service()instead of setting it to0x10000. Other services are added later.
This could end-up by a crash indb_hash_update()if not enough space has been allocated for hash.iov and some entries are overwritten.[...]
Here is the summary with links:
- [v4,1/2] shared/gatt-db: Fix munmap_chunk invalid pointer
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=dacc69373263
- [v4,2/2] unit/test-gatt: Add unordered setup db test (no matching commit)
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html