Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nimble/host: Support database hash #1112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarceauFillon
Copy link

Supports database hash characteristic.
Computes database hash and stores it on first pairing with device.
Computes current hash and compares it with stored hash on reconnection.

See BLE Core spec 5.2 section 7.2

@sjanc sjanc self-requested a review January 18, 2022 14:36
@sjanc
Copy link
Contributor

sjanc commented Jan 18, 2022

Hi, sorry for late response

Could you rebase on top on master branch ?

Supports database hash characteristic.
Computes database hash and stores it on first pairing with device.
Computes current hash and compares it with stored hash on reconnection.

See BLE Core spec 5.2 section 7.2
@MarceauFillon MarceauFillon changed the base branch from 1_4_0_dev to master January 21, 2022 03:18
@MarceauFillon
Copy link
Author

No worries, done!

Copy link
Contributor

@sjanc sjanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

some minor comments (mostly style related), otherwise it looks quite OK for me but CI build fails needs to be addressed before this can go in

also, which peers was this tested against? Android or PTS?

assert(ctxt->op == BLE_GATT_ACCESS_OP_READ_CHR);
assert(ctxt->chr == &ble_svc_gatt_defs[0].characteristics[1]);
uint8_t db_hash[16];
uint16_t len_data = sizeof(db_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please not mix declarations with code (eg. asserts after variables declarations)

uint8_t db_hash[16];
uint16_t len_data = sizeof(db_hash);
int res = ble_compute_db_hash(db_hash);
if(res != 0){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line after declarations

}

rc = os_mbuf_append(ctxt->om, db_hash, len_data);
if(rc !=0){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: if (rc != 0)

@@ -73,7 +108,7 @@ ble_svc_gatt_access(uint16_t conn_handle, uint16_t attr_handle,

put_le16(u8p + 0, ble_svc_gatt_start_handle);
put_le16(u8p + 2, ble_svc_gatt_end_handle);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style

ble_hs_misc_peer_addr_type_to_id(conn->bhc_peer_addr.type);
hash_key.idx = 0;

rc = ble_store_read_hash(conn_handle, &hash_key, &hash_value); // read peer hash, read the database hash characteristic and compare, if rc != 0 => ble_svc_conn_gatt_changed(0, 0xffff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use C style comments /**/ eg:

/* call foo */
foo();

ble_hs_misc_peer_addr_type_to_id(conn->bhc_peer_addr.type);
hash_key.idx = 0;

rc = ble_store_read_hash(conn_handle, &hash_key, &hash_value); // read peer hash, read the database hash characteristic and compare, if rc != 0 => ble_svc_conn_gatt_changed(0, 0xffff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this commented code, is it TODO?


static void ble_db_hash_message_append_u16(uint16_t val, uint8_t *buf, uint8_t *len) {
val = htole16(val);
memcpy(buf + *len, &val, sizeof(uint16_t));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have put_le16, put_le32 etc functions
please use those

@@ -577,6 +579,14 @@ ble_sm_persist_keys(struct ble_sm_proc *proc)
ble_sm_fill_store_value(&peer_addr, authenticated, sc, &proc->peer_keys,
&value_sec);
ble_store_write_peer_sec(&value_sec);


// Compute current database hash and store it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**/

@@ -894,3 +894,5 @@ int ble_gatts_start(void);
*/

#endif

int ble_compute_db_hash(uint8_t db_hash[16]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants