Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230323103835.571037-1-simon.mikuda@streamunlimited.com> <20230323103835.571037-2-simon.mikuda@streamunlimited.com>
In-Reply-To: <20230323103835.571037-2-simon.mikuda@streamunlimited.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 23 Mar 2023 13:59:42 -0700
Message-ID: <CABBYNZKMXnaPydz5pz2Uasg9MeMhBo5aUW5kRRr14gkte_Zvsw@mail.gmail.com>
Subject: Re: [PATCH BlueZ 1/9] gatt-db: Fix crash during calculating hash from
 ATT handles
To:     Simon Mikuda <simon.mikuda@streamunlimited.com>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Simon,

On Thu, Mar 23, 2023 at 3:44=E2=80=AFAM Simon Mikuda
<simon.mikuda@streamunlimited.com> wrote:
>
> It happens when next_handle is lower that discovered number of handles.
> Found by PTS test case: GATT/CL/GAD/BC-01-C

Can you add the backtrace of the crash?

> ---
>  src/shared/gatt-db.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index b696fe33d..c9ffbfeed 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -297,6 +297,7 @@ static void handle_notify(void *data, void *user_data=
)
>  struct hash_data {
>         struct iovec *iov;
>         uint16_t i;
> +       size_t size;
>  };
>
>  static void gen_hash_m(struct gatt_db_attribute *attr, void *user_data)
> @@ -327,7 +328,7 @@ static void gen_hash_m(struct gatt_db_attribute *attr=
, void *user_data)
>         case GATT_CHARAC_AGREG_FMT_UUID:
>                 /* Allocate space for handle + type  */
>                 len =3D 2 + 2;
> -               data =3D malloc(2 + 2 + attr->value_len);
> +               data =3D malloc(2 + 2);

This seems to be a different issue, looks like we are allocating more
than necessary.

>                 put_le16(attr->handle, data);
>                 bt_uuid_to_le(&attr->uuid, data + 2);
>                 break;
> @@ -335,6 +336,13 @@ static void gen_hash_m(struct gatt_db_attribute *att=
r, void *user_data)
>                 return;
>         }
>
> +       if (hash->i >=3D hash->size) {
> +               /* double the size of iov if we've run out of space */
> +               hash->iov =3D realloc(hash->iov, 2 * hash->size * sizeof(=
struct iovec));
> +               memset(hash->iov + hash->size, 0, hash->size * sizeof(str=
uct iovec));
> +               hash->size *=3D 2;

Not sure if we should double the size? I'd probably check why we are
not able to allocate the size properly, perhaps we have an off by one
of the next_handle happens to loop around? A better way would be to
just calculate the actual number of attributes instead of using its
handles since there could be spaces in between handles, we could just
iterate over the services since they should each contain the number of
attributes.

> +       }
> +
>         hash->iov[hash->i].iov_base =3D data;
>         hash->iov[hash->i].iov_len =3D len;
>
> @@ -361,9 +369,10 @@ static bool db_hash_update(void *user_data)
>
>         hash.iov =3D new0(struct iovec, db->next_handle);
>         hash.i =3D 0;
> +       hash.size =3D db->next_handle;
>
>         gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);
> -       bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->ha=
sh);
> +       bt_crypto_gatt_hash(db->crypto, hash.iov, hash.i, db->hash);
>
>         for (i =3D 0; i < hash.i; i++)
>                 free(hash.iov[i].iov_base);
> --
> 2.34.1
>


--=20
Luiz Augusto von Dentz