[PATCH BlueZ] shared/gatt-db: Fix munmap_chunk invalid pointer

[PATCH BlueZ] shared/gatt-db: Fix munmap_chunk invalid pointer

[Frédéric Danis]

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

RE: [BlueZ] shared/gatt-db: Fix munmap_chunk invalid pointer

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

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

Re: [PATCH BlueZ] shared/gatt-db: Fix munmap_chunk invalid pointer

[Luiz Augusto von Dentz]

Hi Frédéric,

On Wed, Jan 3, 2024 at 4:28 AM Frédéric Danis
<frederic.danis@collabora.com> 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

Re: [PATCH BlueZ] shared/gatt-db: Fix munmap_chunk invalid pointer

[Frédéric Danis]

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
> <frederic.danis@collabora.com> 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