shared/gatt-db: Fix munmap_chunk invalid pointer
PATCH 1/1 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.
--- src/shared/gatt-db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
src/shared/gatt-db.c +1 -1
@@ -58,7 +58,7 @@ struct gatt_db {struct bt_crypto *crypto;uint8_t hash[16 ];unsigned int hash_id; -uint16_t next_handle; +uint32_t next_handle;struct queue *services;struct queue *notify_list;
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=814034
---Test result---
Test Summary:
CheckPatch PASS 0.48 seconds GitLint PASS 0.33 seconds BuildEll PASS 24.35 seconds BluezMake PASS 736.35 seconds MakeCheck PASS 12.28 seconds MakeDistcheck PASS 161.49 seconds CheckValgrind PASS 223.98 seconds CheckSmatch PASS 328.92 seconds bluezmakeextell PASS 107.46 seconds IncrementalBuild PASS 688.48 seconds ScanBuild PASS 934.60 seconds
---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz
maintainerHi Frédéric,
Quoted 3 lines · Frédéric Danis
PTS test GATT/CL/GAD/BV-03-C published a service starting at handle
0xfffdand ending at0xffff.
Don't we have a test for it under unit/test-gatt.c? Perhaps it would be a good idea to add one while doing this change.
Quoted 4 lines
This resets the
next_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.
I understand we don't want to loop around but handle 0x10000 is not valid either.
Quoted 9 lines
--- src/shared/gatt-db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)src/shared/gatt-db.c +0
-0
src/shared/gatt-db.c struct gatt_db { · line 61
Quoted 5 lines
src/shared/gatt-db.c +1
-1@@ -58,7 +58,7 @@ struct gatt_db {struct bt_crypto *crypto;uint8_t hash[16 ];unsigned int hash_id; -uint16_t next_handle; +uint32_t next_handle;
I wonder if we can just set the next_handle to 0 and then check it when using it, that way it indicates that it had looped around and handle 0 is invalid already so we shouldn't allocate anything on it.
Quoted 7 lines
src/shared/gatt-db.c +0
-1struct queue *services;struct queue *notify_list; --2.34.1
Signature · 1 line
Luiz Augusto von Dentz
Hi Luiz,
Quoted 8 lines · Luiz Augusto von Dentz
Hi Frédéric,
Quoted 2 lines · Frédéric Danis
PTS test GATT/CL/GAD/BV-03-C published a service starting at handle
0xfffdand ending at0xffff.Don't we have a test for it under
unit/test-gatt.c? Perhaps it would be a good idea to add one while doing this change.
Yes
My idea should be to add a new unordered database and run gatt_db_get_hash() on it.
Quoted 7 lines
Quoted 5 lines
This resets the
next_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.I understand we don't want to loop around but handle
0x10000is not valid either.
Afaiu the next_handle is used as:
- the next available handle, with special value 0 to define an empty db
- and the maximum size to allocate during db_hash_update()
So, 0x10000 is not a valid handle but is a valid size.
gatt_db_insert_service() is already protected to not use handle > 0xFFFF.
Quoted 5 lines
Quoted 5 lines
--- src/shared/gatt-db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 13 lines
src/shared/gatt-db.c struct gatt_db { · line 61
Quoted 10 lines
src/shared/gatt-db.c +1
-1@@ -58,7 +58,7 @@ struct gatt_db {struct bt_crypto *crypto;uint8_t hash[16 ];unsigned int hash_id; -uint16_t next_handle; +uint32_t next_handle;I wonder if we can just set the
next_handleto 0 and then check it when using it, that way it indicates that it had looped around and handle 0 is invalid already so we shouldn't allocate anything on it.
Not sure this can work as 0 can mean it's an empty db or a db requesting UINT16_MAX+1 elements.
During this test case, it loops to 0, but as other services are added setting next_handle to another value, ending up to allocate less memory than expected (i.e. UINT16_MAX+1 elements).
We may replace the next_handle by last_handle, use gatt_db_isempty() instead of the handle 0 to check for empty db, and allocate last_handle+1 in db_hash_update(). Does it seems better?
Regards,
Fred
Signature · 6 lines
Frédéric Danis Senior Software Engineer Collabora Ltd. Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom Registered in England & Wales, no. 5513718