[PATCH BlueZ v2 0/4] Allow to reattach with new composition data

[PATCH BlueZ v2 0/4] Allow to reattach with new composition data

[Jakub Witowski]

This patch allows the application to modify the CID, PID and VID in the composition data.

Version 2: Do not allow to change CRPL in the composition data.
Additionaly verify the device key when updating comp data and remove
unused function in the 3rd patch.

Version 1: According the Mesh Profile (2.3.4 Elements) the modification of fields
other than "Elements" is not prohibited.

Also in my opinion (as you can see in the 1st patch), there is no need to use pointer to
the node_composition struct. The static is more clear and less problematic.

Jakub Witowski (4):
  mesh: use static node_comp instead of the pointer
  mesh: add cid/pid/vid setter
  mesh: remove unused node_set_device_key()
  mesh: allow to reattach with new composition data

 mesh/mesh-config-json.c |  40 ++++++++++++----
 mesh/mesh-config.h      |   2 +
 mesh/node.c             | 100 +++++++++++++++++++++++++---------------
 mesh/node.h             |   1 -
 4 files changed, 96 insertions(+), 47 deletions(-)

-- 
2.20.1

[PATCH BlueZ v2 1/4] mesh: use static node_comp instead of the pointer

[Jakub Witowski]

There is no need to use the pointer to the node_comp data.
This pach uses static node_comp instead.
---
 mesh/node.c | 56 +++++++++++++++++++++++------------------------------
 1 file changed, 24 insertions(+), 32 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index de6e74c4f..6fe70742d 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -90,7 +90,7 @@ struct mesh_node {
 	uint32_t seq_number;
 	bool provisioner;
 	uint16_t primary;
-	struct node_composition *comp;
+	struct node_composition comp;
 	struct {
 		uint16_t interval;
 		uint8_t cnt;
@@ -336,7 +336,6 @@ static void free_node_resources(void *data)
 	free_node_dbus_resources(node);
 
 	mesh_net_free(node->net);
-	l_free(node->comp);
 	l_free(node->storage_dir);
 	l_free(node);
 }
@@ -503,11 +502,10 @@ static bool init_from_storage(struct mesh_config_node *db_node,
 
 	l_queue_push_tail(nodes, node);
 
-	node->comp = l_new(struct node_composition, 1);
-	node->comp->cid = db_node->cid;
-	node->comp->pid = db_node->pid;
-	node->comp->vid = db_node->vid;
-	node->comp->crpl = db_node->crpl;
+	node->comp.cid = db_node->cid;
+	node->comp.pid = db_node->pid;
+	node->comp.vid = db_node->vid;
+	node->comp.crpl = db_node->crpl;
 	node->lpn = db_node->modes.lpn;
 
 	node->proxy = db_node->modes.proxy;
@@ -753,7 +751,7 @@ uint16_t node_get_crpl(struct mesh_node *node)
 	if (!node)
 		return 0;
 
-	return node->comp->crpl;
+	return node->comp.crpl;
 }
 
 uint8_t node_relay_mode_get(struct mesh_node *node, uint8_t *count,
@@ -886,18 +884,18 @@ uint16_t node_generate_comp(struct mesh_node *node, uint8_t *buf, uint16_t sz)
 	uint16_t num_ele = 0;
 	const struct l_queue_entry *ele_entry;
 
-	if (!node || !node->comp || sz < MIN_COMP_SIZE)
+	if (!node || sz < MIN_COMP_SIZE)
 		return 0;
 
 	n = 0;
 
-	l_put_le16(node->comp->cid, buf + n);
+	l_put_le16(node->comp.cid, buf + n);
 	n += 2;
-	l_put_le16(node->comp->pid, buf + n);
+	l_put_le16(node->comp.pid, buf + n);
 	n += 2;
-	l_put_le16(node->comp->vid, buf + n);
+	l_put_le16(node->comp.vid, buf + n);
 	n += 2;
-	l_put_le16(node->comp->crpl, buf + n);
+	l_put_le16(node->comp.crpl, buf + n);
 	n += 2;
 
 	features = 0;
@@ -1198,10 +1196,10 @@ static void convert_node_to_storage(struct mesh_node *node,
 {
 	const struct l_queue_entry *entry;
 
-	db_node->cid = node->comp->cid;
-	db_node->pid = node->comp->pid;
-	db_node->vid = node->comp->vid;
-	db_node->crpl = node->comp->crpl;
+	db_node->cid = node->comp.cid;
+	db_node->pid = node->comp.pid;
+	db_node->vid = node->comp.vid;
+	db_node->crpl = node->comp.crpl;
 	db_node->modes.lpn = node->lpn;
 	db_node->modes.proxy = node->proxy;
 
@@ -1285,29 +1283,28 @@ static bool get_app_properties(struct mesh_node *node, const char *path,
 
 	l_debug("path %s", path);
 
-	node->comp = l_new(struct node_composition, 1);
-	node->comp->crpl = mesh_get_crpl();
+	node->comp.crpl = mesh_get_crpl();
 
 	while (l_dbus_message_iter_next_entry(properties, &key, &variant)) {
 		if (!cid && !strcmp(key, "CompanyID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->cid))
-				goto fail;
+							&node->comp.cid))
+				return false;
 			cid = true;
 			continue;
 		}
 
 		if (!pid && !strcmp(key, "ProductID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->pid))
-				goto fail;
+							&node->comp.pid))
+				return false;
 			pid = true;
 			continue;
 		}
 
 		if (!vid && !strcmp(key, "VersionID")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->vid))
+							&node->comp.vid))
 				return false;
 			vid = true;
 			continue;
@@ -1315,21 +1312,16 @@ static bool get_app_properties(struct mesh_node *node, const char *path,
 
 		if (!strcmp(key, "CRPL")) {
 			if (!l_dbus_message_iter_get_variant(&variant, "q",
-							&node->comp->crpl))
-				goto fail;
+							&node->comp.crpl))
+				return false;
 			continue;
 		}
 	}
 
 	if (!cid || !pid || !vid)
-		goto fail;
+		return false;
 
 	return true;
-fail:
-	l_free(node->comp);
-	node->comp = NULL;
-
-	return false;
 }
 
 static bool add_local_node(struct mesh_node *node, uint16_t unicast, bool kr,
-- 
2.20.1

[PATCH BlueZ v2 2/4] mesh: add cid/pid/vid setter

[Jakub Witowski]

This patch adds the setter for the CID PID and VID.
---
 mesh/mesh-config-json.c | 40 ++++++++++++++++++++++++++++++++--------
 mesh/mesh-config.h      |  2 ++
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/mesh/mesh-config-json.c b/mesh/mesh-config-json.c
index 5855149e3..0574c166e 100644
--- a/mesh/mesh-config-json.c
+++ b/mesh/mesh-config-json.c
@@ -1456,6 +1456,24 @@ static bool write_mode(json_object *jobj, const char *keyword, int value)
 	return true;
 }
 
+static bool write_comp_id(json_object *jobj, uint16_t cid, uint16_t pid,
+								uint16_t vid)
+{
+	if (!jobj)
+		return false;
+
+	if (!write_uint16_hex(jobj, "cid", cid))
+		return false;
+
+	if (!write_uint16_hex(jobj, "pid", pid))
+		return false;
+
+	if (!write_uint16_hex(jobj, "vid", vid))
+		return false;
+
+	return true;
+}
+
 bool mesh_config_write_mode(struct mesh_config *cfg, const char *keyword,
 								int value)
 {
@@ -1595,16 +1613,10 @@ static struct mesh_config *create_config(const char *cfg_path,
 
 	jnode = json_object_new_object();
 
-	/* CID, PID, VID, crpl */
-	if (!write_uint16_hex(jnode, "cid", node->cid))
-		return NULL;
-
-	if (!write_uint16_hex(jnode, "pid", node->pid))
-		return NULL;
-
-	if (!write_uint16_hex(jnode, "vid", node->vid))
+	if (!write_comp_id(jnode, node->cid, node->pid, node->vid))
 		return NULL;
 
+	/* CRPL */
 	if (!write_uint16_hex(jnode, "crpl", node->crpl))
 		return NULL;
 
@@ -2052,6 +2064,18 @@ bool mesh_config_write_ttl(struct mesh_config *cfg, uint8_t ttl)
 	return save_config(cfg->jnode, cfg->node_dir_path);
 }
 
+bool mesh_config_write_comp_id(struct mesh_config *cfg, uint16_t cid,
+						uint16_t pid, uint16_t vid)
+{
+	if (!cfg)
+		return false;
+
+	if (!write_comp_id(cfg->jnode, cid, pid, vid))
+		return false;
+
+	return true;
+}
+
 static bool load_node(const char *fname, const uint8_t uuid[16],
 				mesh_config_node_func_t cb, void *user_data)
 {
diff --git a/mesh/mesh-config.h b/mesh/mesh-config.h
index a5b12bbad..9a5d6e57a 100644
--- a/mesh/mesh-config.h
+++ b/mesh/mesh-config.h
@@ -135,6 +135,8 @@ bool mesh_config_write_unicast(struct mesh_config *cfg, uint16_t unicast);
 bool mesh_config_write_relay_mode(struct mesh_config *cfg, uint8_t mode,
 					uint8_t count, uint16_t interval);
 bool mesh_config_write_ttl(struct mesh_config *cfg, uint8_t ttl);
+bool mesh_config_write_comp_id(struct mesh_config *cfg, uint16_t cid,
+						uint16_t pid, uint16_t vid);
 bool mesh_config_write_mode(struct mesh_config *cfg, const char *keyword,
 								int value);
 bool mesh_config_model_binding_add(struct mesh_config *cfg, uint16_t ele_addr,
-- 
2.20.1

[PATCH BlueZ v2 3/4] mesh: remove unused node_set_device_key()

[Jakub Witowski]

This patch removes node_set_device_key() function,
because it is unused.
---
 mesh/node.c | 5 -----
 mesh/node.h | 1 -
 2 files changed, 6 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index 6fe70742d..d4be070fa 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -628,11 +628,6 @@ uint16_t node_get_primary(struct mesh_node *node)
 		return node->primary;
 }
 
-void node_set_device_key(struct mesh_node *node, uint8_t key[16])
-{
-	memcpy(node->dev_key, key, 16);
-}
-
 const uint8_t *node_get_device_key(struct mesh_node *node)
 {
 	if (!node)
diff --git a/mesh/node.h b/mesh/node.h
index a6bc4a2a6..38aea138f 100644
--- a/mesh/node.h
+++ b/mesh/node.h
@@ -46,7 +46,6 @@ uint16_t node_get_primary(struct mesh_node *node);
 uint16_t node_get_primary_net_idx(struct mesh_node *node);
 void node_set_token(struct mesh_node *node, uint8_t token[8]);
 const uint8_t *node_get_token(struct mesh_node *node);
-void node_set_device_key(struct mesh_node *node, uint8_t key[16]);
 const uint8_t *node_get_device_key(struct mesh_node *node);
 void node_set_num_elements(struct mesh_node *node, uint8_t num_ele);
 uint8_t node_get_num_elements(struct mesh_node *node);
-- 
2.20.1

[PATCH BlueZ v2 4/4] mesh: allow to reattach with new composition data

[Jakub Witowski]

This patch allows to change the CIP/PID/VID data when
reattaching. Additionally the device key is verified
during those change.
---
 mesh/node.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/mesh/node.c b/mesh/node.c
index d4be070fa..86102d1da 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -47,6 +47,9 @@
 
 #define MIN_COMP_SIZE 14
 
+/* COMP_ID_SIZE consists of length of the CID, PID and VID */
+#define COMP_ID_SIZE 6
+
 #define MESH_NODE_PATH_PREFIX "/node"
 
 /* Default values for a new locally created node */
@@ -1389,15 +1392,49 @@ static bool check_req_node(struct managed_obj_request *req)
 		uint16_t attach_len = node_generate_comp(req->attach,
 					attach_comp, sizeof(attach_comp));
 
+		uint8_t keyring_dev_key[16];
+
 		/* Ignore feature bits in Composition Compare */
 		node_comp[8] = 0;
 		attach_comp[8] = 0;
 
+		/* Ignore CID, PID, VID in Composition Compare */
 		if (node_len != attach_len ||
-				memcmp(node_comp, attach_comp, node_len)) {
+					memcmp(node_comp + COMP_ID_SIZE,
+						attach_comp + COMP_ID_SIZE,
+						node_len - COMP_ID_SIZE)) {
 			l_debug("Failed to verify app's composition data");
 			return false;
 		}
+
+		/* Compare CID, VID and PID */
+		if (!memcmp(node_comp, attach_comp, COMP_ID_SIZE))
+			return true;
+
+		/* Verify the device key */
+		keyring_get_remote_dev_key(req->attach, req->attach->primary,
+							keyring_dev_key);
+
+		if (memcmp(keyring_dev_key,
+					node_get_device_key(req->attach), 16))
+			return false;
+
+		if (!mesh_config_write_comp_id(req->attach->cfg,
+							req->node->comp.cid,
+							req->node->comp.pid,
+							req->node->comp.vid)) {
+			l_debug("Failed to update comp id data");
+			return false;
+		}
+
+		if (!mesh_config_save(req->attach->cfg, true, NULL, NULL)) {
+			l_debug("Failed to store comp id");
+			return false;
+		}
+
+		memcpy(&req->attach->comp, &req->node->comp,
+					sizeof(struct node_composition) -
+						sizeof(req->node->comp.crpl));
 	}
 
 	return true;
-- 
2.20.1

Re: [PATCH BlueZ v2 0/4] Allow to reattach with new composition data

[Gix, Brian]

Hi Jakub,
On Thu, 2020-01-30 at 15:34 +0100, Jakub Witowski wrote:
> This patch allows the application to modify the CID, PID and VID in the composition data.
> 
> Version 2: Do not allow to change CRPL in the composition data.
> Additionaly verify the device key when updating comp data and remove
> unused function in the 3rd patch.
> 
> Version 1: According the Mesh Profile (2.3.4 Elements) the modification of fields
> other than "Elements" is not prohibited.
> 
> Also in my opinion (as you can see in the 1st patch), there is no need to use pointer to
> the node_composition struct. The static is more clear and less problematic.
> 
> Jakub Witowski (4):
>   mesh: use static node_comp instead of the pointer
>   mesh: add cid/pid/vid setter
>   mesh: remove unused node_set_device_key()
>   mesh: allow to reattach with new composition data

Patches 1 and 3 of this patchset have been applied, as they are non-controversial.

I would like to wait a little while, as the Working group weighs in, on modifying composition data.

I am actually ready today to allow an App to Attach to an existing node, with modifications to it's CID/PID/VID
(drop daemon validation) with the understanding that the composition stored in node.json is not changed.  But
anything that changes OTA behavior, I would like blessed by the WG and the SIG.


> 
>  mesh/mesh-config-json.c |  40 ++++++++++++----
>  mesh/mesh-config.h      |   2 +
>  mesh/node.c             | 100 +++++++++++++++++++++++++---------------
>  mesh/node.h             |   1 -
>  4 files changed, 96 insertions(+), 47 deletions(-)
>