[pve-devel] applied: [PATCH cluster] pmxcfs: get config properties: ensure we do not read after the config

[Wolfgang Bumiller]

applied with the following fixup commit we talked about off-list:

---8<---
>From c22040264ebe7b5b8b1dcd16c1af8d174600b1ea Mon Sep 17 00:00:00 2001
From: Wolfgang Bumiller <w.bumiller at proxmox.com>
Date: Fri, 30 Aug 2019 10:09:46 +0200
Subject: [PATCH cluster] pmxcfs: cleanup remaining_size calculation

using an end-pointer it's a bit more readable and gets rid
of an (int) cast

Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
---
 data/src/status.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/data/src/status.c b/data/src/status.c
index 1dfde53..e9983b7 100644
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -803,8 +803,9 @@ cfs_create_vmlist_msg(GString *str)
 static char *
 _get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
 {
+	const char *const conf_end = conf + conf_size;
 	char *line = conf;
-	int remaining_size;
+	size_t remaining_size;
 
 	char *next_newline = memchr(conf, '\n', conf_size);
 	if (next_newline == NULL) {
@@ -838,11 +839,11 @@ _get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
 			return v_start;
 		}
 next:
-		remaining_size = conf_size - (int) (next_newline - conf);
-		if (remaining_size <= 1 || remaining_size <= prop_len) {
+		line = next_newline + 1;
+		remaining_size = conf_end - line;
+		if (remaining_size <= prop_len) {
 			return NULL;
 		}
-		line = next_newline + 1;
 		next_newline = memchr(line, '\n', remaining_size);
 		if (next_newline == NULL) {
 			return NULL; // valid property lines end with \n, but none in the config
-- 
2.20.1

---8<---

On Thu, Aug 29, 2019 at 02:45:08PM +0200, Thomas Lamprecht wrote:
> pmxcfs files need to be treated as blobs, while we can have some
> assumptions on certain files, like the $vmid.conf ones, we should
> still cope with problematic files.
> Especially, the files may not end with \0, so always ensure that we
> read at most file-size bytes.
> 
> Replace strtok_r, which assumes that the data is NUL terminated, and
> use memchr, with logic ensuring that we never read over the size
> returned by memdb_read.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
>  data/src/status.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/data/src/status.c b/data/src/status.c
> index e437476..1dfde53 100644
> --- a/data/src/status.c
> +++ b/data/src/status.c
> @@ -801,15 +801,21 @@ cfs_create_vmlist_msg(GString *str)
>  // 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, const char *prop, int prop_len)
> +_get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
>  {
> -	char *line = NULL, *temp = NULL;
> +	char *line = conf;
> +	int remaining_size;
> +
> +	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
> +	}
> +	*next_newline = '\0';
>  
> -	line = strtok_r(conf, "\n", &temp);
>  	while (line != NULL) {
>  		if (!line[0]) goto next;
>  
> -		// snapshot or pending section start and nothing found yet
> +		// 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;
> @@ -832,7 +838,16 @@ _get_property_value(char *conf, const char *prop, int prop_len)
>  			return v_start;
>  		}
>  next:
> -		line = strtok_r(NULL, "\n", &temp);
> +		remaining_size = conf_size - (int) (next_newline - conf);
> +		if (remaining_size <= 1 || remaining_size <= prop_len) {
> +			return NULL;
> +		}
> +		line = next_newline + 1;
> +		next_newline = memchr(line, '\n', remaining_size);
> +		if (next_newline == NULL) {
> +			return NULL; // valid property lines end with \n, but none in the config
> +		}
> +		*next_newline = '\0';
>  	}
>  
>  	return NULL; // not found
> @@ -884,7 +899,7 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
>  		if (tmp == NULL) goto err;
>  		if (size <= prop_len) goto ret;
>  
> -		char *val = _get_property_value(tmp, prop, prop_len);
> +		char *val = _get_property_value(tmp, size, prop, prop_len);
>  		if (val == NULL) goto ret;
>  
>  		g_string_append_printf(str, "\"%u\":{", vmid);
> @@ -907,7 +922,7 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
>  			int size = memdb_read(memdb, path->str, &tmp);
>  			if (tmp == NULL || size <= prop_len) continue;
>  
> -			char *val = _get_property_value(tmp, prop, prop_len);
> +			char *val = _get_property_value(tmp, size, prop, prop_len);
>  			if (val == NULL) continue;
>  
>  			if (!first) g_string_append_printf(str, ",\n");
> -- 
> 2.20.1