RE: [PATCH BlueZ] gatt-server: Flush notify multiple buffer when full and fix overflow

Hi Luiz,

-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Sent: Saturday, June 12, 2021 12:13 AM
To: Sebastian Urban <redacted>
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH BlueZ] gatt-server: Flush notify multiple buffer when
full and fix overflow

Hi Sebastian,

On Fri, Jun 11, 2021 at 5:31 AM Sebastian Urban [off-list ref] wrote:

quoted

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,

quoted

        if (!server || (length && !value))
                return false;

-       if (multiple)
+       if (multiple) {
                data = server->nfy_mult;

+               /* flush buffered data if this request hits buffer size

limit */

quoted

+               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:

The free is already performed by notify_multiple. I've added a comment to clarify 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;
}

Changed. 

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.

quoted

+
        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?

To be on the safe side, I've added back the original logic.

quoted

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

--
Luiz Augusto von Dentz