Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1433-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240129114900.92919-1-verdre@v0yd.nl> <20240129114900.92919-2-verdre@v0yd.nl>
In-Reply-To: <20240129114900.92919-2-verdre@v0yd.nl>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 29 Jan 2024 08:40:41 -0500
Message-ID: <CABBYNZKpUr+EZtKzPo-it3oojG2jrYT4iJMugZ2Y9LATrWPQag@mail.gmail.com>
Subject: Re: [PATCH BlueZ 1/4] mgmt-tester: Add a 0-opcode to expect_hci_list lists
To: =?UTF-8?Q?Jonas_Dre=C3=9Fler?= <verdre@v0yd.nl>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Jonas,

On Mon, Jan 29, 2024 at 6:49=E2=80=AFAM Jonas Dre=C3=9Fler <verdre@v0yd.nl>=
 wrote:
>
> In add_expect_hci_list() we iterate through the entries of the
> expect_hci_list as long as there is an opcode, which means currently
> this relies on overflowing the buffer to detect the end of the list.
>
> This is not great and when running with address sanitizer, the
> out-of-bounds read gets detected and mgmt-tester aborts. Fix it by
> adding a trailing 0-opcode to all those lists.
> ---
>  tools/mgmt-tester.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/tools/mgmt-tester.c b/tools/mgmt-tester.c
> index 7dfd1b0c7..ee12ed7d5 100644
> --- a/tools/mgmt-tester.c
> +++ b/tools/mgmt-tester.c
> @@ -8798,6 +8798,9 @@ static const struct hci_cmd_data multi_ext_adv_add_=
second_hci_cmds[] =3D {
>                 .len =3D sizeof(le_set_ext_adv_enable_inst_2),
>                 .param =3D le_set_ext_adv_enable_inst_2,
>         },
> +       {
> +               .opcode =3D 0,
> +       },

Normally the compiler would put a NULL term when last member has ',',
but we should either use {} to properly terminate the list or perhaps
it would have been better to have a something like
.expect_hci_list_len =3D ARRAY_SIZE(list) to ensure we never access past
the end of the list.

>  };
>
>  static const struct generic_data multi_ext_advertising_add_second_2 =3D =
{
> @@ -8845,6 +8848,9 @@ static const struct hci_cmd_data multi_ext_adv_remo=
ve_adv_hci_cmds[] =3D {
>                 .len =3D sizeof(advertising_instance1_param),
>                 .param =3D advertising_instance1_param,
>         },
> +       {
> +               .opcode =3D 0,
> +       },
>  };
>
>  static const struct generic_data multi_ext_advertising_remove =3D {
> @@ -8877,6 +8883,9 @@ static const struct hci_cmd_data multi_ext_adv_remo=
ve_all_adv_hci_cmds[] =3D {
>         {
>                 .opcode =3D BT_HCI_CMD_LE_CLEAR_ADV_SETS,
>         },
> +       {
> +               .opcode =3D 0,
> +       },
>  };
>
>  static const struct generic_data multi_ext_advertising_remove_all =3D {
> @@ -8913,6 +8922,9 @@ static const struct hci_cmd_data multi_ext_adv_add_=
2_advs_hci_cmds[] =3D {
>                 .len =3D sizeof(set_ext_adv_data_test1),
>                 .param =3D set_ext_adv_data_test1,
>         },
> +       {
> +               .opcode =3D 0,
> +       },
>  };
>
>  static const struct generic_data multi_ext_advertising_add_no_power =3D =
{
> @@ -10378,6 +10390,9 @@ static const struct hci_cmd_data ll_privacy_add_d=
evice_3_hci_list[] =3D {
>                 .param =3D set_resolv_on_param,
>                 .len =3D sizeof(set_resolv_on_param),
>         },
> +       {
> +               .opcode =3D 0,
> +       },
>  };
>
>  static const struct generic_data ll_privacy_add_device_3 =3D {
> @@ -10495,6 +10510,9 @@ static const struct hci_cmd_data ll_privacy_add_d=
evice_9_hci_list[] =3D {
>                 .len =3D sizeof(le_add_to_resolv_list_param),
>                 .param =3D le_add_to_resolv_list_param
>         },
> +       {
> +               .opcode =3D 0,
> +       },
>  };
>
>  static const struct generic_data ll_privacy_add_device_9 =3D {
> @@ -10823,6 +10841,9 @@ static const struct hci_cmd_data ll_privacy_set_d=
evice_flags_1_hci_list[] =3D {
>                 .param =3D set_resolv_on_param,
>                 .len =3D sizeof(set_resolv_on_param),
>         },
> +       {
> +               .opcode =3D 0,
> +       },
>  };
>
>  static const uint8_t device_flags_changed_params_1[] =3D {
> --
> 2.43.0
>


--=20
Luiz Augusto von Dentz