diff mbox series

[BlueZ,v2,01/11] gatt-server: Don't allocate negative data

Message ID 20240705085935.1255725-2-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 
Set a lower-bound to the data MTU to avoid allocating -1 elements if
bt_att_get_mtu() returns zero.

Error: OVERRUN (CWE-119): [#def36] [important]
bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function call "bt_att_get_mtu(server->att)" returns 0.
bluez-5.76/src/shared/gatt-server.c:1121:2: assignment: Assigning: "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data->mtu" is now 0.
bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now 18446744073709551615.
bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__s" = "1UL".
bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg: Calling "memset" with "__p" and "__n * __s" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned. [Note: The source code implementation of the function has been overridden by a builtin model.]
1121|		data->mtu = bt_att_get_mtu(server->att);
1122|		data->length = 0;
1123|->		data->rsp_data = new0(uint8_t, data->mtu - 1);
1124|
1125|		return data;
---
 src/shared/gatt-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

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

On Fri, Jul 5, 2024 at 5:00 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> Set a lower-bound to the data MTU to avoid allocating -1 elements if
> bt_att_get_mtu() returns zero.
>
> Error: OVERRUN (CWE-119): [#def36] [important]
> bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function call "bt_att_get_mtu(server->att)" returns 0.
> bluez-5.76/src/shared/gatt-server.c:1121:2: assignment: Assigning: "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data->mtu" is now 0.

We are disconnected or have an invalid bt_att instance if
bt_att_get_mtu returns 0 so it is probably pointless to attempt to
send any response if that is the case.

> bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now 18446744073709551615.
> bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning: "__s" = "1UL".
> bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg: Calling "memset" with "__p" and "__n * __s" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned. [Note: The source code implementation of the function has been overridden by a builtin model.]
> 1121|           data->mtu = bt_att_get_mtu(server->att);
> 1122|           data->length = 0;
> 1123|->         data->rsp_data = new0(uint8_t, data->mtu - 1);
> 1124|
> 1125|           return data;
> ---
>  src/shared/gatt-server.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 3a53d5dfde6b..66e370d1fe3d 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -1120,7 +1120,7 @@ static struct read_mult_data *read_mult_data_new(struct bt_gatt_server *server,
>         data->cur_handle = 0;
>         data->mtu = bt_att_get_mtu(server->att);
>         data->length = 0;
> -       data->rsp_data = new0(uint8_t, data->mtu - 1);
> +       data->rsp_data = new0(uint8_t, MAX(data->mtu, 1) - 1);
>
>         return data;
>  }
> --
> 2.45.2
>
>
Bastien Nocera July 18, 2024, 12:40 p.m. UTC | #2 
On Mon, 2024-07-08 at 10:44 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Fri, Jul 5, 2024 at 5:00 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > Set a lower-bound to the data MTU to avoid allocating -1 elements
> > if
> > bt_att_get_mtu() returns zero.
> > 
> > Error: OVERRUN (CWE-119): [#def36] [important]
> > bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function
> > call "bt_att_get_mtu(server->att)" returns 0.
> > bluez-5.76/src/shared/gatt-server.c:1121:2: assignment: Assigning:
> > "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data-
> > >mtu" is now 0.
> 
> We are disconnected or have an invalid bt_att instance if
> bt_att_get_mtu returns 0 so it is probably pointless to attempt to
> send any response if that is the case.

Same as for patch 8 in this series:
gatt-server: Fix integer overflow due to cast operation

Is "1" a valid value here?

> 
> > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning:
> > "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now
> > 18446744073709551615.
> > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment: Assigning:
> > "__s" = "1UL".
> > bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg:
> > Calling "memset" with "__p" and "__n * __s" is suspicious because
> > of the very large index, 18446744073709551615. The index may be due
> > to a negative parameter being interpreted as unsigned. [Note: The
> > source code implementation of the function has been overridden by a
> > builtin model.]
> > 1121|           data->mtu = bt_att_get_mtu(server->att);
> > 1122|           data->length = 0;
> > 1123|->         data->rsp_data = new0(uint8_t, data->mtu - 1);
> > 1124|
> > 1125|           return data;
> > ---
> >  src/shared/gatt-server.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> > index 3a53d5dfde6b..66e370d1fe3d 100644
> > --- a/src/shared/gatt-server.c
> > +++ b/src/shared/gatt-server.c
> > @@ -1120,7 +1120,7 @@ static struct read_mult_data
> > *read_mult_data_new(struct bt_gatt_server *server,
> >         data->cur_handle = 0;
> >         data->mtu = bt_att_get_mtu(server->att);
> >         data->length = 0;
> > -       data->rsp_data = new0(uint8_t, data->mtu - 1);
> > +       data->rsp_data = new0(uint8_t, MAX(data->mtu, 1) - 1);
> > 
> >         return data;
> >  }
> > --
> > 2.45.2
> > 
> > 
> 
>
Bastien Nocera July 18, 2024, 1:48 p.m. UTC | #3 
On Thu, 2024-07-18 at 14:40 +0200, Bastien Nocera wrote:
> On Mon, 2024-07-08 at 10:44 -0400, Luiz Augusto von Dentz wrote:
> > Hi Bastien,
> > 
> > On Fri, Jul 5, 2024 at 5:00 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > > 
> > > Set a lower-bound to the data MTU to avoid allocating -1 elements
> > > if
> > > bt_att_get_mtu() returns zero.
> > > 
> > > Error: OVERRUN (CWE-119): [#def36] [important]
> > > bluez-5.76/src/shared/gatt-server.c:1121:2: zero_return: Function
> > > call "bt_att_get_mtu(server->att)" returns 0.
> > > bluez-5.76/src/shared/gatt-server.c:1121:2: assignment:
> > > Assigning:
> > > "data->mtu" = "bt_att_get_mtu(server->att)". The value of "data-
> > > > mtu" is now 0.
> > 
> > We are disconnected or have an invalid bt_att instance if
> > bt_att_get_mtu returns 0 so it is probably pointless to attempt to
> > send any response if that is the case.
> 
> Same as for patch 8 in this series:
> gatt-server: Fix integer overflow due to cast operation
> 
> Is "1" a valid value here?

Never mind, Coverity is happy, so I'm happy too.

> 
> > 
> > > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment:
> > > Assigning:
> > > "__n" = "(size_t)(data->mtu - 1UL)". The value of "__n" is now
> > > 18446744073709551615.
> > > bluez-5.76/src/shared/gatt-server.c:1123:19: assignment:
> > > Assigning:
> > > "__s" = "1UL".
> > > bluez-5.76/src/shared/gatt-server.c:1123:19: overrun-buffer-arg:
> > > Calling "memset" with "__p" and "__n * __s" is suspicious because
> > > of the very large index, 18446744073709551615. The index may be
> > > due
> > > to a negative parameter being interpreted as unsigned. [Note: The
> > > source code implementation of the function has been overridden by
> > > a
> > > builtin model.]
> > > 1121|           data->mtu = bt_att_get_mtu(server->att);
> > > 1122|           data->length = 0;
> > > 1123|->         data->rsp_data = new0(uint8_t, data->mtu - 1);
> > > 1124|
> > > 1125|           return data;
> > > ---
> > >  src/shared/gatt-server.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> > > index 3a53d5dfde6b..66e370d1fe3d 100644
> > > --- a/src/shared/gatt-server.c
> > > +++ b/src/shared/gatt-server.c
> > > @@ -1120,7 +1120,7 @@ static struct read_mult_data
> > > *read_mult_data_new(struct bt_gatt_server *server,
> > >         data->cur_handle = 0;
> > >         data->mtu = bt_att_get_mtu(server->att);
> > >         data->length = 0;
> > > -       data->rsp_data = new0(uint8_t, data->mtu - 1);
> > > +       data->rsp_data = new0(uint8_t, MAX(data->mtu, 1) - 1);
> > > 
> > >         return data;
> > >  }
> > > --
> > > 2.45.2
> > > 
> > > 
> > 
> > 
>
diff mbox series

Patch

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 3a53d5dfde6b..66e370d1fe3d 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1120,7 +1120,7 @@  static struct read_mult_data *read_mult_data_new(struct bt_gatt_server *server,
 	data->cur_handle = 0;
 	data->mtu = bt_att_get_mtu(server->att);
 	data->length = 0;
-	data->rsp_data = new0(uint8_t, data->mtu - 1);
+	data->rsp_data = new0(uint8_t, MAX(data->mtu, 1) - 1);
 
 	return data;
 }