Hi Frédéric,
On Mon, Jan 22, 2024 at 12:43 PM Frédéric Danis
<frederic.danis@xxxxxxxxxxxxx> wrote:
>
> Some PTS GATT tests like GATT/CL/GAR/BI-04-C request to be able to get the
> 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 = 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 = new0(struct bt_att, 1);
> att->chans = queue_new();
> att->mtu = chan->mtu;
> + att->retry_on_sec_error = 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 = 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 !=
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_gatt_client *client,
> bt_gatt_client_destroy_func_t destroy);
> 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, char *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 = 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] == 'y')
> + bt_gatt_client_set_retry_on_sec_error(cli->gatt, true);
> + else if (argv[0][0] == '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 command"},
> + { "set-retry-on-sec-error", cmd_set_retry_on_sec_error,
> + "\tSet retry on security error by elevating security"},
> { }
> };
>
> --
> 2.34.1
>
>
--
Luiz Augusto von Dentz
