| Message ID | 20240122165000.279381-2-frederic.danis@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enhance GATT to pass PTS tests | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
| tedd_an/MakeCheck | success | Bluez Make Check PASS |
| tedd_an/MakeDistcheck | success | Make Distcheck PASS |
| tedd_an/CheckValgrind | success | Check Valgrind PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
| tedd_an/ScanBuild | warning | ScanBuild: tools/btgatt-client.c:1571:15: warning: Null pointer passed to 1st parameter expecting 'nonnull' end_handle = strtol(argv[1], &endptr, 0); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/btgatt-client.c:1893:2: warning: Value stored to 'argv' is never read argv += optind; ^ ~~~~~~ 2 warnings generated. |
Hi Frédéric, On Mon, Jan 22, 2024 at 12:43 PM Frédéric Danis <frederic.danis@collabora.com> 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 > >
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=818707
---Test result---
Test Summary:
CheckPatch PASS 251.88 seconds
GitLint PASS 153.53 seconds
BuildEll PASS 42.15 seconds
BluezMake PASS 730.75 seconds
MakeCheck PASS 11.23 seconds
MakeDistcheck PASS 160.09 seconds
CheckValgrind PASS 224.46 seconds
CheckSmatch PASS 421.96 seconds
bluezmakeextell PASS 106.00 seconds
IncrementalBuild PASS 3388.99 seconds
ScanBuild WARNING 945.24 seconds
Details
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
tools/btgatt-client.c:1571:15: warning: Null pointer passed to 1st parameter expecting 'nonnull'
end_handle = strtol(argv[1], &endptr, 0);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/btgatt-client.c:1893:2: warning: Value stored to 'argv' is never read
argv += optind;
^ ~~~~~~
2 warnings generated.
---
Regards,
Linux Bluetooth
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; }; 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); +} 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"}, { } };