Bluetooth: Mesh: Fix err in mod_app_list packing

Fixes erroneous packing/unpacking of model app list messages
in the configuration client and server. According to the mesh 1.1
protcol spec (4.3.1.1) two app indexes shall be packed in a 3 octet
interleaved format. The current implementation packs them in 4 octets.

This commit also provide a helper function for unpacking key indexes
as public API to facilitate future config model callback API.

Signed-off-by: Anders Storrø <anders.storro@nordicsemi.no>

include/zephyr/bluetooth/mesh/cfg_cli.h

@@ -1644,6 +1644,19 @@ struct bt_mesh_comp_p1_model_item *bt_mesh_comp_p1_item_pull(
 struct bt_mesh_comp_p1_ext_item *bt_mesh_comp_p1_pull_ext_item(
 	struct bt_mesh_comp_p1_model_item *item, struct bt_mesh_comp_p1_ext_item *ext_item);
 
+/** @brief Unpack a list of key index entries from a buffer.
+ *
+ * On success, @c dst_cnt is set to the amount of unpacked key index entries.
+ *
+ * @param buf Message buffer containing encoded AppKey or NetKey Indexes.
+ * @param dst_arr Destination array for the unpacked list.
+ * @param dst_cnt Size of the destination array.
+ *
+ * @return 0 on success.
+ * @return -EMSGSIZE if dst_arr size is to small to parse full message.
+ */
+int bt_mesh_key_idx_unpack_list(struct net_buf_simple *buf, uint16_t *dst_arr, size_t *dst_cnt);
+
 /** @cond INTERNAL_HIDDEN */
 extern const struct bt_mesh_model_op bt_mesh_cfg_cli_op[];
 extern const struct bt_mesh_model_cb bt_mesh_cfg_cli_cb;

subsys/bluetooth/mesh/cfg_cli.c

@@ -308,38 +308,48 @@ struct net_key_list_param {
 	size_t *key_cnt;
 };
 
+int bt_mesh_key_idx_unpack_list(struct net_buf_simple *buf, uint16_t *dst_arr,
+				      size_t *dst_cnt)
+{
+	size_t i;
+
+	if (!dst_cnt) {
+		return 0;
+	}
+
+	for (i = 0; (i + 1) < *dst_cnt && buf->len >= 3; i += 2) {
+		key_idx_unpack_pair(buf, &dst_arr[i], &dst_arr[i + 1]);
+	}
+
+	if (i < *dst_cnt && buf->len >= 2) {
+		dst_arr[i++] = net_buf_simple_pull_le16(buf) & 0xfff;
+	}
+
+	*dst_cnt = i;
+
+	return buf->len > 0 ? -EMSGSIZE : 0;
+}
+
 static int net_key_list(struct bt_mesh_model *model,
 			struct bt_mesh_msg_ctx *ctx,
 			struct net_buf_simple *buf)
 {
 	struct net_key_list_param *param;
-	int i;
 
 	LOG_DBG("net_idx 0x%04x app_idx 0x%04x src 0x%04x len %u: %s", ctx->net_idx, ctx->app_idx,
 		ctx->addr, buf->len, bt_hex(buf->data, buf->len));
 
-	if (bt_mesh_msg_ack_ctx_match(&cli->ack_ctx, OP_NET_KEY_LIST, ctx->addr,
+	if (bt_mesh_msg_ack_ctx_match(&cli->ack_ctx, OP_NET_KEY_LIST, ctx->addr, (void **)&param)) {
-				      (void **)&param)) {
 
 		if (param->keys && param->key_cnt) {
 
-			for (i = 0; i < *param->key_cnt && buf->len >= 3; i += 2) {
+			int err = bt_mesh_key_idx_unpack_list(buf, param->keys, param->key_cnt);
-				key_idx_unpack(buf, &param->keys[i],
-						&param->keys[i + 1]);
-			}
 
-			if (i < *param->key_cnt && buf->len >= 2) {
+			if (err) {
-				param->keys[i++] =
-					net_buf_simple_pull_le16(buf) & 0xfff;
-			}
-
-			if (buf->len > 0) {
 				LOG_ERR("The message size for the application opcode is "
 					"incorrect.");
-				return -EMSGSIZE;
+				return err;
 			}
-
-			*param->key_cnt = i;
 		}
 
 		bt_mesh_msg_ack_ctx_rx(&cli->ack_ctx);
@@ -392,7 +402,7 @@ static int app_key_status(struct bt_mesh_model *model,
 		ctx->addr, buf->len, bt_hex(buf->data, buf->len));
 
 	status = net_buf_simple_pull_u8(buf);
-	key_idx_unpack(buf, &net_idx, &app_idx);
+	key_idx_unpack_pair(buf, &net_idx, &app_idx);
 
 	if (bt_mesh_msg_ack_ctx_match(&cli->ack_ctx, OP_APP_KEY_STATUS, ctx->addr,
 				      (void **)&param)) {
@@ -435,7 +445,6 @@ static int app_key_list(struct bt_mesh_model *model,
 	struct app_key_list_param *param;
 	uint16_t net_idx;
 	uint8_t status;
-	int i;
 
 	LOG_DBG("net_idx 0x%04x app_idx 0x%04x src 0x%04x len %u: %s", ctx->net_idx, ctx->app_idx,
 		ctx->addr, buf->len, bt_hex(buf->data, buf->len));
@@ -453,22 +462,13 @@ static int app_key_list(struct bt_mesh_model *model,
 
 		if (param->keys && param->key_cnt) {
 
-			for (i = 0; i < *param->key_cnt && buf->len >= 3; i += 2) {
+			int err = bt_mesh_key_idx_unpack_list(buf, param->keys, param->key_cnt);
-				key_idx_unpack(buf, &param->keys[i],
-						&param->keys[i + 1]);
-			}
 
-			if (i < *param->key_cnt && buf->len == 2) {
+			if (err) {
-				param->keys[i++] = net_buf_simple_pull_le16(buf) & 0xfff;
-			}
-
-			if (buf->len > 0U) {
 				LOG_ERR("The message size for the application opcode is "
 					"incorrect.");
-				return -EMSGSIZE;
+				return err;
 			}
-
-			*param->key_cnt = i;
 		}
 
 		if (param->status) {
@@ -555,20 +555,14 @@ struct mod_member_list_param {
 	size_t *member_cnt;
 };
 
-static int mod_member_list_handle(struct bt_mesh_msg_ctx *ctx,
+static int mod_sub_list_handle(struct bt_mesh_msg_ctx *ctx, struct net_buf_simple *buf, uint16_t op,
-				   struct net_buf_simple *buf, uint16_t op,
+			       bool vnd)
-				   bool vnd)
 {
 	struct mod_member_list_param *param;
 	uint16_t elem_addr, mod_id, cid;
 	uint8_t status;
 	int i;
 
-	if ((vnd && buf->len < 7U) || (buf->len < 5U)) {
-		LOG_ERR("The message size for the application opcode is incorrect.");
-		return -EMSGSIZE;
-	}
-
 	status = net_buf_simple_pull_u8(buf);
 	elem_addr = net_buf_simple_pull_le16(buf);
 	if (vnd) {
@@ -577,11 +571,10 @@ static int mod_member_list_handle(struct bt_mesh_msg_ctx *ctx,
 
 	mod_id = net_buf_simple_pull_le16(buf);
 
-	if (bt_mesh_msg_ack_ctx_match(&cli->ack_ctx, op, ctx->addr,
+	if (bt_mesh_msg_ack_ctx_match(&cli->ack_ctx, op, ctx->addr, (void **)&param)) {
-				      (void **)&param)) {
 
 		if (param->elem_addr != elem_addr || param->mod_id != mod_id ||
-				(vnd && param->cid != cid)) {
+		    (vnd && param->cid != cid)) {
 			LOG_WRN("Model Member List parameters did not match");
 			return -ENOENT;
 		}
@@ -609,13 +602,57 @@ static int mod_member_list_handle(struct bt_mesh_msg_ctx *ctx,
 	return 0;
 }
 
+static int mod_app_list_handle(struct bt_mesh_msg_ctx *ctx, struct net_buf_simple *buf, uint16_t op,
+			       bool vnd)
+{
+	struct mod_member_list_param *param;
+	uint16_t elem_addr, mod_id, cid;
+	uint8_t status;
+
+	status = net_buf_simple_pull_u8(buf);
+	elem_addr = net_buf_simple_pull_le16(buf);
+	if (vnd) {
+		cid = net_buf_simple_pull_le16(buf);
+	}
+
+	mod_id = net_buf_simple_pull_le16(buf);
+
+	if (bt_mesh_msg_ack_ctx_match(&cli->ack_ctx, op, ctx->addr, (void **)&param)) {
+
+		if (param->elem_addr != elem_addr || param->mod_id != mod_id ||
+		    (vnd && param->cid != cid)) {
+			LOG_WRN("Model Member List parameters did not match");
+			return -ENOENT;
+		}
+
+		if (param->member_cnt && param->members) {
+
+			int err =
+				bt_mesh_key_idx_unpack_list(buf, param->members, param->member_cnt);
+
+			if (err) {
+				LOG_ERR("The message size for the application opcode is "
+					"incorrect.");
+				return err;
+			}
+		}
+
+		if (param->status) {
+			*param->status = status;
+		}
+
+		bt_mesh_msg_ack_ctx_rx(&cli->ack_ctx);
+	}
+	return 0;
+}
+
 static int mod_app_list(struct bt_mesh_model *model,
 			struct bt_mesh_msg_ctx *ctx, struct net_buf_simple *buf)
 {
 	LOG_DBG("net_idx 0x%04x app_idx 0x%04x src 0x%04x len %u: %s", ctx->net_idx, ctx->app_idx,
 		ctx->addr, buf->len, bt_hex(buf->data, buf->len));
 
-	return mod_member_list_handle(ctx, buf, OP_SIG_MOD_APP_LIST, false);
+	return mod_app_list_handle(ctx, buf, OP_SIG_MOD_APP_LIST, false);
 }
 
 static int mod_app_list_vnd(struct bt_mesh_model *model,
@@ -625,7 +662,7 @@ static int mod_app_list_vnd(struct bt_mesh_model *model,
 	LOG_DBG("net_idx 0x%04x app_idx 0x%04x src 0x%04x len %u: %s", ctx->net_idx, ctx->app_idx,
 		ctx->addr, buf->len, bt_hex(buf->data, buf->len));
 
-	return mod_member_list_handle(ctx, buf, OP_VND_MOD_APP_LIST, true);
+	return mod_app_list_handle(ctx, buf, OP_VND_MOD_APP_LIST, true);
 }
 
 struct mod_pub_param {
@@ -777,7 +814,7 @@ static int mod_sub_list(struct bt_mesh_model *model,
 	LOG_DBG("net_idx 0x%04x app_idx 0x%04x src 0x%04x len %u: %s", ctx->net_idx, ctx->app_idx,
 		ctx->addr, buf->len, bt_hex(buf->data, buf->len));
 
-	return mod_member_list_handle(ctx, buf, OP_MOD_SUB_LIST, false);
+	return mod_sub_list_handle(ctx, buf, OP_MOD_SUB_LIST, false);
 }
 
 static int mod_sub_list_vnd(struct bt_mesh_model *model,
@@ -787,7 +824,7 @@ static int mod_sub_list_vnd(struct bt_mesh_model *model,
 	LOG_DBG("net_idx 0x%04x app_idx 0x%04x src 0x%04x len %u: %s", ctx->net_idx, ctx->app_idx,
 		ctx->addr, buf->len, bt_hex(buf->data, buf->len));
 
-	return mod_member_list_handle(ctx, buf, OP_MOD_SUB_LIST_VND, true);
+	return mod_sub_list_handle(ctx, buf, OP_MOD_SUB_LIST_VND, true);
 }
 
 struct hb_sub_param {
@@ -1315,7 +1352,7 @@ int bt_mesh_cfg_cli_app_key_add(uint16_t net_idx, uint16_t addr, uint16_t key_ne
 	};
 
 	bt_mesh_model_msg_init(&msg, OP_APP_KEY_ADD);
-	key_idx_pack(&msg, key_net_idx, key_app_idx);
+	key_idx_pack_pair(&msg, key_net_idx, key_app_idx);
 	net_buf_simple_add_mem(&msg, app_key, 16);
 
 	return bt_mesh_msg_ackd_send(cli->model, &ctx, &msg, !status ? NULL : &rsp);
@@ -1339,7 +1376,7 @@ int bt_mesh_cfg_cli_app_key_update(uint16_t net_idx, uint16_t addr, uint16_t key
 	};
 
 	bt_mesh_model_msg_init(&msg, OP_APP_KEY_UPDATE);
-	key_idx_pack(&msg, key_net_idx, key_app_idx);
+	key_idx_pack_pair(&msg, key_net_idx, key_app_idx);
 	net_buf_simple_add_mem(&msg, app_key, 16);
 
 	return bt_mesh_msg_ackd_send(cli->model, &ctx, &msg, !status ? NULL : &rsp);
@@ -1409,7 +1446,7 @@ int bt_mesh_cfg_cli_app_key_del(uint16_t net_idx, uint16_t addr, uint16_t key_ne
 	};
 
 	bt_mesh_model_msg_init(&msg, OP_APP_KEY_DEL);
-	key_idx_pack(&msg, key_net_idx, key_app_idx);
+	key_idx_pack_pair(&msg, key_net_idx, key_app_idx);
 
 	return bt_mesh_msg_ackd_send(cli->model, &ctx, &msg, !status ? NULL : &rsp);
 }

subsys/bluetooth/mesh/cfg_srv.c

@@ -280,6 +280,28 @@ static uint8_t mod_unbind(struct bt_mesh_model *model, uint16_t key_idx, bool st
 	return STATUS_SUCCESS;
 }
 
+static void key_idx_pack_list(struct net_buf_simple *buf, uint16_t *arr, size_t cnt)
+{
+	uint16_t *idx = NULL;
+
+	for (int i = 0; i < cnt; i++) {
+		if (arr[i] != BT_MESH_KEY_UNUSED) {
+			if (!idx) {
+				idx = &arr[i];
+				continue;
+			}
+
+			key_idx_pack_pair(buf, *idx, arr[i]);
+			idx = NULL;
+		}
+	}
+
+	if (idx) {
+		net_buf_simple_add_le16(buf, *idx);
+	}
+
+}
+
 static int send_app_key_status(struct bt_mesh_model *model,
 			       struct bt_mesh_msg_ctx *ctx,
 			       uint8_t status,
@@ -289,7 +311,7 @@ static int send_app_key_status(struct bt_mesh_model *model,
 
 	bt_mesh_model_msg_init(&msg, OP_APP_KEY_STATUS);
 	net_buf_simple_add_u8(&msg, status);
-	key_idx_pack(&msg, net_idx, app_idx);
+	key_idx_pack_pair(&msg, net_idx, app_idx);
 
 	if (bt_mesh_model_send(model, ctx, &msg, NULL, NULL)) {
 		LOG_ERR("Unable to send App Key Status response");
@@ -305,7 +327,7 @@ static int app_key_add(struct bt_mesh_model *model,
 	uint16_t key_net_idx, key_app_idx;
 	uint8_t status;
 
-	key_idx_unpack(buf, &key_net_idx, &key_app_idx);
+	key_idx_unpack_pair(buf, &key_net_idx, &key_app_idx);
 
 	LOG_DBG("AppIdx 0x%04x NetIdx 0x%04x", key_app_idx, key_net_idx);
 
@@ -321,7 +343,7 @@ static int app_key_update(struct bt_mesh_model *model,
 	uint16_t key_net_idx, key_app_idx;
 	uint8_t status;
 
-	key_idx_unpack(buf, &key_net_idx, &key_app_idx);
+	key_idx_unpack_pair(buf, &key_net_idx, &key_app_idx);
 
 	LOG_DBG("AppIdx 0x%04x NetIdx 0x%04x", key_app_idx, key_net_idx);
 
@@ -357,7 +379,7 @@ static int app_key_del(struct bt_mesh_model *model,
 	uint16_t key_net_idx, key_app_idx;
 	uint8_t status;
 
-	key_idx_unpack(buf, &key_net_idx, &key_app_idx);
+	key_idx_unpack_pair(buf, &key_net_idx, &key_app_idx);
 
 	LOG_DBG("AppIdx 0x%04x NetIdx 0x%04x", key_app_idx, key_net_idx);
 
@@ -379,7 +401,6 @@ static int app_key_get(struct bt_mesh_model *model,
 	uint16_t get_idx;
 	uint8_t status;
 	ssize_t count;
-	int i;
 
 	get_idx = net_buf_simple_pull_le16(buf);
 	if (get_idx > 0xfff) {
@@ -409,13 +430,7 @@ static int app_key_get(struct bt_mesh_model *model,
 		count = ARRAY_SIZE(app_idx);
 	}
 
-	for (i = 0; i < count - 1; i += 2) {
+	key_idx_pack_list(&msg, app_idx, count);
-		key_idx_pack(&msg, app_idx[i], app_idx[i + 1]);
-	}
-
-	if (i < count) {
-		net_buf_simple_add_le16(&msg, app_idx[i]);
-	}
 
 send_status:
 	if (bt_mesh_model_send(model, ctx, &msg, NULL, NULL)) {
@@ -1731,7 +1746,6 @@ static int net_key_get(struct bt_mesh_model *model,
 				 IDX_LEN(CONFIG_BT_MESH_SUBNET_COUNT));
 	uint16_t net_idx[CONFIG_BT_MESH_SUBNET_COUNT];
 	ssize_t count;
-	int i;
 
 	bt_mesh_model_msg_init(&msg, OP_NET_KEY_LIST);
 
@@ -1740,13 +1754,7 @@ static int net_key_get(struct bt_mesh_model *model,
 		count = ARRAY_SIZE(net_idx);
 	}
 
-	for (i = 0; i < count - 1; i += 2) {
+	key_idx_pack_list(&msg, net_idx, count);
-		key_idx_pack(&msg, net_idx[i], net_idx[i + 1]);
-	}
-
-	if (i < count) {
-		net_buf_simple_add_le16(&msg, net_idx[i]);
-	}
 
 	if (bt_mesh_model_send(model, ctx, &msg, NULL, NULL)) {
 		LOG_ERR("Unable to send NetKey List");
@@ -2037,13 +2045,7 @@ send_list:
 	}
 
 	if (mod) {
-		int i;
+		key_idx_pack_list(&msg, mod->keys, mod->keys_cnt);
-
-		for (i = 0; i < mod->keys_cnt; i++) {
-			if (mod->keys[i] != BT_MESH_KEY_UNUSED) {
-				net_buf_simple_add_le16(&msg, mod->keys[i]);
-			}
-		}
 	}
 
 	if (bt_mesh_model_send(model, ctx, &msg, NULL, NULL)) {

subsys/bluetooth/mesh/foundation.h

@@ -154,15 +154,13 @@ void bt_mesh_attention(struct bt_mesh_model *model, uint8_t time);
 
 #include <zephyr/sys/byteorder.h>
 
-static inline void key_idx_pack(struct net_buf_simple *buf,
+static inline void key_idx_pack_pair(struct net_buf_simple *buf, uint16_t idx1, uint16_t idx2)
-				uint16_t idx1, uint16_t idx2)
 {
 	net_buf_simple_add_le16(buf, idx1 | ((idx2 & 0x00f) << 12));
 	net_buf_simple_add_u8(buf, idx2 >> 4);
 }
 
-static inline void key_idx_unpack(struct net_buf_simple *buf,
+static inline void key_idx_unpack_pair(struct net_buf_simple *buf, uint16_t *idx1, uint16_t *idx2)
-				  uint16_t *idx1, uint16_t *idx2)
 {
 	*idx1 = sys_get_le16(&buf->data[0]) & 0xfff;
 	*idx2 = sys_get_le16(&buf->data[1]) >> 4;