mbox series

[BlueZ,0/9] Fix a number of static analysis issues #3

Message ID 20240530150057.444585-1-hadess@hadess.net
Headers show
Series Fix a number of static analysis issues #3 | expand

Message

Bastien Nocera May 30, 2024, 2:57 p.m. UTC
14 defects fixed, and 1 error check added.

Let me know whether there's any problems with the implementation, I'm
thinking in particular of the avdtp changes which are pretty invasive.

Cheers

Bastien Nocera (9):
  rctest: Fix possible overrun
  mgmt-tester: Fix buffer overrun
  l2test: Add missing error checking
  rfkill: Avoid using a signed int for an unsigned variable
  shared/mainloop: Fix integer overflow
  sdp: Fix ineffective error guard
  obexd: Fix buffer overrun
  bap: Fix more memory leaks on error
  avdtp: Fix manipulating struct as an array

 gobex/gobex.c                |  2 +-
 lib/sdp.c                    |  8 ++++----
 profiles/audio/avdtp.c       | 33 ++++++++++++++++++++++-----------
 profiles/audio/bap.c         |  5 ++++-
 src/rfkill.c                 |  2 +-
 src/shared/mainloop-notify.c |  3 ++-
 tools/l2test.c               |  5 +++++
 tools/mgmt-tester.c          |  2 +-
 tools/rctest.c               |  3 ++-
 9 files changed, 42 insertions(+), 21 deletions(-)

Comments

Pauli Virtanen May 30, 2024, 4:57 p.m. UTC | #1
Hi,

to, 2024-05-30 kello 16:58 +0200, Bastien Nocera kirjoitti:
> Don't manipulate the "req" structs as if they were flat arrays, static
> analysis and humans are both equally confused by this kind of usage.

struct start_req {
	union {
		struct seid required[1];
		struct seid seids[0];
	};
} __attribute__ ((packed));

and access only via req->seids?

> 
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def26] [important]
> bluez-5.76/profiles/audio/avdtp.c:1675:2: address_of: Taking address with "&start->first_seid" yields a singleton pointer.
> bluez-5.76/profiles/audio/avdtp.c:1675:2: assign: Assigning: "seid" = "&start->first_seid".
> bluez-5.76/profiles/audio/avdtp.c:1679:25: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 1677|           int i;
> 1678|
> 1679|->         for (i = 0; i < count; i++, seid++) {
> 1680|                   if (seid->seid == id) {
> 1681|                           req->collided = TRUE;
> 
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def27] [important]
> bluez-5.76/profiles/audio/avdtp.c:1690:2: address_of: Taking address with "&suspend->first_seid" yields a singleton pointer.
> bluez-5.76/profiles/audio/avdtp.c:1690:2: assign: Assigning: "seid" = "&suspend->first_seid".
> bluez-5.76/profiles/audio/avdtp.c:1694:25: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 1692|		int i;
> 1693|
> 1694|->		for (i = 0; i < count; i++, seid++) {
> 1695|			if (seid->seid == id) {
> 1696|				req->collided = TRUE;
> 
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def28] [important]
> bluez-5.76/profiles/audio/avdtp.c:1799:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer.
> bluez-5.76/profiles/audio/avdtp.c:1799:2: assign: Assigning: "seid" = "&req->first_seid".
> bluez-5.76/profiles/audio/avdtp.c:1801:30: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 1799|		seid = &req->first_seid;
> 1800|
> 1801|->		for (i = 0; i < seid_count; i++, seid++) {
> 1802|			failed_seid = seid->seid;
> 1803|
> 
> Error: ARRAY_VS_SINGLETON (CWE-119): [#def29] [important]
> bluez-5.76/profiles/audio/avdtp.c:1912:2: address_of: Taking address with "&req->first_seid" yields a singleton pointer.
> bluez-5.76/profiles/audio/avdtp.c:1912:2: assign: Assigning: "seid" = "&req->first_seid".
> bluez-5.76/profiles/audio/avdtp.c:1914:30: ptr_arith: Using "seid" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 1912|		seid = &req->first_seid;
> 1913|
> 1914|->	for (i = 0; i < seid_count; i++, seid++) {
> 1915|			failed_seid = seid->seid;
> 1916|
> ---
>  profiles/audio/avdtp.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index 3667e08400dd..38c1870e619d 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -429,6 +429,20 @@ static void avdtp_sep_set_state(struct avdtp *session,
>  				struct avdtp_local_sep *sep,
>  				avdtp_state_t state);
>  
> +#define REQ_GET_NTH_SEID(x)						\
> +	static struct seid *						\
> +	x##_req_get_nth_seid(struct x##_req *req, int count, int i)	\
> +	{								\
> +		if (count == 0 || i >= count)				\
> +			return NULL;					\
> +		if (i == 1)						\
> +			return &req->first_seid;			\
> +		return &req->other_seids[i];				\

(i == 0) and [i - 1]?

> +	}
> +
> +REQ_GET_NTH_SEID(start)
> +REQ_GET_NTH_SEID(suspend)
> +
>  static const char *avdtp_statestr(avdtp_state_t state)
>  {
>  	switch (state) {
> @@ -1672,11 +1686,11 @@ static void check_seid_collision(struct pending_req *req, uint8_t id)
>  static void check_start_collision(struct pending_req *req, uint8_t id)
>  {
>  	struct start_req *start = req->data;
> -	struct seid *seid = &start->first_seid;
>  	int count = 1 + req->data_size - sizeof(struct start_req);
>  	int i;
>  
> -	for (i = 0; i < count; i++, seid++) {
> +	for (i = 0; i < count; i++) {
> +		struct seid *seid = start_req_get_nth_seid(start, count, i);
>  		if (seid->seid == id) {
>  			req->collided = TRUE;
>  			return;
> @@ -1687,11 +1701,11 @@ static void check_start_collision(struct pending_req *req, uint8_t id)
>  static void check_suspend_collision(struct pending_req *req, uint8_t id)
>  {
>  	struct suspend_req *suspend = req->data;
> -	struct seid *seid = &suspend->first_seid;
>  	int count = 1 + req->data_size - sizeof(struct suspend_req);
>  	int i;
>  
> -	for (i = 0; i < count; i++, seid++) {
> +	for (i = 0; i < count; i++) {
> +		struct seid *seid = suspend_req_get_nth_seid(suspend, count, i);
>  		if (seid->seid == id) {
>  			req->collided = TRUE;
>  			return;
> @@ -1785,7 +1799,6 @@ static gboolean avdtp_start_cmd(struct avdtp *session, uint8_t transaction,
>  	struct avdtp_local_sep *sep;
>  	struct avdtp_stream *stream;
>  	struct stream_rej rej;
> -	struct seid *seid;
>  	uint8_t err, failed_seid;
>  	int seid_count, i;
>  
> @@ -1796,9 +1809,9 @@ static gboolean avdtp_start_cmd(struct avdtp *session, uint8_t transaction,
>  
>  	seid_count = 1 + size - sizeof(struct start_req);
>  
> -	seid = &req->first_seid;
> +	for (i = 0; i < seid_count; i++) {
> +		struct seid *seid = start_req_get_nth_seid(req, seid_count, i);
>  
> -	for (i = 0; i < seid_count; i++, seid++) {
>  		failed_seid = seid->seid;
>  
>  		sep = find_local_sep_by_seid(session, seid->seid);
> @@ -1898,7 +1911,6 @@ static gboolean avdtp_suspend_cmd(struct avdtp *session, uint8_t transaction,
>  	struct avdtp_local_sep *sep;
>  	struct avdtp_stream *stream;
>  	struct stream_rej rej;
> -	struct seid *seid;
>  	uint8_t err, failed_seid;
>  	int seid_count, i;
>  
> @@ -1909,9 +1921,8 @@ static gboolean avdtp_suspend_cmd(struct avdtp *session, uint8_t transaction,
>  
>  	seid_count = 1 + size - sizeof(struct suspend_req);
>  
> -	seid = &req->first_seid;
> -
> -	for (i = 0; i < seid_count; i++, seid++) {
> +	for (i = 0; i < seid_count; i++) {
> +		struct seid *seid = suspend_req_get_nth_seid(req, seid_count, i);
>  		failed_seid = seid->seid;
>  
>  		sep = find_local_sep_by_seid(session, seid->seid);
Bastien Nocera May 31, 2024, 3:50 p.m. UTC | #2
On Thu, 2024-05-30 at 19:57 +0300, Pauli Virtanen wrote:
> Hi,
> 
> to, 2024-05-30 kello 16:58 +0200, Bastien Nocera kirjoitti:
> > Don't manipulate the "req" structs as if they were flat arrays,
> > static
> > analysis and humans are both equally confused by this kind of
> > usage.
> 
> struct start_req {
> 	union {
> 		struct seid required[1];
> 		struct seid seids[0];
> 	};
> } __attribute__ ((packed));
> 
> and access only via req->seids?

That's a good idea, I'll give it a try.

> <snip>
> > +#define
> > REQ_GET_NTH_SEID(x)						\
> > +	static struct seid
> > *						\
> > +	x##_req_get_nth_seid(struct x##_req *req, int count, int
> > i)	\
> > +	{							
> > 	\
> > +		if (count == 0 || i >=
> > count)				\
> > +			return
> > NULL;					\
> > +		if (i ==
> > 1)						\
> > +			return &req-
> > >first_seid;			\
> > +		return &req-
> > >other_seids[i];				\
> 
> (i == 0) and [i - 1]?

*facepalm*

Yes, this will need a v2, thanks for spotting that.
Luiz Augusto von Dentz June 3, 2024, 7:19 p.m. UTC | #3
Hi Bastien,

On Fri, May 31, 2024 at 11:51 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2024-05-30 at 19:57 +0300, Pauli Virtanen wrote:
> > Hi,
> >
> > to, 2024-05-30 kello 16:58 +0200, Bastien Nocera kirjoitti:
> > > Don't manipulate the "req" structs as if they were flat arrays,
> > > static
> > > analysis and humans are both equally confused by this kind of
> > > usage.
> >
> > struct start_req {
> >       union {
> >               struct seid required[1];
> >               struct seid seids[0];
> >       };
> > } __attribute__ ((packed));
> >
> > and access only via req->seids?
>
> That's a good idea, I'll give it a try.
>
> > <snip>
> > > +#define
> > > REQ_GET_NTH_SEID(x)                                         \
> > > +   static struct seid
> > > *                                           \
> > > +   x##_req_get_nth_seid(struct x##_req *req, int count, int
> > > i)  \
> > > +   {
> > >     \
> > > +           if (count == 0 || i >=
> > > count)                              \
> > > +                   return
> > > NULL;                                       \
> > > +           if (i ==
> > > 1)                                          \
> > > +                   return &req-
> > > >first_seid;                        \
> > > +           return &req-
> > > >other_seids[i];                            \
> >
> > (i == 0) and [i - 1]?
>
> *facepalm*
>
> Yes, this will need a v2, thanks for spotting that.

Ive applied the set except for this one, please resend once you are
done with the suggested changes.
patchwork-bot+bluetooth@kernel.org June 3, 2024, 7:20 p.m. UTC | #4
Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 30 May 2024 16:57:54 +0200 you wrote:
> 14 defects fixed, and 1 error check added.
> 
> Let me know whether there's any problems with the implementation, I'm
> thinking in particular of the avdtp changes which are pretty invasive.
> 
> Cheers
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/9] rctest: Fix possible overrun
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=24cf04939502
  - [BlueZ,2/9] mgmt-tester: Fix buffer overrun
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=aa54087f13d5
  - [BlueZ,3/9] l2test: Add missing error checking
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=ccec5e8ef171
  - [BlueZ,4/9] rfkill: Avoid using a signed int for an unsigned variable
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c9fcea121f9a
  - [BlueZ,5/9] shared/mainloop: Fix integer overflow
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=6cf9117bfd3f
  - [BlueZ,6/9] sdp: Fix ineffective error guard
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=bd954700e631
  - [BlueZ,7/9] obexd: Fix buffer overrun
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=1764cea5c7fd
  - [BlueZ,8/9] bap: Fix more memory leaks on error
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=cc8e6ef63509
  - [BlueZ,9/9] avdtp: Fix manipulating struct as an array
    (no matching commit)

You are awesome, thank you!
Bastien Nocera July 2, 2024, 8:53 a.m. UTC | #5
On Mon, 2024-06-03 at 15:19 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Fri, May 31, 2024 at 11:51 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Thu, 2024-05-30 at 19:57 +0300, Pauli Virtanen wrote:
> > > Hi,
> > > 
> > > to, 2024-05-30 kello 16:58 +0200, Bastien Nocera kirjoitti:
> > > > Don't manipulate the "req" structs as if they were flat arrays,
> > > > static
> > > > analysis and humans are both equally confused by this kind of
> > > > usage.
> > > 
> > > struct start_req {
> > >       union {
> > >               struct seid required[1];
> > >               struct seid seids[0];
> > >       };
> > > } __attribute__ ((packed));
> > > 
> > > and access only via req->seids?
> > 
> > That's a good idea, I'll give it a try.
> > 
> > > <snip>
> > > > +#define
> > > > REQ_GET_NTH_SEID(x)                                         \
> > > > +   static struct seid
> > > > *                                           \
> > > > +   x##_req_get_nth_seid(struct x##_req *req, int count, int
> > > > i)  \
> > > > +   {
> > > >     \
> > > > +           if (count == 0 || i >=
> > > > count)                              \
> > > > +                   return
> > > > NULL;                                       \
> > > > +           if (i ==
> > > > 1)                                          \
> > > > +                   return &req-
> > > > > first_seid;                        \
> > > > +           return &req-
> > > > > other_seids[i];                            \
> > > 
> > > (i == 0) and [i - 1]?
> > 
> > *facepalm*
> > 
> > Yes, this will need a v2, thanks for spotting that.
> 
> Ive applied the set except for this one, please resend once you are
> done with the suggested changes.

I've redone this patch, it's now in:
https://patchwork.kernel.org/project/bluetooth/list/?series=867448
https://lore.kernel.org/linux-bluetooth/20240702084900.773620-3-hadess@hadess.net/T/#u

Thanks to Pauli for the help.

>