| Message ID | 20210611122915.21068-1-surban@surban.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ] gatt-server: Flush notify multiple buffer when full and fix overflow | expand |
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=498857 ---Test result--- Test Summary: CheckPatch PASS 0.64 seconds GitLint PASS 0.13 seconds Prep - Setup ELL PASS 46.67 seconds Build - Prep PASS 0.11 seconds Build - Configure PASS 8.18 seconds Build - Make PASS 202.14 seconds Make Check PASS 9.49 seconds Make Distcheck PASS 237.27 seconds Build w/ext ELL - Configure PASS 8.12 seconds Build w/ext ELL - Make PASS 188.19 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
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 >
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
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;