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

2021-06-11 12:31:35

by Sebastian Urban

[permalink] [raw]

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

2021-06-11 12:55:30

by bluez.test.bot

[permalink] [raw]

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

2021-06-11 22:18:01

by Luiz Augusto von Dentz

[permalink] [raw]

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 <[email protected]> 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

2021-06-12 09:32:11

by Sebastian Urban

[permalink] [raw]

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

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <[email protected]>
> Sent: Saturday, June 12, 2021 12:13 AM
> To: Sebastian Urban <[email protected]>
> Cc: [email protected]
> 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 <[email protected]> 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

2021-06-12 09:34:32

by Sebastian Urban

[permalink] [raw]

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

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

2021-06-12 09:59:12

by bluez.test.bot

[permalink] [raw]

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

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

2021-06-13 10:41:48

by Sebastian Urban

[permalink] [raw]

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

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

2021-06-13 11:06:56

by bluez.test.bot

[permalink] [raw]

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

2021-06-14 21:08:25

by Luiz Augusto von Dentz

[permalink] [raw]

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

Hi Sebastian,

On Sun, Jun 13, 2021 at 4:06 AM <[email protected]> wrote:
>
> 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=499527
>
> ---Test result---
>
> Test Summary:
> CheckPatch PASS 0.64 seconds
> GitLint PASS 0.15 seconds
> Prep - Setup ELL PASS 51.54 seconds
> Build - Prep PASS 0.15 seconds
> Build - Configure PASS 9.04 seconds
> Build - Make PASS 228.99 seconds
> Make Check PASS 9.32 seconds
> Make Distcheck PASS 267.21 seconds
> Build w/ext ELL - Configure PASS 9.24 seconds
> Build w/ext ELL - Make PASS 213.85 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 - 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

Applied, thanks.

--
Luiz Augusto von Dentz