Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210611122915.21068-1-surban@surban.net>
In-Reply-To: <20210611122915.21068-1-surban@surban.net>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 11 Jun 2021 15:13:05 -0700
Message-ID: <CABBYNZJBbiBvw+r1XLxE5=uXVv3cXe8EpCd3YOF7ZQVmDGxyBg@mail.gmail.com>
Subject: Re: [PATCH BlueZ] gatt-server: Flush notify multiple buffer when full
 and fix overflow
To:     Sebastian Urban <surban@surban.net>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Sebastian,

On Fri, Jun 11, 2021 at 5:31 AM Sebastian Urban <surban@surban.net> wrote:
>
> This fixes the calculation of available buffer space in
> bt_gatt_server_send_notification and sends pending notifications
> immediately when there is no more room to add a notification.
>
> Previously there was a buffer overflow caused by incorrect calculation
> of available buffer space: data->offset can equal data->len
> from a previous call to this function, leading
> (data->len - data->offset) to underflow after data->offset += 2.
> ---
>  src/shared/gatt-server.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 970c35f94..d53efe782 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -1700,20 +1700,35 @@ bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
>         if (!server || (length && !value))
>                 return false;
>
> -       if (multiple)
> +       if (multiple) {
>                 data = server->nfy_mult;
>
> +               /* flush buffered data if this request hits buffer size limit */
> +               if (data && data->offset > 0 &&
> +                               data->len - data->offset < 4 + length) {
> +                       if (server->nfy_mult->id)
> +                               timeout_remove(server->nfy_mult->id);
> +                       notify_multiple(server);
> +                       data = NULL;
> +               }
> +       }
> +
>         if (!data) {
>                 data = new0(struct nfy_mult_data, 1);
>                 data->len = bt_att_get_mtu(server->att) - 1;
>                 data->pdu = malloc(data->len);
>         }
>
> +       if (multiple) {
> +               if (data->len - data->offset < 4 + length)
> +                       return false;
> +       } else {
> +               if (data->len - data->offset < 2 + length)
> +                       return false;
> +       }

We are missing free(data) when the code returns above, beside I think
it is better to group the code in something like this:

bool notify_append_le16(struct nfy_mult_data *data, uin16_t value)
{
    if (data->offset + sizeof(value) > data->size)
        return false;

    put_le16(value, data->pdu + data->offset);
    data->offset += 2;

    return true;
}

So this can be called for both adding handle and length, it may also
be cleaner to add a similar function but taking arbitrary length which
will deal with checking if the data fits and memcpy.

> +
>         put_le16(handle, data->pdu + data->offset);
>         data->offset += 2;
> -
> -       length = MIN(data->len - data->offset, length);

This was supposed to truncate the data if it was bigger than MTU, I'm
not sure we shall remove this logic or this was actually intentional?

> -
>         if (multiple) {
>                 put_le16(length, data->pdu + data->offset);
>                 data->offset += 2;
> --
> 2.25.1
>


-- 
Luiz Augusto von Dentz