Fix ble_gap_unpair_oldest_peer to prevent writing to invalid memory

[darshan7patel]

• Modifications were made for secure handling of bonded peers.

• The changes ensures proper handling of bonded peers, preventing unintended memory writes.

[andrzej-kaczmarek]

what exactly was the issue? original code retrieves only 1 address so it looks fine.

[darshan7patel]

what exactly was the issue? original code retrieves only 1 address so it looks fine.

• Changes were made to accommodate a maximum of BLE_STORE_MAX_BONDS bonds. Previously, it was hardcoded to support only a single bond, limiting the system to handle just one bond.

• Additionally, the function ble_store_util_bonded_peers is designed to store bonded peers and return a list. To ensure proper memory allocation, it is essential to pass an array as the function argument. Failure to do so may result in the data being stored outside the allocated memory. Therefore, passing an array is crucial for correct functionality.

[andrzej-kaczmarek]

This function removes 1 bond which is 1st on the returned list and thus it only needs to retrieve that single bond. There's no need to retrieve all bonded devices only to pick 1st one anyway...

[darshan7patel]

This function removes 1 bond which is 1st on the returned list and thus it only needs to retrieve that single bond. There's no need to retrieve all bonded devices only to pick 1st one anyway...

That's correct. However, it's necessary to provide BLE_STORE_MAX_BONDS as a parameter to ble_store_util_bonded_peers to manage the maximum bond count. Subsequently, ble_store_util_iter_unique_peer will be invoked. In this process, when it accesses set->peer_id_addrs[set->num_peers], it requires an array to access that specific index. Therefore, an array is essential when the maximum bond count is greater than one.

[andrzej-kaczmarek]

max_peers parameter passed to ble_store_util_bonded_peers is set to 1 which means ble_store_iterate will stop iterating after 1st matched peer. There's no need for an array because no more than 1 peer is returned.

[darshan7patel]

max_peers parameter passed to ble_store_util_bonded_peers is set to 1 which means ble_store_iterate will stop iterating after 1st matched peer. There's no need for an array because no more than 1 peer is returned.

But in the ble_store_util_bonded_peer function, the max_peer parameter (which is the third parameter), is hardcoded to 1. This could be problematic when the maximum count is 2 (or BLE_STORE_MAX_BONDS > 1). When the count reaches 2, it returns an error code (rc=6) indicating resource exhaustion, and consequently, the ble_gap_unpair function is not called, leading to the oldest peer not being deleted.

Previously, when an array was not used, the function appeared to work, but there was a mismatch between the list count and the actual stored values and it's possible that the data was being stored outside the allocated memory. This changes should resolve the resource exhaustion error and enable the proper deletion of the oldest peer.

[andrzej-kaczmarek]

ble_store_util_bonded_peers returns rc=6 only if ble_store_iterate returns rc=6 so assuming you're right, please point to the location in file which returns this error code in described scenario.

[darshan7patel]

ble_store_util_bonded_peers returns rc=6 only if ble_store_iterate returns rc=6 so assuming you're right, please point to the location in file which returns this error code in described scenario.

The error code rc=6 is returned from ble_store_read within the ble_store_iterate function, indicating resource exhaustion.

[andrzej-kaczmarek]

Again, please point me to exact location in code which returns this error code. I don't see how ble_store_read can return rc=6 in any case for either config or ram stores.

[darshan7patel]

Hi @andrzej-kaczmarek,

Upon review, I noticed that instead of passing BLE_STORE_MAX_BONDS as the argument for ble_store_util_bonded_peers in ble_gap_unpair_oldest_peer, a hardcoded value of 1 is being used. Consequently, within ble_store_iterate called from ble_store_util_bonded_peers, the callback function ble_store_util_iter_unique_peer is invoked. It returns 1 since set->num_peers >= set->max_peers evaluates to true, causing an overflow. As a result, it updates the set status to BLE_HS_ENOMEM (6) due to resource exhaustion.

Thus, this is the place where overflow occurred.

These changes should address the resource exhaustion error and enable proper deletion of the oldest peer.

[andrzej-kaczmarek]

That behavior is on purpose because we only need single peer returned by ble_store_util_bonded_peers in ble_store_util_delete_oldest_peer. The status field set from ble_store_util_iterate_unique_peer is simply ignored because we don't care if there were more peers than requested (i.e. 1). We only care about return value from the function call and that can only fail if there was actual error when reading from store.

[darshan7patel]

Okay, earlier when I attempted to bond 3 devices by setting MAX_BOND_COUNT to 2, it didn't work correctly. The correct behavior should be to delete the oldest peer when bonding with the 3rd device. Therefore, I made changes, and now it functions correctly. Additionally, in ble_gap_unpair_oldest_except, we are passing an array and BLE_STORE_MAX_BONDS as parameters in ble_store_util_bonded_peers, which is correct. So, I took reference from there and modified ble_gap_unpair_oldest_peer to pass an array and BLE_STORE_MAX_BONDS as arguments, resulting in the correct behaviour when the number of devices is greater than 2. And regardless of MAX_BOND_COUNT, the correct behavior will always be to delete the oldest device.
However, I will conduct more tests to ensure everything is functioning properly.

[andrzej-kaczmarek]

ble_gap_unpair_oldest_except uses BLE_STORE_MAX_BONDS because it needs to retrieve all bonds in order to apply filter.

I don't really see how ble_gap_unpair_oldest_peer can fail because of using 1 instead of BLE_STORE_MAX_BONDS. Actually I've just checked it in my app and it works perfectly fine.