Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-860-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240103092816.22952-1-frederic.danis@collabora.com>
In-Reply-To: <20240103092816.22952-1-frederic.danis@collabora.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 3 Jan 2024 10:50:31 -0500
Message-ID: <CABBYNZJ-pPU=3jFWK8dDWrqPMG_8Uzow5XTxEP7mKnV7OhdajA@mail.gmail.com>
Subject: Re: [PATCH BlueZ] shared/gatt-db: Fix munmap_chunk invalid pointer
To: =?UTF-8?B?RnLDqWTDqXJpYyBEYW5pcw==?= <frederic.danis@collabora.com>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Fr=C3=A9d=C3=A9ric,

On Wed, Jan 3, 2024 at 4:28=E2=80=AFAM Fr=C3=A9d=C3=A9ric Danis
<frederic.danis@collabora.com> wrote:
>
> PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfff=
d
> 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
>
>


--=20
Luiz Augusto von Dentz