diff mbox series

[BlueZ,v2,08/11] gatt-server: Fix integer overflow due to cast operation

Message ID 20240705085935.1255725-9-hadess@hadess.net
State New
Headers show
Series Fix a number of static analysis issues #5 | expand

Commit Message

Bastien Nocera July 5, 2024, 8:57 a.m. UTC 
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(-)

Comments

Luiz Augusto von Dentz July 8, 2024, 3:17 p.m. UTC | #1 
Hi Bastien,

On Fri, Jul 5, 2024 at 5:00 AM Bastien Nocera <hadess@hadess.net> 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
>
>
Bastien Nocera July 18, 2024, 12:38 p.m. UTC | #2 
On Mon, 2024-07-08 at 11:17 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Fri, Jul 5, 2024 at 5:00 AM Bastien Nocera <hadess@hadess.net>
> 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.

There's more than one fix in this patch. One is the data type used
(should be a size_t, not a uint16_t), the other is avoid a negative or
zero pdu value. What happens to guard against a "1" mtu? Do we need to
guard against it?

> 
> >                 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.

I see you fixed this in 110a8b47a4f159a8239795255b6c1c0edd79e4cd
Thanks!

> 
> >         async_read_op_destroy(op);
> >  }
> > 
> > --
> > 2.45.2
> > 
> > 
> 
>
diff mbox series

Patch

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) {
 		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));
 	async_read_op_destroy(op);
 }