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(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 676f963ec..d32c9a70f 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;
+ uint32_t next_handle;
struct queue *services;
struct queue *notify_list;
--
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
Hi Frédéric,
On Wed, Jan 3, 2024 at 4:28 AM Frédéric Danis
<[email protected]> wrote:
>
> PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
> and ending at 0xffff.
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.
> 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.
I understand we don't want to loop around but handle 0x10000 is not
valid either.
> ---
> src/shared/gatt-db.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 676f963ec..d32c9a70f 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;
> + 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.
> struct queue *services;
>
> struct queue *notify_list;
> --
> 2.34.1
>
>
--
Luiz Augusto von Dentz
Hi Luiz,
On 03/01/2024 16:50, Luiz Augusto von Dentz wrote:
> Hi Frédéric,
>
> On Wed, Jan 3, 2024 at 4:28 AM Frédéric Danis
> <[email protected]> wrote:
>> PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
>> and ending at 0xffff.
> 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.
>
>> 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.
> I understand we don't want to loop around but handle 0x10000 is 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.
>
>> ---
>> src/shared/gatt-db.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 676f963ec..d32c9a70f 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;
>> + 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.
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
--
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