| Message ID | 20210611122915.21068-1-surban@surban.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ] gatt-server: Flush notify multiple buffer when full and fix overflow | expand |
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;