Hi Bastien,
On Fri, Jul 5, 2024 at 5:00 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote:
>
> Error: INTEGER_OVERFLOW (CWE-190): [#def25] [important]
> bluez-5.76/src/shared/gatt-server.c:927:2: cast_overflow: Truncation due to cast operation on "((unsigned int)mtu - 1U < len) ? (unsigned int)mtu - 1U : len" from 32 to 16 bits.
> bluez-5.76/src/shared/gatt-server.c:927:2: overflow_sink: "((unsigned int)mtu - 1U < len) ? (unsigned int)mtu - 1U : len", which might have overflowed, is passed to "bt_att_chan_send(op->chan, rsp_opcode, (len ? value : NULL), (((unsigned int)mtu - 1U < len) ? (unsigned int)mtu - 1U : len), NULL, NULL, NULL)".
> 925| rsp_opcode = get_read_rsp_opcode(op->opcode);
> 926|
> 927|-> bt_att_chan_send_rsp(op->chan, rsp_opcode, len ? value : NULL,
> 928| MIN((unsigned int) mtu - 1, len));
> 929| async_read_op_destroy(op);
> ---
> src/shared/gatt-server.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 66e370d1fe3d..e0e1776779cd 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -908,7 +908,7 @@ static void read_complete_cb(struct gatt_db_attribute *attr, int err,
> struct async_read_op *op = user_data;
> struct bt_gatt_server *server = op->server;
> uint8_t rsp_opcode;
> - uint16_t mtu;
> + size_t mtu;
> uint16_t handle;
>
> DBG(server, "Read Complete: err %d", err);
> @@ -916,7 +916,7 @@ static void read_complete_cb(struct gatt_db_attribute *attr, int err,
> mtu = bt_att_get_mtu(server->att);
> handle = gatt_db_attribute_get_handle(attr);
>
> - if (err) {
> + if (err || mtu <= 1) {
Also pointless here if the mtu is 0 then we should not attempt to send anything.
> bt_att_chan_send_error_rsp(op->chan, op->opcode, handle, err);
> async_read_op_destroy(op);
> return;
> @@ -925,7 +925,7 @@ static void read_complete_cb(struct gatt_db_attribute *attr, int err,
> rsp_opcode = get_read_rsp_opcode(op->opcode);
>
> bt_att_chan_send_rsp(op->chan, rsp_opcode, len ? value : NULL,
> - MIN((unsigned int) mtu - 1, len));
> + MIN(mtu - 1, len));
And this is incorrect as well, we need the mtu of the channel here not
bt_att_get_mtu.
> async_read_op_destroy(op);
> }
>
> --
> 2.45.2
>
>
--
Luiz Augusto von Dentz
