From 554e063421f07fe2a9028ea2b389a503e1d24ee0 Mon Sep 17 00:00:00 2001 From: Andrzej Kaczmarek Date: Thu, 13 Jun 2024 18:32:24 +0200 Subject: [PATCH] nimble/ll: Fix encryption pause/restart procedure This fixes 2 issues in encrpytion pause/restart procedure (i.e. key refresh) initiated in central role: 1. We transition to "paused" encryption state when LL_PAUSE_ENC_RSP is txd, but that is done after we try to enqueue LL_ENC_REQ. This means LL_ENC_REQ is put at the end of tx queue. By conicidence this makes order of PDUs correct when there's only LL_PAUSE_ENC_RSP on tx queue, but it fails if there was an ACL packet already queued. In such case LL_ENC_REQ is queued after that ACL packet which means neither will be sent - ACL packet cannot be sent because we have no encryption, LL_ENC_REQ cannot be sent because there's ACL packet in front. 2. We do not check if LL_ENC_REQ was properly queued (i.e. if mbuf was allocated for PDU) so it may happen that LL_ENC_REQ will never be queued. In both scenarios the connection will timeout eventually since encryption restart procedure cannot be completed. This fix ensures that LL_ENC_REQ is queued properly by using a "pending" flag that is evaluated when checking for pending procedures. --- .../include/controller/ble_ll_conn.h | 1 + nimble/controller/src/ble_ll_ctrl.c | 38 ++++++++++--------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/nimble/controller/include/controller/ble_ll_conn.h b/nimble/controller/include/controller/ble_ll_conn.h index 88e7934b1f..7d6af874dd 100644 --- a/nimble/controller/include/controller/ble_ll_conn.h +++ b/nimble/controller/include/controller/ble_ll_conn.h @@ -108,6 +108,7 @@ struct ble_ll_conn_sm_flags { uint32_t encrypted : 1; uint32_t encrypt_ltk_req : 1; uint32_t encrypt_event_sent : 1; + uint32_t pending_encrypt_restart : 1; uint32_t version_ind_txd : 1; uint32_t version_ind_rxd : 1; uint32_t features_rxd : 1; diff --git a/nimble/controller/src/ble_ll_ctrl.c b/nimble/controller/src/ble_ll_ctrl.c index d2b8a31b7b..da12a5f313 100644 --- a/nimble/controller/src/ble_ll_ctrl.c +++ b/nimble/controller/src/ble_ll_ctrl.c @@ -2684,6 +2684,9 @@ ble_ll_ctrl_proc_start(struct ble_ll_conn_sm *connsm, int ctrl_proc, void ble_ll_ctrl_chk_proc_start(struct ble_ll_conn_sm *connsm) { +#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LE_ENCRYPTION) + struct os_mbuf *om; +#endif int i; /* XXX: TODO new rules! Cannot start certain control procedures if other @@ -2706,6 +2709,22 @@ ble_ll_ctrl_chk_proc_start(struct ble_ll_conn_sm *connsm) return; } +#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LE_ENCRYPTION) + if (connsm->flags.pending_encrypt_restart) { + /* This flag should only be set after LL_PAUSE_ENC_RSP was txd from + * central and there should be already active encryption procedure + * ongoing. We need to restart encryption to complete key refresh. + */ + BLE_LL_ASSERT(connsm->cur_ctrl_proc == BLE_LL_CTRL_PROC_ENCRYPT); + BLE_LL_ASSERT(IS_PENDING_CTRL_PROC(connsm, BLE_LL_CTRL_PROC_ENCRYPT)); + + om = ble_ll_ctrl_proc_init(connsm, BLE_LL_CTRL_PROC_ENCRYPT, NULL); + if (om) { + connsm->flags.pending_encrypt_restart = 0; + } + } +#endif + /* If there is a running procedure or no pending, do nothing */ if ((connsm->cur_ctrl_proc == BLE_LL_CTRL_PROC_IDLE) && (connsm->pending_ctrl_procs != 0)) { @@ -2755,9 +2774,6 @@ ble_ll_ctrl_rx_pdu(struct ble_ll_conn_sm *connsm, struct os_mbuf *om) uint8_t *dptr; uint8_t *rspbuf; uint8_t *rspdata; -#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LE_ENCRYPTION) - int restart_encryption; -#endif int rc = 0; uint8_t rsp_opcode = 0; @@ -2802,10 +2818,6 @@ ble_ll_ctrl_rx_pdu(struct ble_ll_conn_sm *connsm, struct os_mbuf *om) ble_ll_trace_u32x2(BLE_LL_TRACE_ID_CTRL_RX, opcode, len); -#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LE_ENCRYPTION) - restart_encryption = 0; -#endif - /* If opcode comes from reserved value or CtrlData fields is invalid * we shall respond with LL_UNKNOWN_RSP */ @@ -2956,9 +2968,6 @@ ble_ll_ctrl_rx_pdu(struct ble_ll_conn_sm *connsm, struct os_mbuf *om) break; case BLE_LL_CTRL_PAUSE_ENC_RSP: rsp_opcode = ble_ll_ctrl_rx_pause_enc_rsp(connsm); - if (rsp_opcode == BLE_LL_CTRL_PAUSE_ENC_RSP) { - restart_encryption = 1; - } break; #endif case BLE_LL_CTRL_PING_REQ: @@ -3032,13 +3041,6 @@ ble_ll_ctrl_rx_pdu(struct ble_ll_conn_sm *connsm, struct os_mbuf *om) } len = g_ble_ll_ctrl_pkt_lengths[rsp_opcode] + 1; ble_ll_conn_enqueue_pkt(connsm, om, BLE_LL_LLID_CTRL, len); -#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LE_ENCRYPTION) - if (restart_encryption) { - /* XXX: what happens if this fails? Meaning we cant allocate - mbuf? */ - ble_ll_ctrl_proc_init(connsm, BLE_LL_CTRL_PROC_ENCRYPT, NULL); - } -#endif } #if MYNEWT_VAL(BLE_LL_CONN_INIT_AUTO_DLE) @@ -3206,6 +3208,8 @@ ble_ll_ctrl_tx_done(struct os_mbuf *txpdu, struct ble_ll_conn_sm *connsm) case BLE_LL_CTRL_PAUSE_ENC_RSP: if (connsm->conn_role == BLE_LL_CONN_ROLE_PERIPHERAL) { connsm->enc_data.enc_state = CONN_ENC_S_PAUSE_ENC_RSP_WAIT; + } else { + connsm->flags.pending_encrypt_restart = 1; } break; #endif