Skip to content

Commit 6e19440

Browse files
anhmoltlemrey
anhmolt
authored and
lemrey
committed
lib: ble_conn_params: fix unsafe use of conn_handle as idx into array
It is not safe to assume that connection handles will start at zero and always stay below the number of max connections. Therefore, connection handle cannot, and should not, be used to index into arrays or lists. Fix this by using the nrf_sdh_ble_idx_get function to return an index that has been assigned to the connection handle by the softdevice handler. Signed-off-by: Andreas Moltumyr <[email protected]>
1 parent ceb0b72 commit 6e19440

File tree

5 files changed

+184
-173
lines changed

5 files changed

+184
-173
lines changed

‎include/ble_conn_params.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,6 @@ int ble_conn_params_override(uint16_t conn_handle, const ble_gap_conn_params_t *
119119
*
120120
* @return 0 On success.
121121
* @return -EINVAL Invalid ATT MTU or connection handle.
122-
* @return -ENOTCONN Peer is not connected.
123122
*/
124123
int ble_conn_params_att_mtu_set(uint16_t conn_handle, uint16_t att_mtu);
125124

@@ -132,7 +131,6 @@ int ble_conn_params_att_mtu_set(uint16_t conn_handle, uint16_t att_mtu);
132131
* @return 0 On success.
133132
* @return -EINVAL Invalid connection handle.
134133
* @return -EFAULT @p att_mtu is @c NULL.
135-
* @return -ENOTCONN Peer is not connected.
136134
*/
137135
int ble_conn_params_att_mtu_get(uint16_t conn_handle, uint16_t *att_mtu);
138136

@@ -147,7 +145,6 @@ int ble_conn_params_att_mtu_get(uint16_t conn_handle, uint16_t *att_mtu);
147145
*
148146
* @return 0 On success.
149147
* @return -EINVAL Invalid data length or connection handle.
150-
* @return -ENOTCONN Peer is not connected.
151148
*/
152149
int ble_conn_params_data_length_set(uint16_t conn_handle, uint8_t data_length);
153150

@@ -160,7 +157,6 @@ int ble_conn_params_data_length_set(uint16_t conn_handle, uint8_t data_length);
160157
* @return 0 On success.
161158
* @return -EINVAL Invalid connection handle.
162159
* @return -EFAULT @p data_length is @c NULL.
163-
* @return -ENOTCONN Peer is not connected.
164160
*/
165161
int ble_conn_params_data_length_get(uint16_t conn_handle, uint8_t *data_length);
166162

@@ -175,7 +171,6 @@ int ble_conn_params_data_length_get(uint16_t conn_handle, uint8_t *data_length);
175171
*
176172
* @return 0 On success.
177173
* @return -EINVAL Invalid data length or connection handle.
178-
* @return -ENOTCONN Peer is not connected.
179174
*/
180175
int ble_conn_params_phy_radio_mode_set(uint16_t conn_handle, ble_gap_phys_t phy_pref);
181176

@@ -188,6 +183,5 @@ int ble_conn_params_phy_radio_mode_set(uint16_t conn_handle, ble_gap_phys_t phy_
188183
* @return 0 On success.
189184
* @return -EINVAL Invalid connection handle.
190185
* @return -EFAULT @p phy_pref is @c NULL.
191-
* @return -ENOTCONN Peer is not connected.
192186
*/
193187
int ble_conn_params_phy_radio_mode_get(uint16_t conn_handle, ble_gap_phys_t *phy_pref);

‎lib/ble_conn_params/att_mtu.c‎

Lines changed: 59 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -17,141 +17,149 @@ extern void ble_conn_params_event_send(const struct ble_conn_params_evt *evt);
1717
static struct {
1818
uint16_t att_mtu;
1919
uint8_t att_mtu_exchange_pending : 1;
20-
uint8_t connected : 1;
2120
} links[CONFIG_NRF_SDH_BLE_TOTAL_LINK_COUNT] = {
2221
[0 ... CONFIG_NRF_SDH_BLE_TOTAL_LINK_COUNT - 1] = {
2322
.att_mtu = CONFIG_BLE_CONN_PARAMS_ATT_MTU,
2423
}
2524
};
2625

27-
static void mtu_exchange_request(uint16_t conn_handle)
26+
static void mtu_exchange_request(uint16_t conn_handle, int idx)
2827
{
2928
int err;
3029

31-
err = sd_ble_gattc_exchange_mtu_request(conn_handle, links[conn_handle].att_mtu);
30+
err = sd_ble_gattc_exchange_mtu_request(conn_handle, links[idx].att_mtu);
3231
if (!err) {
3332
return;
3433
}
3534

3635
if (err == NRF_ERROR_BUSY) {
3736
/* Retry */
3837
LOG_DBG("Another procedure is ongoing, will retry");
39-
links[conn_handle].att_mtu_exchange_pending = true;
38+
links[idx].att_mtu_exchange_pending = true;
4039
} else if (err) {
4140
LOG_ERR("Failed to initiate ATT MTU exchange, nrf_error %#x", err);
4241
}
4342
}
4443

45-
static void on_exchange_mtu_req_evt(uint16_t conn_handle,
44+
static void on_exchange_mtu_req_evt(uint16_t conn_handle, int idx,
4645
const ble_gatts_evt_exchange_mtu_request_t *evt)
4746
{
4847
int err;
49-
uint16_t client_mtu = evt->client_rx_mtu;
5048

51-
links[conn_handle].att_mtu = MIN(client_mtu, links[conn_handle].att_mtu);
52-
links[conn_handle].att_mtu_exchange_pending = false;
49+
/* Determine the lowest ATT MTU between our own desired ATT MTU and the peer's. */
50+
links[idx].att_mtu = MIN(evt->client_rx_mtu, links[idx].att_mtu);
51+
links[idx].att_mtu_exchange_pending = false;
5352

54-
LOG_INF("Peer %#x requested ATT MTU of %u bytes", conn_handle, client_mtu);
53+
LOG_INF("Peer %#x requested ATT MTU of %u bytes", conn_handle, evt->client_rx_mtu);
5554

56-
err = sd_ble_gatts_exchange_mtu_reply(conn_handle, links[conn_handle].att_mtu);
55+
err = sd_ble_gatts_exchange_mtu_reply(conn_handle, links[idx].att_mtu);
5756
if (err) {
5857
LOG_ERR("Failed to reply to MTU exchange request, nrf_error %#x", err);
5958
return;
6059
}
6160

62-
LOG_INF("ATT MTU set to %u bytes for peer %#x", links[conn_handle].att_mtu, conn_handle);
61+
LOG_INF("ATT MTU set to %u bytes for peer %#x", links[idx].att_mtu, conn_handle);
6362

6463
/* The ATT MTU exchange has finished, send an event to the application */
6564
const struct ble_conn_params_evt app_evt = {
6665
.id = BLE_CONN_PARAMS_EVT_ATT_MTU_UPDATED,
6766
.conn_handle = conn_handle,
68-
.att_mtu = links[conn_handle].att_mtu,
67+
.att_mtu = links[idx].att_mtu,
6968
};
7069

7170
ble_conn_params_event_send(&app_evt);
7271
}
7372

7473
/* The effective ATT MTU is set to the lowest between what we requested and the peer's response.
75-
* This events concludes the ATT MTU exchange.
74+
* This event concludes the ATT MTU exchange.
7675
*/
77-
static void on_exchange_mtu_rsp_evt(uint16_t conn_handle,
76+
static void on_exchange_mtu_rsp_evt(uint16_t conn_handle, int idx,
7877
const ble_gattc_evt_exchange_mtu_rsp_t *evt)
7978
{
80-
uint16_t server_rx_mtu = evt->server_rx_mtu;
79+
/* Determine the lowest ATT MTU between our own desired ATT MTU and the peer's. */
80+
links[idx].att_mtu = MIN(evt->server_rx_mtu, links[idx].att_mtu);
81+
links[idx].att_mtu_exchange_pending = false;
8182

82-
/* Determine the lowest MTU between our own desired MTU and the peer's */
83-
links[conn_handle].att_mtu_exchange_pending = false;
84-
links[conn_handle].att_mtu = MIN(server_rx_mtu, links[conn_handle].att_mtu);
83+
LOG_INF("ATT MTU set to %u bytes for peer %#x", links[idx].att_mtu, conn_handle);
8584

86-
LOG_INF("ATT MTU set to %u bytes for peer %#x", links[conn_handle].att_mtu, conn_handle);
87-
88-
/* Send an event to the application */
85+
/* The ATT MTU exchange has finished, send an event to the application. */
8986
const struct ble_conn_params_evt app_evt = {
9087
.id = BLE_CONN_PARAMS_EVT_ATT_MTU_UPDATED,
9188
.conn_handle = conn_handle,
92-
.att_mtu = links[conn_handle].att_mtu,
89+
.att_mtu = links[idx].att_mtu,
9390
};
9491

9592
ble_conn_params_event_send(&app_evt);
9693
}
9794

98-
static void on_connected(uint16_t conn_handle, const ble_gap_evt_connected_t *evt)
95+
static void on_connected(uint16_t conn_handle, int idx, const ble_gap_evt_connected_t *evt)
9996
{
97+
ARG_UNUSED(evt);
98+
10099
if (IS_ENABLED(CONFIG_BLE_CONN_PARAMS_INITIATE_ATT_MTU_EXCHANGE)) {
101100
LOG_INF("Initiating ATT MTU exchange procedure (%u -> %u bytes) for peer %#x",
102-
BLE_GATT_ATT_MTU_DEFAULT, links[conn_handle].att_mtu, conn_handle);
101+
BLE_GATT_ATT_MTU_DEFAULT, links[idx].att_mtu, conn_handle);
103102

104-
mtu_exchange_request(conn_handle);
103+
mtu_exchange_request(conn_handle, idx);
105104
}
106-
107-
links[conn_handle].connected = true;
108105
}
109106

110-
static void on_disconnected(uint16_t conn_handle)
107+
static void on_disconnected(uint16_t conn_handle, int idx)
111108
{
112-
links[conn_handle].connected = false;
109+
ARG_UNUSED(conn_handle);
110+
111+
links[idx].att_mtu = CONFIG_BLE_CONN_PARAMS_ATT_MTU;
112+
links[idx].att_mtu_exchange_pending = false;
113113
}
114114

115115
static void on_ble_evt(const ble_evt_t *evt, void *ctx)
116116
{
117117
const uint16_t conn_handle = evt->evt.common_evt.conn_handle;
118+
const int idx = nrf_sdh_ble_idx_get(conn_handle);
119+
120+
__ASSERT(idx >= 0, "Invalid idx %d for conn_handle %#x, evt_id %#x",
121+
idx, conn_handle, evt->header.evt_id);
118122

119123
switch (evt->header.evt_id) {
120124
case BLE_GAP_EVT_CONNECTED:
121-
on_connected(conn_handle, &evt->evt.gap_evt.params.connected);
122-
break;
125+
on_connected(conn_handle, idx, &evt->evt.gap_evt.params.connected);
126+
/* There is no pending ATT MTU exchange or the exchange was just attempted
127+
* and retry is not needed immediately. Return here.
128+
*/
129+
return;
123130
case BLE_GAP_EVT_DISCONNECTED:
124-
on_disconnected(conn_handle);
125-
break;
131+
on_disconnected(conn_handle, idx);
132+
/* No neeed to retry ATT MTU exchange if disconnected. Return here. */
133+
return;
126134

127135
case BLE_GATTC_EVT_EXCHANGE_MTU_RSP:
128136
on_exchange_mtu_rsp_evt(
129-
conn_handle, &evt->evt.gattc_evt.params.exchange_mtu_rsp);
137+
conn_handle, idx, &evt->evt.gattc_evt.params.exchange_mtu_rsp);
130138
break;
131139
case BLE_GATTS_EVT_EXCHANGE_MTU_REQUEST:
132140
on_exchange_mtu_req_evt(
133-
conn_handle, &evt->evt.gatts_evt.params.exchange_mtu_request);
141+
conn_handle, idx, &evt->evt.gatts_evt.params.exchange_mtu_request);
134142
break;
135143
default:
136144
/* Ignore */
137145
break;
138146
}
139147

140-
if (conn_handle > CONFIG_NRF_SDH_BLE_TOTAL_LINK_COUNT) {
141-
return;
142-
}
143-
144-
/* Retry any procedures that were busy */
145-
if (links[conn_handle].connected && links[conn_handle].att_mtu_exchange_pending) {
146-
links[conn_handle].att_mtu_exchange_pending = false;
147-
mtu_exchange_request(conn_handle);
148+
/* Retry the ATT MTU exchange procedure for the current connection handle
149+
* if the SoftDevice was previously busy.
150+
*/
151+
if (links[idx].att_mtu_exchange_pending) {
152+
links[idx].att_mtu_exchange_pending = false;
153+
mtu_exchange_request(conn_handle, idx);
148154
}
149155
}
150156
NRF_SDH_BLE_OBSERVER(ble_observer, on_ble_evt, NULL, 0);
151157

152158
int ble_conn_params_att_mtu_set(uint16_t conn_handle, uint16_t att_mtu)
153159
{
154-
if (conn_handle > ARRAY_SIZE(links)) {
160+
const int idx = nrf_sdh_ble_idx_get(conn_handle);
161+
162+
if (idx < 0) {
155163
return -EINVAL;
156164
}
157165

@@ -160,22 +168,25 @@ int ble_conn_params_att_mtu_set(uint16_t conn_handle, uint16_t att_mtu)
160168
return -EINVAL;
161169
}
162170

163-
links[conn_handle].att_mtu = att_mtu;
164-
mtu_exchange_request(conn_handle);
171+
links[idx].att_mtu = att_mtu;
172+
mtu_exchange_request(conn_handle, idx);
165173

166174
return 0;
167175
}
168176

169177
int ble_conn_params_att_mtu_get(uint16_t conn_handle, uint16_t *att_mtu)
170178
{
171-
if (conn_handle > ARRAY_SIZE(links)) {
179+
const int idx = nrf_sdh_ble_idx_get(conn_handle);
180+
181+
if (idx < 0) {
172182
return -EINVAL;
173183
}
184+
174185
if (!att_mtu) {
175186
return -EFAULT;
176187
}
177188

178-
*att_mtu = links[conn_handle].att_mtu;
189+
*att_mtu = links[idx].att_mtu;
179190

180191
return 0;
181192
}

‎lib/ble_conn_params/conn_param.c‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,13 @@ static struct {
3030
}
3131
};
3232

33-
static void conn_params_negotiate(uint16_t conn_handle)
33+
static void conn_params_negotiate(uint16_t conn_handle, int idx)
3434
{
3535
int err;
3636

3737
LOG_DBG("Negotiating desired parameters with peer %#x", conn_handle);
3838

39-
err = sd_ble_gap_conn_param_update(conn_handle, &links[conn_handle].ppcp);
39+
err = sd_ble_gap_conn_param_update(conn_handle, &links[idx].ppcp);
4040
if (err) {
4141
LOG_ERR("Failed to request GAP connection parameters update, nrf_error %#x", err);
4242
}
@@ -87,23 +87,22 @@ static bool conn_params_can_agree(const ble_gap_conn_params_t *conn_params)
8787
return true;
8888
}
8989

90-
static void on_connected(uint16_t conn_handle, const ble_gap_evt_connected_t *evt)
90+
static void on_connected(uint16_t conn_handle, int idx, const ble_gap_evt_connected_t *evt)
9191
{
92-
const uint8_t role = evt->role;
93-
94-
links[conn_handle].retries = CONFIG_BLE_CONN_PARAMS_NEGOTIATION_RETRIES;
92+
links[idx].retries = CONFIG_BLE_CONN_PARAMS_NEGOTIATION_RETRIES;
9593

9694
/* Copy default ppcp */
97-
memcpy(&links[conn_handle].ppcp, &ppcp, sizeof(ble_gap_conn_params_t));
95+
memcpy(&links[idx].ppcp, &ppcp, sizeof(ble_gap_conn_params_t));
9896

99-
if (role == BLE_GAP_ROLE_PERIPH) {
97+
if (evt->role == BLE_GAP_ROLE_PERIPH) {
10098
if (!conn_params_can_agree(&evt->conn_params)) {
101-
conn_params_negotiate(conn_handle);
99+
conn_params_negotiate(conn_handle, idx);
102100
}
103101
}
104102
}
105103

106-
static void on_conn_params_update(uint16_t conn_handle, const ble_gap_evt_conn_param_update_t *evt)
104+
static void on_conn_params_update(uint16_t conn_handle, int idx,
105+
const ble_gap_evt_conn_param_update_t *evt)
107106
{
108107
LOG_DBG("GAP connection params updated, conn. interval min %u max %u,"
109108
" slave latency %u, sup. timeout %u",
@@ -125,9 +124,9 @@ static void on_conn_params_update(uint16_t conn_handle, const ble_gap_evt_conn_p
125124
/** TODO: should use a timer here
126125
* what is this really used for
127126
*/
128-
if (links[conn_handle].retries) {
129-
links[conn_handle].retries--;
130-
conn_params_negotiate(conn_handle);
127+
if (links[idx].retries) {
128+
links[idx].retries--;
129+
conn_params_negotiate(conn_handle, idx);
131130
return;
132131
}
133132

@@ -145,26 +144,24 @@ static void on_conn_params_update(uint16_t conn_handle, const ble_gap_evt_conn_p
145144
}
146145
}
147146

148-
static void on_disconnected(uint16_t conn_handle, const ble_gap_evt_disconnected_t *evt)
149-
{
150-
}
151-
152147
static void on_ble_evt(const ble_evt_t *evt, void *ctx)
153148
{
154149
const uint16_t conn_handle = evt->evt.common_evt.conn_handle;
150+
const int idx = nrf_sdh_ble_idx_get(conn_handle);
151+
152+
__ASSERT(idx >= 0, "Invalid idx %d for conn_handle %#x, evt_id %#x",
153+
idx, conn_handle, evt->header.evt_id);
155154

156155
switch (evt->header.evt_id) {
157156
case BLE_GAP_EVT_CONNECTED:
158-
on_connected(conn_handle, &evt->evt.gap_evt.params.connected);
157+
on_connected(conn_handle, idx, &evt->evt.gap_evt.params.connected);
159158
break;
160159

161160
case BLE_GAP_EVT_DISCONNECTED:
162-
on_disconnected(conn_handle, &evt->evt.gap_evt.params.disconnected);
163161
break;
164162

165163
case BLE_GAP_EVT_CONN_PARAM_UPDATE:
166-
on_conn_params_update(
167-
conn_handle, &evt->evt.gap_evt.params.conn_param_update);
164+
on_conn_params_update(conn_handle, idx, &evt->evt.gap_evt.params.conn_param_update);
168165
break;
169166

170167
default:
@@ -198,15 +195,16 @@ NRF_SDH_STATE_EVT_OBSERVER(ble_conn_params_sdh_state_observer, on_state_evt, NUL
198195
int ble_conn_params_override(uint16_t conn_handle, const ble_gap_conn_params_t *conn_params)
199196
{
200197
int err;
198+
const int idx = nrf_sdh_ble_idx_get(conn_handle);
201199

202-
if (conn_handle > ARRAY_SIZE(links)) {
200+
if (idx < 0) {
203201
return -EINVAL;
204202
}
205203
if (!conn_params) {
206204
return -EFAULT;
207205
}
208206

209-
links[conn_handle].ppcp = *conn_params;
207+
links[idx].ppcp = *conn_params;
210208
err = sd_ble_gap_conn_param_update(conn_handle, conn_params);
211209
if (err) {
212210
return -EINVAL;

0 commit comments

Comments
 (0)