[pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method

[pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method

[Dominik Csapak]

for getting multiple properties from the in memory config of the
guests. I added a new CSF_IPC_ call to maintain backwards compatibility.

It basically behaves the same as
CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
instead.

The old way of getting a single property is now also done by
the new function.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes since v2:
* add 'a-z' check while parsing the properties, this way the later min/max
  code works as intended

 data/src/cfs-ipc-ops.h |   2 +
 data/src/server.c      |  62 +++++++++++++++
 data/src/status.c      | 174 ++++++++++++++++++++++++++++-------------
 data/src/status.h      |   3 +
 4 files changed, 185 insertions(+), 56 deletions(-)

diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h
index 003e233..249308d 100644
--- a/data/src/cfs-ipc-ops.h
+++ b/data/src/cfs-ipc-ops.h
@@ -43,4 +43,6 @@
 
 #define CFS_IPC_VERIFY_TOKEN 12
 
+#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13
+
 #endif
diff --git a/data/src/server.c b/data/src/server.c
index 549788a..b9394c8 100644
--- a/data/src/server.c
+++ b/data/src/server.c
@@ -89,6 +89,13 @@ typedef struct {
 	char property[];
 } cfs_guest_config_propery_get_request_header_t;
 
+typedef struct {
+	struct qb_ipc_request_header req_header;
+	uint32_t vmid;
+	uint8_t num_props;
+	char props[]; /* list of \0 terminated properties */
+} cfs_guest_config_properties_get_request_header_t;
+
 typedef struct {
 	struct qb_ipc_request_header req_header;
 	char token[];
@@ -348,6 +355,61 @@ static int32_t s1_msg_process_fn(
 
 			result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
 		}
+	} else if (request_id == CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) {
+
+		cfs_guest_config_properties_get_request_header_t *rh =
+			(cfs_guest_config_properties_get_request_header_t *) data;
+
+		int propslen = request_size - G_STRUCT_OFFSET(cfs_guest_config_properties_get_request_header_t, props);
+
+		result = 0;
+		if (rh->vmid < 100 && rh->vmid != 0) {
+			cfs_debug("vmid out of range %u", rh->vmid);
+			result = -EINVAL;
+		} else if (rh->vmid >= 100 && !vmlist_vm_exists(rh->vmid)) {
+			result = -ENOENT;
+		} else if (propslen <= 1) {
+			cfs_debug("propslen <= 1, %d", propslen);
+			result = -EINVAL;
+		} else {
+			const char **properties = malloc(sizeof(char*) * rh->num_props);
+			char *current = (rh->props);
+			int remaining = propslen;
+			for (uint8_t i = 0; i < rh->num_props; i++) {
+			    uint8_t proplen = strnlen(current, remaining);
+			    if (proplen == 0) {
+				cfs_debug("property length 0");
+				result = -EINVAL;
+				break;
+			    }
+			    if (proplen == remaining) {
+				cfs_debug("property not \\0 terminated");
+				result = -EINVAL;
+				break;
+			    }
+			    if (current[0] < 'a' || current[0] > 'z') {
+				cfs_debug("property does not start with [a-z]");
+				result = -EINVAL;
+				break;
+			    }
+			    properties[i] = current;
+			    current[proplen] = '\0'; // ensure property is 0 terminated
+			    remaining -= (proplen + 1);
+			    current += proplen + 1;
+			}
+
+			if (remaining != 0) {
+			    cfs_debug("leftover data after parsing %ul properties", rh->num_props);
+			    result = -EINVAL;
+			}
+
+			if (result == 0) {
+			    cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
+			    result = cfs_create_guest_conf_properties_msg(outbuf, memdb, properties, rh->num_props, rh->vmid);
+			}
+
+			free(properties);
+		}
 	} else if (request_id == CFS_IPC_VERIFY_TOKEN) {
 
 		cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;
diff --git a/data/src/status.c b/data/src/status.c
index 9bceaeb..33ca06f 100644
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -804,25 +804,52 @@ cfs_create_vmlist_msg(GString *str)
 	return 0;
 }
 
-// checks the conf for a line starting with '$prop:' and returns the value
-// afterwards, whitout initial whitespace(s), we only deal with the format
-// restricion imposed by our perl VM config parser, main reference is
+// checks if a config line starts with the given prop. if yes, writes a '\0'
+// at the end of the value, and returns the pointer where the value starts
+char *
+_get_property_value_from_line(char *line, int line_len, const char *prop, int prop_len)
+{
+	if (line_len <= prop_len + 1) return NULL;
+
+	if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
+		char *v_start = &line[prop_len + 1];
+
+		// drop initial value whitespaces here already
+		while (*v_start && isspace(*v_start)) v_start++;
+
+		if (!*v_start) return NULL;
+
+		char *v_end = &line[line_len - 1];
+		while (v_end > v_start && isspace(*v_end)) v_end--;
+		v_end[1] = '\0';
+
+		return v_start;
+	}
+
+	return NULL;
+}
+
+// checks the conf for lines starting with the given props and
+// writes the pointers into the correct positions into the 'found' array
+// afterwards, whithout initial whitespace(s), we only deal with the format
+// restriction imposed by our perl VM config parser, main reference is
 // PVE::QemuServer::parse_vm_config this allows to be very fast and still
 // relatively simple
-// main restrictions used for our advantage is the properties match reges:
+// main restrictions used for our advantage is the properties match regex:
 // ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) from parse_vm_config
 // currently we only look at the current configuration in place, i.e., *no*
-// snapshort and *no* pending changes
-static char *
-_get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
+// snapshot and *no* pending changes
+void
+_get_property_values(char **found, char *conf, int conf_size, const char **props, uint8_t num_props, char min, char max)
 {
 	const char *const conf_end = conf + conf_size;
 	char *line = conf;
-	size_t remaining_size;
+	size_t remaining_size = conf_size;
+	int count = 0;
 
 	char *next_newline = memchr(conf, '\n', conf_size);
 	if (next_newline == NULL) {
-		return NULL; // valid property lines end with \n, but none in the config
+		return; // valid property lines end with \n, but none in the config
 	}
 	*next_newline = '\0';
 
@@ -830,41 +857,33 @@ _get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
 		if (!line[0]) goto next;
 
 		// snapshot or pending section start, but nothing found yet -> not found
-		if (line[0] == '[') return NULL;
-		// properties start with /^[a-z]/, so continue early if not
-		if (line[0] < 'a' || line[0] > 'z') goto next;
+		if (line[0] == '[') return;
+		// continue early if line does not begin with the min/max char of the properties
+		if (line[0] < min || line[0] > max) goto next;
 
 		int line_len = strlen(line);
-		if (line_len <= prop_len + 1) goto next;
-
-		if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
-			char *v_start = &line[prop_len + 1];
-
-			// drop initial value whitespaces here already
-			while (*v_start && isspace(*v_start)) v_start++;
-
-			if (!*v_start) return NULL;
-
-			char *v_end = &line[line_len - 1];
-			while (v_end > v_start && isspace(*v_end)) v_end--;
-			v_end[1] = '\0';
-
-			return v_start;
+		for (uint8_t i = 0; i < num_props; i++) {
+			char * value = _get_property_value_from_line(line, line_len, props[i], strlen(props[i]));
+			if (value != NULL) {
+				count += (found[i] != NULL) & 0x1; // count newly found lines
+				found[i] = value;
+			}
+		}
+		if (count == num_props) {
+			// found all
+			return;
 		}
 next:
 		line = next_newline + 1;
 		remaining_size = conf_end - line;
-		if (remaining_size <= prop_len) {
-			return NULL;
-		}
 		next_newline = memchr(line, '\n', remaining_size);
 		if (next_newline == NULL) {
-			return NULL; // valid property lines end with \n, but none in the config
+			return;
 		}
 		*next_newline = '\0';
 	}
 
-	return NULL; // not found
+	return;
 }
 
 static void
@@ -883,24 +902,73 @@ _g_str_append_kv_jsonescaped(GString *str, const char *k, const char *v)
 }
 
 int
-cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid)
+_print_found_properties(
+	GString *str,
+	gpointer conf,
+	int size,
+	const char **props,
+	uint8_t num_props,
+	uint32_t vmid,
+	char **values,
+	char min,
+	char max,
+	int first)
+{
+	_get_property_values(values, conf, size, props, num_props, min, max);
+
+	uint8_t found = 0;
+	for (uint8_t i = 0; i < num_props; i++) {
+		if (values[i] != NULL) {
+			if (found) {
+				g_string_append_c(str, ',');
+			} else {
+				if (!first) g_string_append_printf(str, ",\n");
+				else first = 0;
+				g_string_append_printf(str, "\"%u\":{", vmid);
+				found = 1;
+			}
+			_g_str_append_kv_jsonescaped(str, props[i], values[i]);
+		}
+	}
+
+	if (found) {
+		g_string_append_c(str, '}');
+	}
+
+	return first;
+}
+
+int
+cfs_create_guest_conf_properties_msg(GString *str, memdb_t *memdb, const char **props, uint8_t num_props, uint32_t vmid)
 {
 	g_return_val_if_fail(cfs_status.vmlist != NULL, -EINVAL);
 	g_return_val_if_fail(str != NULL, -EINVAL);
 
-	int prop_len = strlen(prop);
-	int res = 0;
-	GString *path = NULL;
-
 	// Prelock &memdb->mutex in order to enable the usage of memdb_read_nolock
 	// to prevent Deadlocks as in #2553
 	g_mutex_lock (&memdb->mutex);
 	g_mutex_lock (&mutex);
 
-	g_string_printf(str,"{\n");
+	g_string_printf(str, "{\n");
 
 	GHashTable *ht = cfs_status.vmlist;
+
+	int res = 0;
+	GString *path = NULL;
 	gpointer tmp = NULL;
+	char **values = calloc(num_props, sizeof(char*));
+	char min = 'z', max = 'a';
+
+	for (uint8_t i = 0; i < num_props; i++) {
+		if (props[i][0] > max) {
+			max = props[i][0];
+		}
+
+		if (props[i][0] < min) {
+			min = props[i][0];
+		}
+	}
+
 	if (!g_hash_table_size(ht)) {
 		goto ret;
 	}
@@ -919,15 +987,8 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
 		// use memdb_read_nolock because lock is handled here
 		int size = memdb_read_nolock(memdb, path->str, &tmp);
 		if (tmp == NULL) goto err;
-		if (size <= prop_len) goto ret;
-
-		char *val = _get_property_value(tmp, size, prop, prop_len);
-		if (val == NULL) goto ret;
-
-		g_string_append_printf(str, "\"%u\":{", vmid);
-		_g_str_append_kv_jsonescaped(str, prop, val);
-		g_string_append_c(str, '}');
 
+		_print_found_properties(str, tmp, size, props, num_props, vmid, values, min, max, 1);
 	} else {
 		GHashTableIter iter;
 		g_hash_table_iter_init (&iter, ht);
@@ -943,21 +1004,16 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
 			tmp = NULL;
 			// use memdb_read_nolock because lock is handled here
 			int size = memdb_read_nolock(memdb, path->str, &tmp);
-			if (tmp == NULL || size <= prop_len) continue;
+			if (tmp == NULL) continue;
 
-			char *val = _get_property_value(tmp, size, prop, prop_len);
-			if (val == NULL) continue;
-
-			if (!first) g_string_append_printf(str, ",\n");
-			else first = 0;
-
-			g_string_append_printf(str, "\"%u\":{", vminfo->vmid);
-			_g_str_append_kv_jsonescaped(str, prop, val);
-			g_string_append_c(str, '}');
+			memset(values, 0, sizeof(char*)*num_props); // reset array
+			first = _print_found_properties(str, tmp, size, props, num_props,
+					vminfo->vmid, values, min, max, first);
 		}
 	}
 ret:
 	g_free(tmp);
+	free(values);
 	if (path != NULL) {
 		g_string_free(path, TRUE);
 	}
@@ -973,6 +1029,12 @@ enoent:
 	goto ret;
 }
 
+int
+cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid)
+{
+	return cfs_create_guest_conf_properties_msg(str, memdb, &prop, 1, vmid);
+}
+
 void
 record_memdb_change(const char *path)
 {
diff --git a/data/src/status.h b/data/src/status.h
index bbf0948..041cb34 100644
--- a/data/src/status.h
+++ b/data/src/status.h
@@ -163,4 +163,7 @@ cfs_create_memberlist_msg(
 int
 cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid);
 
+int
+cfs_create_guest_conf_properties_msg(GString *str, memdb_t *memdb, const char **props, uint8_t num_props, uint32_t vmid);
+
 #endif /* _PVE_STATUS_H_ */
-- 
2.30.2

[pve-devel] [PATCH cluster v3 2/2] Cluster: add get_guest_config_properties

[Dominik Csapak]

akin to get_guest_config_property, but with a list of properties.
uses the new CFS_IPC_GET_GUEST_CONFIG_PROPERTIES

also adds the same NOTEs regarding parsing/permissions to the comment
of get_guest_config_property

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 data/PVE/Cluster.pm | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index 05451fd..d0148fc 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -340,10 +340,37 @@ sub get_node_kv {
     return $res;
 }
 
+# properties: an array-ref of config properties you want to get, e.g., this
+# is perfect to get multiple properties of a guest _fast_
+# (>100 faster than manual parsing here)
+# vmid: optional, if a valid is passed we only check that one, else return all
+# NOTE: does *not* searches snapshot and PENDING entries sections!
+# NOTE: returns the guest config lines (excluding trailing whitespace) as is,
+#       so for non-trivial properties, checking the validity must be done
+# NOTE: no permission check is done, that is the responsibilty of the caller
+sub get_guest_config_properties {
+    my ($properties, $vmid) = @_;
+
+    die "properties required" if !defined($properties);
+
+    my $num_props = scalar(@$properties);
+    die "only up to 255 properties supported" if $num_props > 255;
+    my $bindata = pack "VC", $vmid // 0, $num_props;
+    for my $property (@$properties) {
+	$bindata .= pack "Z*", $property;
+    }
+    my $res = $ipcc_send_rec_json->(CFS_IPC_GET_GUEST_CONFIG_PROPERTIES, $bindata);
+
+    return $res;
+}
+
 # property: a config property you want to get, e.g., this is perfect to get
 # the 'lock' entry of a guest _fast_ (>100 faster than manual parsing here)
 # vmid: optional, if a valid is passed we only check that one, else return all
 # NOTE: does *not* searches snapshot and PENDING entries sections!
+# NOTE: returns the guest config lines (excluding trailing whitespace) as is,
+#       so for non-trivial properties, checking the validity must be done
+# NOTE: no permission check is done, that is the responsibilty of the caller
 sub get_guest_config_property {
     my ($property, $vmid) = @_;
 
-- 
2.30.2

Re: [pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method

[Fabian Grünbichler]

On March 17, 2022 12:44 pm, Dominik Csapak wrote:
> for getting multiple properties from the in memory config of the
> guests. I added a new CSF_IPC_ call to maintain backwards compatibility.
> 
> It basically behaves the same as
> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
> instead.
> 
> The old way of getting a single property is now also done by
> the new function.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

one more small nit, otherwise this LGTM

> ---
> changes since v2:
> * add 'a-z' check while parsing the properties, this way the later min/max
>   code works as intended
> 
>  data/src/cfs-ipc-ops.h |   2 +
>  data/src/server.c      |  62 +++++++++++++++
>  data/src/status.c      | 174 ++++++++++++++++++++++++++++-------------
>  data/src/status.h      |   3 +
>  4 files changed, 185 insertions(+), 56 deletions(-)
> 
> diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h
> index 003e233..249308d 100644
> --- a/data/src/cfs-ipc-ops.h
> +++ b/data/src/cfs-ipc-ops.h
> @@ -43,4 +43,6 @@
>  
>  #define CFS_IPC_VERIFY_TOKEN 12
>  
> +#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13
> +
>  #endif
> diff --git a/data/src/server.c b/data/src/server.c
> index 549788a..b9394c8 100644
> --- a/data/src/server.c
> +++ b/data/src/server.c
> @@ -89,6 +89,13 @@ typedef struct {
>  	char property[];
>  } cfs_guest_config_propery_get_request_header_t;
>  
> +typedef struct {
> +	struct qb_ipc_request_header req_header;
> +	uint32_t vmid;
> +	uint8_t num_props;
> +	char props[]; /* list of \0 terminated properties */
> +} cfs_guest_config_properties_get_request_header_t;
> +
>  typedef struct {
>  	struct qb_ipc_request_header req_header;
>  	char token[];
> @@ -348,6 +355,61 @@ static int32_t s1_msg_process_fn(
>  
>  			result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
>  		}
> +	} else if (request_id == CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) {
> +
> +		cfs_guest_config_properties_get_request_header_t *rh =
> +			(cfs_guest_config_properties_get_request_header_t *) data;
> +
> +		int propslen = request_size - G_STRUCT_OFFSET(cfs_guest_config_properties_get_request_header_t, props);
> +
> +		result = 0;
> +		if (rh->vmid < 100 && rh->vmid != 0) {
> +			cfs_debug("vmid out of range %u", rh->vmid);
> +			result = -EINVAL;

sanity check for num_props being > 0 here would probably be good. with 
the current code it's not problematic, but who knows how this evolves in 
the future ;)

(alternatively, we could also drop num_props entirely and just say the 
msg must at most contain 255 properties, and always allocate 255 slots 
below and abort on the 256th property? but I guess the safeguard against 
bugs on the caller side doesn't hurt and it saves a bit of memory in the 
common case which is probably with <10 properties even if we find a few 
more use cases :))

> +		} else if (rh->vmid >= 100 && !vmlist_vm_exists(rh->vmid)) {
> +			result = -ENOENT;
> +		} else if (propslen <= 1) {
> +			cfs_debug("propslen <= 1, %d", propslen);
> +			result = -EINVAL;
> +		} else {
> +			const char **properties = malloc(sizeof(char*) * rh->num_props);

malloc(0), so we must never dereference the returned pointer in that 
case.

> +			char *current = (rh->props);
> +			int remaining = propslen;
> +			for (uint8_t i = 0; i < rh->num_props; i++) {

loop is skipped since 0 < 0 is false

> +			    uint8_t proplen = strnlen(current, remaining);
> +			    if (proplen == 0) {
> +				cfs_debug("property length 0");
> +				result = -EINVAL;
> +				break;
> +			    }
> +			    if (proplen == remaining) {
> +				cfs_debug("property not \\0 terminated");
> +				result = -EINVAL;
> +				break;
> +			    }
> +			    if (current[0] < 'a' || current[0] > 'z') {
> +				cfs_debug("property does not start with [a-z]");
> +				result = -EINVAL;
> +				break;
> +			    }
> +			    properties[i] = current;
> +			    current[proplen] = '\0'; // ensure property is 0 terminated
> +			    remaining -= (proplen + 1);
> +			    current += proplen + 1;
> +			}
> +
> +			if (remaining != 0) {

this must then be true, since we had data but didn't process it in any 
way

> +			    cfs_debug("leftover data after parsing %ul properties", rh->num_props);
> +			    result = -EINVAL;
> +			}
> +
> +			if (result == 0) {
> +			    cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
> +			    result = cfs_create_guest_conf_properties_msg(outbuf, memdb, properties, rh->num_props, rh->vmid);
> +			}
> +
> +			free(properties);

so we free it here without ever touching it and return an error. since 
about the only thing we are allowed to do with the result of malloc(0) 
is freeing it, right now this is okay :)

> +		}
>  	} else if (request_id == CFS_IPC_VERIFY_TOKEN) {
>  
>  		cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;

[..]

Re: [pve-devel] [PATCH cluster v3 1/2] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method

[Fabian Grünbichler]

On April 8, 2022 10:40 am, Fabian Grünbichler wrote:
> On March 17, 2022 12:44 pm, Dominik Csapak wrote:
>> for getting multiple properties from the in memory config of the
>> guests. I added a new CSF_IPC_ call to maintain backwards compatibility.
>> 
>> It basically behaves the same as
>> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
>> instead.
>> 
>> The old way of getting a single property is now also done by
>> the new function.
>> 
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> 
> one more small nit, otherwise this LGTM
> 

obviously after hitting send I found another one :-P

>> ---
>> changes since v2:
>> * add 'a-z' check while parsing the properties, this way the later min/max
>>   code works as intended
>> 
>>  data/src/cfs-ipc-ops.h |   2 +
>>  data/src/server.c      |  62 +++++++++++++++
>>  data/src/status.c      | 174 ++++++++++++++++++++++++++++-------------
>>  data/src/status.h      |   3 +
>>  4 files changed, 185 insertions(+), 56 deletions(-)
>> 
>> diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h
>> index 003e233..249308d 100644
>> --- a/data/src/cfs-ipc-ops.h
>> +++ b/data/src/cfs-ipc-ops.h
>> @@ -43,4 +43,6 @@
>>  
>>  #define CFS_IPC_VERIFY_TOKEN 12
>>  
>> +#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13
>> +
>>  #endif
>> diff --git a/data/src/server.c b/data/src/server.c
>> index 549788a..b9394c8 100644
>> --- a/data/src/server.c
>> +++ b/data/src/server.c
>> @@ -89,6 +89,13 @@ typedef struct {
>>  	char property[];
>>  } cfs_guest_config_propery_get_request_header_t;
>>  
>> +typedef struct {
>> +	struct qb_ipc_request_header req_header;
>> +	uint32_t vmid;
>> +	uint8_t num_props;
>> +	char props[]; /* list of \0 terminated properties */
>> +} cfs_guest_config_properties_get_request_header_t;
>> +
>>  typedef struct {
>>  	struct qb_ipc_request_header req_header;
>>  	char token[];
>> @@ -348,6 +355,61 @@ static int32_t s1_msg_process_fn(
>>  
>>  			result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
>>  		}
>> +	} else if (request_id == CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) {
>> +
>> +		cfs_guest_config_properties_get_request_header_t *rh =
>> +			(cfs_guest_config_properties_get_request_header_t *) data;
>> +
>> +		int propslen = request_size - G_STRUCT_OFFSET(cfs_guest_config_properties_get_request_header_t, props);

so request_size is int32_t, and the offset is 32 + 32 (qb header) + 32 + 
8 = 13 bytes, so propslen will always fit in an int and be positive.

>> +
>> +		result = 0;
>> +		if (rh->vmid < 100 && rh->vmid != 0) {
>> +			cfs_debug("vmid out of range %u", rh->vmid);
>> +			result = -EINVAL;
>> +		} else if (rh->vmid >= 100 && !vmlist_vm_exists(rh->vmid)) {
>> +			result = -ENOENT;
>> +		} else if (propslen <= 1) {
>> +			cfs_debug("propslen <= 1, %d", propslen);
>> +			result = -EINVAL;
>> +		} else {
>> +			const char **properties = malloc(sizeof(char*) * rh->num_props);
>> +			char *current = (rh->props);
>> +			int remaining = propslen;

remaining starts positive

>> +			for (uint8_t i = 0; i < rh->num_props; i++) {
>> +			    uint8_t proplen = strnlen(current, remaining);

this ain't no uint8_t ;) so this takes a size_t (positive int is fine 
accordingly) and returns one. a property can be longer than 255 chars 
though, which makes the assignment overflow and gives us a 
wrong/truncated proplen

>> +			    if (proplen == 0) {

could trigger this if the overflow aligns right.

>> +				cfs_debug("property length 0");
>> +				result = -EINVAL;
>> +				break;
>> +			    }
>> +			    if (proplen == remaining) {

can't be triggered by overflow since proplen must always be smaller than 
remaining

>> +				cfs_debug("property not \\0 terminated");
>> +				result = -EINVAL;
>> +				break;
>> +			    }
>> +			    if (current[0] < 'a' || current[0] > 'z') {

this could be triggered depending on how exactly proplen overflows / the 
property gets truncated

>> +				cfs_debug("property does not start with [a-z]");
>> +				result = -EINVAL;
>> +				break;
>> +			    }
>> +			    properties[i] = current;
>> +			    current[proplen] = '\0'; // ensure property is 0 terminated

truncates an overly long property

>> +			    remaining -= (proplen + 1);
>> +			    current += proplen + 1;

and we start the next iteration at the truncated position

>> +			}
>> +
>> +			if (remaining != 0) {

which likely makes us end up here (except if the msg was malformed with 
not-null-terminated properties, in which case they might align again and 
just their boundaries are wrong ;))

given that we assume remaining remains positive, and barring further 
future bugs that invariant should hold (we start off positive, in each 
iteration proplen must be < remaining to reach the subtraction which 
therefore at most reduces remaining to 0), we could switch both 
remaining and proplen to size_t ?

or if the/a limit on property length is actually desired, we should 
explicitly check for that (propslen <= limit * count at the start, and 
then in the loop body still get a size_t from strnlen and check that the 
result is < limit)

> 
>> +			    cfs_debug("leftover data after parsing %ul properties", rh->num_props);
>> +			    result = -EINVAL;
>> +			}
>> +
>> +			if (result == 0) {
>> +			    cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
>> +			    result = cfs_create_guest_conf_properties_msg(outbuf, memdb, properties, rh->num_props, rh->vmid);
>> +			}
>> +
>> +			free(properties);
>> +		}
>>  	} else if (request_id == CFS_IPC_VERIFY_TOKEN) {
>>  
>>  		cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;
> 
> [..]
>