[backport v3.5-branch] Bluetooth: Controller: Few control procedure and compiler re-ordering fixes

subsys/bluetooth/controller/hci/hci.c

@@ -8505,7 +8505,7 @@ static void le_ltk_request(struct pdu_data *pdu_data, uint16_t handle,
 }
 
 static void encrypt_change(uint8_t err, uint16_t handle,
-			   struct net_buf *buf)
+			   struct net_buf *buf, bool encryption_on)
 {
 	struct bt_hci_evt_encrypt_change *ep;
 
@@ -8516,9 +8516,9 @@ static void encrypt_change(uint8_t err, uint16_t handle,
 	hci_evt_create(buf, BT_HCI_EVT_ENCRYPT_CHANGE, sizeof(*ep));
 	ep = net_buf_add(buf, sizeof(*ep));
 
-	ep->status = err;
+	ep->status = err ? err : (encryption_on ? err : BT_HCI_ERR_UNSPECIFIED);
 	ep->handle = sys_cpu_to_le16(handle);
-	ep->encrypt = !err ? 1 : 0;
+	ep->encrypt = encryption_on ? 1 : 0;
 }
 #endif /* CONFIG_BT_CTLR_LE_ENC */
 
@@ -8660,7 +8660,7 @@ static void encode_data_ctrl(struct node_rx_pdu *node_rx,
 		break;
 
 	case PDU_DATA_LLCTRL_TYPE_START_ENC_RSP:
-		encrypt_change(0x00, handle, buf);
+		encrypt_change(0x00, handle, buf, true);
 		break;
 #endif /* CONFIG_BT_CTLR_LE_ENC */
 
@@ -8677,7 +8677,7 @@ static void encode_data_ctrl(struct node_rx_pdu *node_rx,
 #if defined(CONFIG_BT_CTLR_LE_ENC)
 	case PDU_DATA_LLCTRL_TYPE_REJECT_IND:
 		encrypt_change(pdu_data->llctrl.reject_ind.error_code, handle,
-			       buf);
+			       buf, false);
 		break;
 #endif /* CONFIG_BT_CTLR_LE_ENC */
 

subsys/bluetooth/controller/ll_sw/ull.c

@@ -1951,12 +1951,15 @@ int ull_disable(void *lll)
 	if (!ull_ref_get(hdr)) {
 		return -EALREADY;
 	}
+	cpu_dmb(); /* Ensure synchronized data access */
 
 	k_sem_init(&sem, 0, 1);
 
 	hdr->disabled_param = &sem;
 	hdr->disabled_cb = disabled_cb;
 
+	cpu_dmb(); /* Ensure synchronized data access */
+
 	/* ULL_HIGH can run after we have call `ull_ref_get` and it can
 	 * decrement the ref count. Hence, handle this race condition by
 	 * ensuring that `disabled_cb` has been set while the ref count is still

subsys/bluetooth/controller/ll_sw/ull_llcp.c

@@ -284,24 +284,41 @@ void llcp_rx_node_retain(struct proc_ctx *ctx)
 {
 	LL_ASSERT(ctx->node_ref.rx);
 
-	/* Mark RX node to NOT release */
-	ctx->node_ref.rx->hdr.type = NODE_RX_TYPE_RETAIN;
+	/* Only retain if not already retained */
+	if (ctx->node_ref.rx->hdr.type != NODE_RX_TYPE_RETAIN) {
+		/* Mark RX node to NOT release */
+		ctx->node_ref.rx->hdr.type = NODE_RX_TYPE_RETAIN;
 
-	/* store link element reference to use once this node is moved up */
-	ctx->node_ref.rx->hdr.link = ctx->node_ref.link;
+		/* store link element reference to use once this node is moved up */
+		ctx->node_ref.rx->hdr.link = ctx->node_ref.link;
+	}
+}
+
+void llcp_rx_node_release(struct proc_ctx *ctx)
+{
+	LL_ASSERT(ctx->node_ref.rx);
+
+	/* Only release if retained */
+	if (ctx->node_ref.rx->hdr.type == NODE_RX_TYPE_RETAIN) {
+		/* Mark RX node to release and release */
+		ctx->node_ref.rx->hdr.type = NODE_RX_TYPE_RELEASE;
+		ll_rx_put_sched(ctx->node_ref.rx->hdr.link, ctx->node_ref.rx);
+	}
 }
 
 void llcp_nodes_release(struct ll_conn *conn, struct proc_ctx *ctx)
 {
 	if (ctx->node_ref.rx && ctx->node_ref.rx->hdr.type == NODE_RX_TYPE_RETAIN) {
 		/* RX node retained, so release */
 		ctx->node_ref.rx->hdr.link->mem = conn->llcp.rx_node_release;
+		ctx->node_ref.rx->hdr.type = NODE_RX_TYPE_RELEASE;
 		conn->llcp.rx_node_release = ctx->node_ref.rx;
 	}
 #if defined(CONFIG_BT_CTLR_PHY) && defined(CONFIG_BT_CTLR_DATA_LENGTH)
 	if (ctx->proc == PROC_PHY_UPDATE && ctx->data.pu.ntf_dle_node) {
 		/* RX node retained, so release */
 		ctx->data.pu.ntf_dle_node->hdr.link->mem = conn->llcp.rx_node_release;
+		ctx->data.pu.ntf_dle_node->hdr.type = NODE_RX_TYPE_RELEASE;
 		conn->llcp.rx_node_release = ctx->data.pu.ntf_dle_node;
 	}
 #endif
@@ -706,9 +723,6 @@ void ull_cp_release_nodes(struct ll_conn *conn)
 		hdr = &rx->hdr;
 		rx = hdr->link->mem;
 
-		/* Mark for buffer for release */
-		hdr->type = NODE_RX_TYPE_RELEASE;
-
 		/* enqueue rx node towards Thread */
 		ll_rx_put(hdr->link, hdr);
 	}

subsys/bluetooth/controller/ll_sw/ull_llcp_common.c

@@ -1135,6 +1135,7 @@ static void rp_comm_ntf(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t gene
 
 	/* Allocate ntf node */
 	ntf = ctx->node_ref.rx;
+	ctx->node_ref.rx = NULL;
 	LL_ASSERT(ntf);
 
 	/* This should be an 'old' RX node, so put/sched when done */

subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c

@@ -196,6 +196,22 @@ static bool cu_check_conn_parameters(struct ll_conn *conn, struct proc_ctx *ctx)
 }
 #endif /* CONFIG_BT_CTLR_CONN_PARAM_REQ */
 
+static bool cu_check_conn_ind_parameters(struct ll_conn *conn, struct proc_ctx *ctx)
+{
+	const uint16_t interval_max = ctx->data.cu.interval_max; /* unit 1.25ms */
+	const uint16_t timeout = ctx->data.cu.timeout; /* unit 10ms */
+	const uint16_t latency = ctx->data.cu.latency;
+
+	/* Valid conn_update_ind parameters */
+	return (interval_max >= CONN_INTERVAL_MIN(conn)) &&
+	       (interval_max <= CONN_UPDATE_CONN_INTV_4SEC) &&
+	       (latency <= CONN_UPDATE_LATENCY_MAX) &&
+	       (timeout >= CONN_UPDATE_TIMEOUT_100MS) &&
+	       (timeout <= CONN_UPDATE_TIMEOUT_32SEC) &&
+	       ((timeout * 4U) > /* *4U re. conn events is equivalent to *2U re. ms */
+		((latency + 1U) * interval_max));
+}
+
 static void cu_prepare_update_ind(struct ll_conn *conn, struct proc_ctx *ctx)
 {
 	ctx->data.cu.win_size = 1U;
@@ -585,8 +601,20 @@ static void lp_cu_st_wait_rx_conn_update_ind(struct ll_conn *conn, struct proc_c
 	switch (evt) {
 	case LP_CU_EVT_CONN_UPDATE_IND:
 		llcp_pdu_decode_conn_update_ind(ctx, param);
+
+		/* Invalid PDU, mark the connection for termination */
+		if (!cu_check_conn_ind_parameters(conn, ctx)) {
+			llcp_rr_set_incompat(conn, INCOMPAT_NO_COLLISION);
+			conn->llcp_terminate.reason_final = BT_HCI_ERR_INVALID_LL_PARAM;
+			lp_cu_complete(conn, ctx);
+			break;
+		}
+
+		llcp_rr_set_incompat(conn, INCOMPAT_RESERVED);
+
 		/* Keep RX node to use for NTF */
 		llcp_rx_node_retain(ctx);
+
 		ctx->state = LP_CU_STATE_WAIT_INSTANT;
 		break;
 	case LP_CU_EVT_UNKNOWN:
@@ -633,8 +661,7 @@ static void lp_cu_check_instant(struct ll_conn *conn, struct proc_ctx *ctx, uint
 			lp_cu_ntf_complete(conn, ctx, evt, param);
 		} else {
 			/* Release RX node kept for NTF */
-			ctx->node_ref.rx->hdr.type = NODE_RX_TYPE_RELEASE;
-			ll_rx_put_sched(ctx->node_ref.rx->hdr.link, ctx->node_ref.rx);
+			llcp_rx_node_release(ctx);
 			ctx->node_ref.rx = NULL;
 
 			lp_cu_complete(conn, ctx);
@@ -973,11 +1000,18 @@ static void rp_cu_st_wait_conn_param_req_available(struct ll_conn *conn, struct
 	case RP_CU_EVT_RUN:
 		if (cpr_active_is_set(conn)) {
 			ctx->state = RP_CU_STATE_WAIT_CONN_PARAM_REQ_AVAILABLE;
+
 			if (!llcp_rr_ispaused(conn) && llcp_tx_alloc_peek(conn, ctx)) {
 				/* We're good to reject immediately */
 				ctx->data.cu.rejected_opcode = PDU_DATA_LLCTRL_TYPE_CONN_PARAM_REQ;
 				ctx->data.cu.error = BT_HCI_ERR_UNSUPP_LL_PARAM_VAL;
 				rp_cu_send_reject_ext_ind(conn, ctx, evt, param);
+
+				/* Possibly retained rx node to be released as we won't need it */
+				llcp_rx_node_release(ctx);
+				ctx->node_ref.rx = NULL;
+
+				break;
 			}
 			/* In case we have to defer NTF */
 			llcp_rx_node_retain(ctx);
@@ -992,6 +1026,9 @@ static void rp_cu_st_wait_conn_param_req_available(struct ll_conn *conn, struct
 				rp_cu_conn_param_req_ntf(conn, ctx);
 				ctx->state = RP_CU_STATE_WAIT_CONN_PARAM_REQ_REPLY;
 			} else {
+				/* Possibly retained rx node to be released as we won't need it */
+				llcp_rx_node_release(ctx);
+				ctx->node_ref.rx = NULL;
 #if defined(CONFIG_BT_CTLR_USER_CPR_ANCHOR_POINT_MOVE)
 				/* Handle APM as a vendor specific user extension */
 				if (conn->lll.role == BT_HCI_ROLE_PERIPHERAL &&
@@ -1177,8 +1214,7 @@ static void rp_cu_check_instant(struct ll_conn *conn, struct proc_ctx *ctx, uint
 			cu_ntf(conn, ctx);
 		} else {
 			/* Release RX node kept for NTF */
-			ctx->node_ref.rx->hdr.type = NODE_RX_TYPE_RELEASE;
-			ll_rx_put_sched(ctx->node_ref.rx->hdr.link, ctx->node_ref.rx);
+			llcp_rx_node_release(ctx);
 			ctx->node_ref.rx = NULL;
 		}
 		rp_cu_complete(conn, ctx);
@@ -1198,19 +1234,27 @@ static void rp_cu_st_wait_rx_conn_update_ind(struct ll_conn *conn, struct proc_c
 		case BT_HCI_ROLE_PERIPHERAL:
 			llcp_pdu_decode_conn_update_ind(ctx, param);
 
-			if (is_instant_not_passed(ctx->data.cu.instant,
-						  ull_conn_event_counter(conn))) {
+			/* Valid PDU */
+			if (cu_check_conn_ind_parameters(conn, ctx)) {
+				if (is_instant_not_passed(ctx->data.cu.instant,
+							  ull_conn_event_counter(conn))) {
+					/* Keep RX node to use for NTF */
+					llcp_rx_node_retain(ctx);
 
-				llcp_rx_node_retain(ctx);
+					ctx->state = RP_CU_STATE_WAIT_INSTANT;
+
+					/* In case we only just received it in time */
+					rp_cu_check_instant(conn, ctx, evt, param);
+					break;
+				}
 
-				ctx->state = RP_CU_STATE_WAIT_INSTANT;
-				/* In case we only just received it in time */
-				rp_cu_check_instant(conn, ctx, evt, param);
-			} else {
 				conn->llcp_terminate.reason_final = BT_HCI_ERR_INSTANT_PASSED;
-				llcp_rr_complete(conn);
-				ctx->state = RP_CU_STATE_IDLE;
+			} else {
+				conn->llcp_terminate.reason_final = BT_HCI_ERR_INVALID_LL_PARAM;
 			}
+
+			llcp_rr_complete(conn);
+			ctx->state = RP_CU_STATE_IDLE;
 			break;
 		default:
 			/* Unknown role */

subsys/bluetooth/controller/ll_sw/ull_llcp_enc.c

@@ -225,6 +225,7 @@ static void lp_enc_ntf(struct ll_conn *conn, struct proc_ctx *ctx)
 
 	/* Piggy-back on RX node */
 	ntf = ctx->node_ref.rx;
+	ctx->node_ref.rx = NULL;
 	LL_ASSERT(ntf);
 
 	ntf->hdr.type = NODE_RX_TYPE_DC_PDU;
@@ -381,19 +382,29 @@ static void lp_enc_store_s(struct ll_conn *conn, struct proc_ctx *ctx, struct pd
 
 static inline uint8_t reject_error_code(struct pdu_data *pdu)
 {
+	uint8_t error;
+
 	if (pdu->llctrl.opcode == PDU_DATA_LLCTRL_TYPE_REJECT_IND) {
-		return pdu->llctrl.reject_ind.error_code;
+		error = pdu->llctrl.reject_ind.error_code;
 #if defined(CONFIG_BT_CTLR_EXT_REJ_IND)
 	} else if (pdu->llctrl.opcode == PDU_DATA_LLCTRL_TYPE_REJECT_EXT_IND) {
-		return pdu->llctrl.reject_ext_ind.error_code;
+		error = pdu->llctrl.reject_ext_ind.error_code;
 #endif /* CONFIG_BT_CTLR_EXT_REJ_IND */
 	} else {
 		/* Called with an invalid PDU */
 		LL_ASSERT(0);
 
 		/* Keep compiler happy */
-		return BT_HCI_ERR_UNSPECIFIED;
+		error = BT_HCI_ERR_UNSPECIFIED;
 	}
+
+	/* Check expected error code from the peer */
+	if (error != BT_HCI_ERR_PIN_OR_KEY_MISSING &&
+	    error != BT_HCI_ERR_UNSUPP_REMOTE_FEATURE) {
+		error = BT_HCI_ERR_UNSPECIFIED;
+	}
+
+	return error;
 }
 
 static void lp_enc_st_wait_rx_enc_rsp(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt,

subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h

@@ -413,6 +413,7 @@ void llcp_ntf_set_pending(struct ll_conn *conn);
 void llcp_ntf_clear_pending(struct ll_conn *conn);
 bool llcp_ntf_pending(struct ll_conn *conn);
 void llcp_rx_node_retain(struct proc_ctx *ctx);
+void llcp_rx_node_release(struct proc_ctx *ctx);
 
 /*
  * ULL -> LLL Interface

subsys/bluetooth/controller/ll_sw/ull_llcp_local.c

@@ -81,6 +81,12 @@ void llcp_lr_check_done(struct ll_conn *conn, struct proc_ctx *ctx)
 		ctx_header = llcp_lr_peek(conn);
 		LL_ASSERT(ctx_header == ctx);
 
+		/* If we have a node rx it must not be marked RETAIN as
+		 * the memory referenced would leak
+		 */
+		LL_ASSERT(ctx->node_ref.rx == NULL ||
+			  ctx->node_ref.rx->hdr.type != NODE_RX_TYPE_RETAIN);
+
 		lr_dequeue(conn);
 
 		llcp_proc_ctx_release(ctx);
@@ -312,6 +318,11 @@ void llcp_lr_rx(struct ll_conn *conn, struct proc_ctx *ctx, memq_link_t *link,
 		break;
 	}
 
+	/* If rx node was not retained clear reference */
+	if (ctx->node_ref.rx && ctx->node_ref.rx->hdr.type != NODE_RX_TYPE_RETAIN) {
+		ctx->node_ref.rx = NULL;
+	}
+
 	llcp_lr_check_done(conn, ctx);
 }
 

subsys/bluetooth/controller/ll_sw/ull_llcp_phy.c

@@ -433,6 +433,7 @@ static void pu_ntf(struct ll_conn *conn, struct proc_ctx *ctx)
 
 	/* Piggy-back on stored RX node */
 	ntf = ctx->node_ref.rx;
+	ctx->node_ref.rx = NULL;
 	LL_ASSERT(ntf);
 
 	if (ctx->data.pu.ntf_pu) {
@@ -449,15 +450,9 @@ static void pu_ntf(struct ll_conn *conn, struct proc_ctx *ctx)
 	}
 
 	/* Enqueue notification towards LL */
-#if defined(CONFIG_BT_CTLR_DATA_LENGTH)
-	/* only 'put' as the 'sched' is handled when handling DLE ntf */
-	ll_rx_put(ntf->hdr.link, ntf);
-#else
 	ll_rx_put_sched(ntf->hdr.link, ntf);
-#endif /* CONFIG_BT_CTLR_DATA_LENGTH */
 
 	ctx->data.pu.ntf_pu = 0;
-	ctx->node_ref.rx = NULL;
 }
 
 #if defined(CONFIG_BT_CTLR_DATA_LENGTH)

subsys/bluetooth/controller/ll_sw/ull_llcp_remote.c

@@ -118,6 +118,12 @@ void llcp_rr_check_done(struct ll_conn *conn, struct proc_ctx *ctx)
 		ctx_header = llcp_rr_peek(conn);
 		LL_ASSERT(ctx_header == ctx);
 
+		/* If we have a node rx it must not be marked RETAIN as
+		 * the memory referenced would leak
+		 */
+		LL_ASSERT(ctx->node_ref.rx == NULL ||
+			  ctx->node_ref.rx->hdr.type != NODE_RX_TYPE_RETAIN);
+
 		rr_dequeue(conn);
 
 		llcp_proc_ctx_release(ctx);
@@ -307,6 +313,12 @@ void llcp_rr_rx(struct ll_conn *conn, struct proc_ctx *ctx, memq_link_t *link,
 		LL_ASSERT(0);
 		break;
 	}
+
+	/* If rx node was not retained clear reference */
+	if (ctx->node_ref.rx && ctx->node_ref.rx->hdr.type != NODE_RX_TYPE_RETAIN) {
+		ctx->node_ref.rx = NULL;
+	}
+
 	llcp_rr_check_done(conn, ctx);
 }