Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1239-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240122165000.279381-1-frederic.danis@collabora.com> <20240122165000.279381-2-frederic.danis@collabora.com>
In-Reply-To: <20240122165000.279381-2-frederic.danis@collabora.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 22 Jan 2024 13:13:12 -0500
Message-ID: <CABBYNZJ3Wfdhf5fU6hLSZh1Sz0zZHr_Gf5anMqOyW+CoMedOow@mail.gmail.com>
Subject: Re: [PATCH BlueZ 1/5] gatt: Prevent security level change for PTS
 GATT tests
To: =?UTF-8?B?RnLDqWTDqXJpYyBEYW5pcw==?= <frederic.danis@collabora.com>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Fr=C3=A9d=C3=A9ric,

On Mon, Jan 22, 2024 at 12:43=E2=80=AFPM Fr=C3=A9d=C3=A9ric Danis
<frederic.danis@collabora.com> wrote:
>
> Some PTS GATT tests like GATT/CL/GAR/BI-04-C request to be able to get th=
e
> security error and do not try to change the security level.
>
> this commit adds a variable allowing to prevent to change the security
> level.
> ---
>  src/shared/att.c         | 14 ++++++++++++++
>  src/shared/att.h         |  1 +
>  src/shared/gatt-client.c |  9 +++++++++
>  src/shared/gatt-client.h |  2 ++
>  tools/btgatt-client.c    | 38 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 64 insertions(+)
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 62c884b65..decc24314 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -87,6 +87,8 @@ struct bt_att {
>
>         struct sign_info *local_sign;
>         struct sign_info *remote_sign;
> +
> +       bool retry_on_sec_error;

Id use something like auto_retry, or perhaps just use
att_send_op->retry instead.

>  };
>
>  struct sign_info {
> @@ -786,6 +788,9 @@ static bool handle_error_rsp(struct bt_att_chan *chan=
, uint8_t *pdu,
>
>         *opcode =3D rsp->opcode;
>
> +       if (!att->retry_on_sec_error)
> +               return false;
> +
>         /* If operation has already been marked as retry don't attempt to=
 change
>          * the security again.
>          */
> @@ -1262,6 +1267,7 @@ struct bt_att *bt_att_new(int fd, bool ext_signed)
>         att =3D new0(struct bt_att, 1);
>         att->chans =3D queue_new();
>         att->mtu =3D chan->mtu;
> +       att->retry_on_sec_error =3D true;
>
>         /* crypto is optional, if not available leave it NULL */
>         if (!ext_signed)
> @@ -2042,3 +2048,11 @@ bool bt_att_has_crypto(struct bt_att *att)
>
>         return att->crypto ? true : false;
>  }
> +
> +void bt_att_set_retry_on_sec_error(struct bt_att *att, bool retry_on_sec=
_error)
> +{
> +       if (!att)
> +               return;
> +
> +       att->retry_on_sec_error =3D retry_on_sec_error;
> +}
> diff --git a/src/shared/att.h b/src/shared/att.h
> index 4aa3de87b..8ed89ba80 100644
> --- a/src/shared/att.h
> +++ b/src/shared/att.h
> @@ -110,3 +110,4 @@ bool bt_att_set_local_key(struct bt_att *att, uint8_t=
 sign_key[16],
>  bool bt_att_set_remote_key(struct bt_att *att, uint8_t sign_key[16],
>                         bt_att_counter_func_t func, void *user_data);
>  bool bt_att_has_crypto(struct bt_att *att);
> +void bt_att_set_retry_on_sec_error(struct bt_att *att, bool retry_on_sec=
_error);
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 5de679c9b..b484db9db 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -3818,3 +3818,12 @@ bool bt_gatt_client_idle_unregister(struct bt_gatt=
_client *client,
>
>         return false;
>  }
> +
> +void bt_gatt_client_set_retry_on_sec_error(struct bt_gatt_client *client=
,
> +                                               bool retry_on_sec_error)
> +{
> +       if (!client)
> +               return;
> +
> +       bt_att_set_retry_on_sec_error(client->att, retry_on_sec_error);
> +}

I wonder if it wouldn't be better to just have it as
bt_att_set_retry(guint op) and then set it via att_send_op->retry !=3D
retry; so it can be used on a per operation basis that way we can try
to have a property over D-Bus to be able to pass such tests using
bluetoothctl.

> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
> index bccd04a62..fdb841df0 100644
> --- a/src/shared/gatt-client.h
> +++ b/src/shared/gatt-client.h
> @@ -134,3 +134,5 @@ unsigned int bt_gatt_client_idle_register(struct bt_g=
att_client *client,
>                                         bt_gatt_client_destroy_func_t des=
troy);
>  bool bt_gatt_client_idle_unregister(struct bt_gatt_client *client,
>                                                 unsigned int id);
> +void bt_gatt_client_set_retry_on_sec_error(struct bt_gatt_client *client=
,
> +                                               bool retry_on_sec_error);
> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> index 58a03bd48..76c74c7a8 100644
> --- a/tools/btgatt-client.c
> +++ b/tools/btgatt-client.c
> @@ -1295,6 +1295,42 @@ static void cmd_set_sign_key(struct client *cli, c=
har *cmd_str)
>                 set_sign_key_usage();
>  }
>
> +static void set_retry_on_sec_error_usage(void)
> +{
> +       printf("Usage: set-retry-on-sec-error <y/n>\n"
> +               "e.g.:\n"
> +               "\tset-retry-on-sec-error n\n");
> +}
> +
> +static void cmd_set_retry_on_sec_error(struct client *cli, char *cmd_str=
)
> +{
> +       char *argv[2];
> +       int argc =3D 0;
> +
> +       if (!bt_gatt_client_is_ready(cli->gatt)) {
> +               printf("GATT client not initialized\n");
> +               return;
> +       }
> +
> +       if (!parse_args(cmd_str, 1, argv, &argc)) {
> +               printf("Too many arguments\n");
> +               set_retry_on_sec_error_usage();
> +               return;
> +       }
> +
> +       if (argc < 1) {
> +               set_retry_on_sec_error_usage();
> +               return;
> +       }
> +
> +       if (argv[0][0] =3D=3D 'y')
> +               bt_gatt_client_set_retry_on_sec_error(cli->gatt, true);
> +       else if (argv[0][0] =3D=3D 'n')
> +               bt_gatt_client_set_retry_on_sec_error(cli->gatt, false);
> +       else
> +               printf("Invalid argument: %s\n", argv[0]);
> +}
> +
>  static void cmd_help(struct client *cli, char *cmd_str);
>
>  typedef void (*command_func_t)(struct client *cli, char *cmd_str);
> @@ -1329,6 +1365,8 @@ static struct {
>                                 "\tGet security level on le connection"},
>         { "set-sign-key", cmd_set_sign_key,
>                                 "\tSet signing key for signed write comma=
nd"},
> +       { "set-retry-on-sec-error", cmd_set_retry_on_sec_error,
> +                       "\tSet retry on security error by elevating secur=
ity"},
>         { }
>  };
>
> --
> 2.34.1
>
>


--=20
Luiz Augusto von Dentz