Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Saturday, June 12, 2021 12:13 AM
> To: Sebastian Urban <surban@xxxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx> 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
