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

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

[Sebastian Urban]

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 | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 970c35f94..e7155f16a 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1700,20 +1700,34 @@ 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;
+	}
+
 	put_le16(handle, data->pdu + data->offset);
 	data->offset += 2;
-
-	length = MIN(data->len - data->offset, length);
-
 	if (multiple) {
 		put_le16(length, data->pdu + data->offset);
 		data->offset += 2;
-- 
2.25.1

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2973 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=498667

---Test result---

Test Summary:
CheckPatch                    FAIL      0.31 seconds
GitLint                       PASS      0.11 seconds
Prep - Setup ELL              PASS      44.15 seconds
Build - Prep                  PASS      0.10 seconds
Build - Configure             PASS      7.14 seconds
Build - Make                  PASS      189.01 seconds
Make Check                    PASS      9.48 seconds
Make Distcheck                PASS      207.31 seconds
Build w/ext ELL - Configure   PASS      6.78 seconds
Build w/ext ELL - Make        PASS      157.09 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
gatt-server: Flush notify multiple buffer when full and fix overflow
WARNING:LONG_LINE_COMMENT: line length of 81 exceeds 80 columns
#28: FILE: src/shared/gatt-server.c:1706:
+		/* Flush buffered data, if this request hits buffer size limit */

WARNING:LONG_LINE: line length of 88 exceeds 80 columns
#29: FILE: src/shared/gatt-server.c:1707:
+		if (data && data->offset > 0 && data->len - data->offset < 4 + length) {

- total: 0 errors, 2 warnings, 38 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] gatt-server: Flush notify multiple buffer when full and fix" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - PASS
Desc: Build the BlueZ source tree

##############################
Test: Make Check - PASS
Desc: Run 'make check'

##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - PASS
Desc: Build BlueZ source with '--enable-external-ell' configuration



---
Regards,
Linux Bluetooth

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

[Sebastian Urban]

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;
+	}
+
 	put_le16(handle, data->pdu + data->offset);
 	data->offset += 2;
-
-	length = MIN(data->len - data->offset, length);
-
 	if (multiple) {
 		put_le16(length, data->pdu + data->offset);
 		data->offset += 2;
-- 
2.25.1

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

[Luiz Augusto von Dentz]

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

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

[Sebastian Urban]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Saturday, June 12, 2021 12:13 AM
> To: Sebastian Urban <surban@surban.net>
> 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 <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:

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

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

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

[Sebastian Urban]

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 | 43 +++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 970c35f94..5dbe9d66c 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1690,6 +1690,17 @@ static bool notify_multiple(void *user_data)
 	return false;
 }
 
+bool notify_append_le16(struct nfy_mult_data *data, uint16_t value)
+{
+	if (data->offset + sizeof(value) > data->len)
+		return false;
+
+	put_le16(value, data->pdu + data->offset);
+	data->offset += sizeof(value);
+
+	return true;
+}
+
 bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
 					uint16_t handle, const uint8_t *value,
 					uint16_t length, bool multiple)
@@ -1700,23 +1711,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 has been freed by notify_multiple */
+			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);
 	}
 
-	put_le16(handle, data->pdu + data->offset);
-	data->offset += 2;
-
-	length = MIN(data->len - data->offset, length);
+	if (!notify_append_le16(data, handle))
+		goto error;
 
 	if (multiple) {
-		put_le16(length, data->pdu + data->offset);
-		data->offset += 2;
+		length = MIN(data->len - data->offset - 2, length);
+		if (!notify_append_le16(data, length))
+			goto error;
+	} else {
+		length = MIN(data->len - data->offset, length);
 	}
 
 	memcpy(data->pdu + data->offset, value, length);
@@ -1740,6 +1763,12 @@ bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
 	free(data);
 
 	return result;
+
+error:
+	if (data)
+		free(data);
+
+	return false;
 }
 
 struct ind_data {
-- 
2.25.1

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3111 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=499291

---Test result---

Test Summary:
CheckPatch                    PASS      0.52 seconds
GitLint                       PASS      0.12 seconds
Prep - Setup ELL              PASS      47.89 seconds
Build - Prep                  PASS      0.15 seconds
Build - Configure             PASS      8.11 seconds
Build - Make                  FAIL      10.16 seconds
Make Check                    FAIL      0.57 seconds
Make Distcheck                PASS      243.02 seconds
Build w/ext ELL - Configure   PASS      8.14 seconds
Build w/ext ELL - Make        FAIL      9.66 seconds

Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf

##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
src/shared/gatt-server.c:1693:6: error: no previous declaration for ‘notify_append_le16’ [-Werror=missing-declarations]
 1693 | bool notify_append_le16(struct nfy_mult_data *data, uint16_t value)
      |      ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:6955: src/shared/gatt-server.lo] Error 1
make: *** [Makefile:4134: all] Error 2


##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
src/shared/gatt-server.c:1693:6: error: no previous declaration for ‘notify_append_le16’ [-Werror=missing-declarations]
 1693 | bool notify_append_le16(struct nfy_mult_data *data, uint16_t value)
      |      ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:6955: src/shared/gatt-server.lo] Error 1
make: *** [Makefile:10406: check] Error 2


##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
src/shared/gatt-server.c:1693:6: error: no previous declaration for ‘notify_append_le16’ [-Werror=missing-declarations]
 1693 | bool notify_append_le16(struct nfy_mult_data *data, uint16_t value)
      |      ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:6955: src/shared/gatt-server.lo] Error 1
make: *** [Makefile:4134: all] Error 2




---
Regards,
Linux Bluetooth

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

[Sebastian Urban]

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 | 43 +++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 970c35f94..dc4e681c9 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1690,6 +1690,17 @@ static bool notify_multiple(void *user_data)
 	return false;
 }
 
+static bool notify_append_le16(struct nfy_mult_data *data, uint16_t value)
+{
+	if (data->offset + sizeof(value) > data->len)
+		return false;
+
+	put_le16(value, data->pdu + data->offset);
+	data->offset += sizeof(value);
+
+	return true;
+}
+
 bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
 					uint16_t handle, const uint8_t *value,
 					uint16_t length, bool multiple)
@@ -1700,23 +1711,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 has been freed by notify_multiple */
+			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);
 	}
 
-	put_le16(handle, data->pdu + data->offset);
-	data->offset += 2;
-
-	length = MIN(data->len - data->offset, length);
+	if (!notify_append_le16(data, handle))
+		goto error;
 
 	if (multiple) {
-		put_le16(length, data->pdu + data->offset);
-		data->offset += 2;
+		length = MIN(data->len - data->offset - 2, length);
+		if (!notify_append_le16(data, length))
+			goto error;
+	} else {
+		length = MIN(data->len - data->offset, length);
 	}
 
 	memcpy(data->pdu + data->offset, value, length);
@@ -1740,6 +1763,12 @@ bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
 	free(data);
 
 	return result;
+
+error:
+	if (data)
+		free(data);
+
+	return false;
 }
 
 struct ind_data {
-- 
2.25.1