2015-01-23 14:25:15

by Romain Izard

Subject: [PATCH 1/2] shared/gatt-server: Avoid memory corruption

When sending notification and indication data, the size of the allocated
packet is the smallest of the MTU and the payload size.

The copy procedure uses the payload size in all cases, which can lead to
memory corruption. Use the packet size instead.
---
src/shared/gatt-server.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index b406ed6..dd9c88f 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1506,7 +1506,7 @@ bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
return false;

put_le16(handle, pdu);
- memcpy(pdu + 2, value, length);
+ memcpy(pdu + 2, value, pdu_len - 2);

result = !!bt_att_send(server->att, BT_ATT_OP_HANDLE_VAL_NOT, pdu,
pdu_len, NULL, NULL, NULL);
@@ -1571,7 +1571,7 @@ bool bt_gatt_server_send_indication(struct bt_gatt_server *server,
data->user_data = user_data;

put_le16(handle, pdu);
- memcpy(pdu + 2, value, length);
+ memcpy(pdu + 2, value, pdu_len - 2);

result = !!bt_att_send(server->att, BT_ATT_OP_HANDLE_VAL_IND, pdu,
pdu_len, conf_cb,
--
2.1.0

2015-01-26 15:53:31

by Luiz Augusto von Dentz

Subject: Re: [PATCH 1/2] shared/gatt-server: Avoid memory corruption

Hi Romain,

On Fri, Jan 23, 2015 at 4:25 PM, Romain Izard
<[email protected]> wrote:
> When sending notification and indication data, the size of the allocated
> packet is the smallest of the MTU and the payload size.
>
> The copy procedure uses the payload size in all cases, which can lead to
> memory corruption. Use the packet size instead.
> ---
> src/shared/gatt-server.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index b406ed6..dd9c88f 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -1506,7 +1506,7 @@ bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
> return false;
>
> put_le16(handle, pdu);
> - memcpy(pdu + 2, value, length);
> + memcpy(pdu + 2, value, pdu_len - 2);
>
> result = !!bt_att_send(server->att, BT_ATT_OP_HANDLE_VAL_NOT, pdu,
> pdu_len, NULL, NULL, NULL);
> @@ -1571,7 +1571,7 @@ bool bt_gatt_server_send_indication(struct bt_gatt_server *server,
> data->user_data = user_data;
>
> put_le16(handle, pdu);
> - memcpy(pdu + 2, value, length);
> + memcpy(pdu + 2, value, pdu_len - 2);
>
> result = !!bt_att_send(server->att, BT_ATT_OP_HANDLE_VAL_IND, pdu,
> pdu_len, conf_cb,
> --
> 2.1.0

Applied, thanks.


--
Luiz Augusto von Dentz

2015-01-23 14:25:16

by Romain Izard

Subject: [PATCH 2/2] shared/gatt-server: Fix notification & indication packet size

For both Handle Value Notification & Handle Value Indication, the size
of the payload is limited to (ATT_MTU - 3) bytes, as indicated in Vol 3,
part F, section 3.4.7.1 & 3.4.7.2 of the Bluetooth 4.2 specification.

In the function, the first reserved byte is implicit and corresponds to
the opcode, while the 2 next bytes are explicit and contain the
attribute handle.

Ensure that the truncation of large payloads is done with the correct
value.
---
src/shared/gatt-server.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index dd9c88f..c7974fb 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1500,7 +1500,7 @@ bool bt_gatt_server_send_notification(struct bt_gatt_server *server,
if (!server || (length && !value))
return false;

- pdu_len = MIN(bt_att_get_mtu(server->att), length + 2);
+ pdu_len = MIN(bt_att_get_mtu(server->att) - 1, length + 2);
pdu = malloc(pdu_len);
if (!pdu)
return false;
@@ -1555,7 +1555,7 @@ bool bt_gatt_server_send_indication(struct bt_gatt_server *server,
if (!server || (length && !value))
return false;

- pdu_len = MIN(bt_att_get_mtu(server->att), length + 2);
+ pdu_len = MIN(bt_att_get_mtu(server->att) - 1, length + 2);
pdu = malloc(pdu_len);
if (!pdu)
return false;
--
2.1.0