secvar: overhaul secvar struct by removing static sized fields

Originally, the secvar struct was intended to contain all the variable
information seperate from the linked list/etc metadata, so that copying
and serialization/deserialization could be handled by a single memcpy().

This is fragile, potentially compiler dependent, and doesn't account for
endianness. Therefore, this patch removes the static allocation for key, now
allocates a buffer for data, and completely removes the now unnecessary
secvar_node struct.

As a side effect, some of the secvar_util functionality has been tweaked where
it makes sense. Most notably alloc_secvar now takes in an extra argument as it
now has to allocate the key

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

3 files changed, 88 insertions, 97 deletions

diff --git a/libstb/secvar/secvar.h b/libstb/secvar/secvar.h
index b141b70..fe66021 100644
--- a/libstb/secvar/secvar.h
+++ b/libstb/secvar/secvar.h
@@ -16,24 +16,18 @@ enum {
 };
 
 
-struct secvar_node {
-	struct list_node link;
-	struct secvar *var;
-	uint64_t flags;		// Flag for how *var should be stored
-	uint64_t size;		// How much space was allocated for data
-};
-
 #define SECVAR_FLAG_VOLATILE		0x1 // Instructs storage driver to ignore variable on writes
 #define SECVAR_FLAG_SECURE_STORAGE	0x2 // Hint for storage driver to select storage location
 
 struct secvar {
+	struct list_node link;
 	uint64_t key_len;
 	uint64_t data_size;
-	char key[SECVAR_MAX_KEY_LEN];
-	char data[0];
+	uint64_t flags;
+	char *key;
+	char *data;
 };
 
-
 extern struct list_head variable_bank;
 extern struct list_head update_bank;
 extern int secvar_enabled;
@@ -44,13 +38,13 @@ extern struct secvar_backend_driver secvar_backend;
 // Helper functions
 void clear_bank_list(struct list_head *bank);
 int copy_bank_list(struct list_head *dst, struct list_head *src);
-struct secvar_node *alloc_secvar(uint64_t size);
-struct secvar_node *new_secvar(const char *key, uint64_t key_len,
+struct secvar *alloc_secvar(uint64_t key_len, uint64_t data_size);
+struct secvar *new_secvar(const char *key, uint64_t key_len,
 			       const char *data, uint64_t data_size,
 			       uint64_t flags);
-int realloc_secvar(struct secvar_node *node, uint64_t size);
-void dealloc_secvar(struct secvar_node *node);
-struct secvar_node *find_secvar(const char *key, uint64_t key_len, struct list_head *bank);
+int realloc_secvar(struct secvar *node, uint64_t size);
+void dealloc_secvar(struct secvar *node);
+struct secvar *find_secvar(const char *key, uint64_t key_len, struct list_head *bank);
 int is_key_empty(const char *key, uint64_t key_len);
 int list_length(struct list_head *bank);
 
diff --git a/libstb/secvar/secvar_api.c b/libstb/secvar/secvar_api.c
index 62c3250..7245e85 100644
--- a/libstb/secvar/secvar_api.c
+++ b/libstb/secvar/secvar_api.c
@@ -11,7 +11,7 @@
 
 static int64_t opal_secvar_get(const char *key, uint64_t key_len, void *data, uint64_t *data_size)
 {
-	struct secvar_node *node;
+	struct secvar *var;
 	int64_t rc = OPAL_SUCCESS;
 
 	if (!secvar_enabled)
@@ -26,18 +26,18 @@ static int64_t opal_secvar_get(const char *key, uint64_t key_len, void *data, ui
 	if (!data_size)
 		return OPAL_PARAMETER;
 
-	node = find_secvar(key, key_len, &variable_bank);
-	if (!node)
+	var = find_secvar(key, key_len, &variable_bank);
+	if (!var)
 		return OPAL_EMPTY; // Variable not found, bail early
 
 	if (!data)
 		rc = OPAL_SUCCESS;
-	else if (*data_size < node->var->data_size)
+	else if (*data_size < var->data_size)
 		rc = OPAL_PARTIAL;
 	else
-		memcpy(data, node->var->data, node->var->data_size);
+		memcpy(data, var->data, var->data_size);
 
-	*data_size = node->var->data_size;
+	*data_size = var->data_size;
 
 	return rc;
 }
@@ -46,7 +46,7 @@ opal_call(OPAL_SECVAR_GET, opal_secvar_get, 4);
 
 static int64_t opal_secvar_get_next(char *key, uint64_t *key_len, uint64_t key_buf_size)
 {
-	struct secvar_node *node;
+	struct secvar *var;
 
 	if (!secvar_enabled)
 		return OPAL_UNSUPPORTED;
@@ -64,25 +64,25 @@ static int64_t opal_secvar_get_next(char *key, uint64_t *key_len, uint64_t key_b
 		return OPAL_PARAMETER;
 
 	if (!is_key_empty(key, *key_len)) {
-		node = find_secvar(key, *key_len, &variable_bank);
-		if (!node)
+		var = find_secvar(key, *key_len, &variable_bank);
+		if (!var)
 			return OPAL_PARAMETER;
 
-		node = list_next(&variable_bank, node, link);
+		var = list_next(&variable_bank, var, link);
 	} else {
-		node = list_top(&variable_bank, struct secvar_node, link);
+		var = list_top(&variable_bank, struct secvar, link);
 	}
 
-	if (!node)
+	if (!var)
 		return OPAL_EMPTY;
 
-	if (key_buf_size < node->var->key_len) {
-		*key_len = node->var->key_len;
+	if (key_buf_size < var->key_len) {
+		*key_len = var->key_len;
 		return OPAL_PARTIAL;
 	}
 
-	*key_len = node->var->key_len;
-	memcpy(key, node->var->key, node->var->key_len);
+	*key_len = var->key_len;
+	memcpy(key, var->key, var->key_len);
 
 	return OPAL_SUCCESS;
 }
@@ -91,7 +91,7 @@ opal_call(OPAL_SECVAR_GET_NEXT, opal_secvar_get_next, 3);
 
 static int64_t opal_secvar_enqueue_update(const char *key, uint64_t key_len, void *data, uint64_t data_size)
 {
-	struct secvar_node *node;
+	struct secvar *var;
 
 	if (!secvar_enabled)
 		return OPAL_UNSUPPORTED;
@@ -114,41 +114,39 @@ static int64_t opal_secvar_enqueue_update(const char *key, uint64_t key_len, voi
 	if (is_key_empty(key, key_len))
 		return OPAL_PARAMETER;
 
-	node = find_secvar(key, key_len, &update_bank);
+	var = find_secvar(key, key_len, &update_bank);
 
 	// Unstage an update
 	if (data_size == 0) {
-		if (!node)
+		if (!var)
 			return OPAL_EMPTY;
 
-		if (node->var)
-			free(node->var);
-		list_del(&node->link);
-		free(node);
+		list_del(&var->link);
+		dealloc_secvar(var);
 		goto out;
 	}
 
-	if (node) {
-		list_del(&node->link);
+	if (var) {
+		list_del(&var->link);
 		// Realloc var if too small
-		if (node->size < data_size) {
-			if (realloc_secvar(node, data_size))
+		if (var->data_size < data_size) {
+			if (realloc_secvar(var, data_size))
 				return OPAL_NO_MEM;
 		} else {
-			memset(node->var, 0x00, sizeof(struct secvar) + node->var->data_size);
+			memset(var->data, 0x00, var->data_size);
 		}
 	} else {
-		node = alloc_secvar(data_size);
-		if (!node)
+		var = alloc_secvar(key_len, data_size);
+		if (!var)
 			return OPAL_NO_MEM;
 	}
 
-	memcpy(node->var->key, key, key_len);
-	node->var->key_len = key_len;
-	memcpy(node->var->data, data, data_size);
-	node->var->data_size = data_size;
+	memcpy(var->key, key, key_len);
+	var->key_len = key_len;
+	memcpy(var->data, data, data_size);
+	var->data_size = data_size;
 
-	list_add_tail(&update_bank, &node->link);
+	list_add_tail(&update_bank, &var->link);
 
 out:
 	if (secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK))
diff --git a/libstb/secvar/secvar_util.c b/libstb/secvar/secvar_util.c
index 3aadbd9..533170a 100644
--- a/libstb/secvar/secvar_util.c
+++ b/libstb/secvar/secvar_util.c
@@ -13,33 +13,25 @@
 
 void clear_bank_list(struct list_head *bank)
 {
-	struct secvar_node *node, *next;
+	struct secvar *var, *next;
 
 	if (!bank)
 		return;
 
-	list_for_each_safe(bank, node, next, link) {
-		list_del(&node->link);
-		dealloc_secvar(node);
+	list_for_each_safe(bank, var, next, link) {
+		list_del(&var->link);
+		dealloc_secvar(var);
 	}
 }
 
 int copy_bank_list(struct list_head *dst, struct list_head *src)
 {
-	struct secvar_node *node, *tmp;
+	struct secvar *var, *tmp;
 
-	list_for_each(src, node, link) {
+	list_for_each(src, var, link) {
 		/* Allocate new secvar using actual data size */
-		tmp = alloc_secvar(node->var->data_size);
-		if (!tmp)
-			return OPAL_NO_MEM;
-
-		/* Copy over flags metadata */
-		tmp->flags = node->flags;
-
-		/* Full-clone over the secvar struct */
-		memcpy(tmp->var, node->var, tmp->size + sizeof(struct secvar));
-
+		tmp = new_secvar(var->key, var->key_len, var->data,
+				 var->data_size, var->flags);
 		/* Append to new list */
 		list_add_tail(dst, &tmp->link);
 	}
@@ -47,30 +39,38 @@ int copy_bank_list(struct list_head *dst, struct list_head *src)
 	return OPAL_SUCCESS;
 }
 
-struct secvar_node *alloc_secvar(uint64_t size)
+struct secvar *alloc_secvar(uint64_t key_len, uint64_t data_size)
 {
-	struct secvar_node *ret;
+	struct secvar *ret;
 
-	ret = zalloc(sizeof(struct secvar_node));
+	ret = zalloc(sizeof(struct secvar));
 	if (!ret)
 		return NULL;
 
-	ret->var = zalloc(sizeof(struct secvar) + size);
-	if (!ret->var) {
+	ret->key = zalloc(key_len);
+	if (!ret->key) {
+		free(ret->key);
+		return NULL;
+	}
+
+	ret->data = zalloc(data_size);
+	if (!ret->data) {
+		free(ret->key);
 		free(ret);
 		return NULL;
 	}
 
-	ret->size = size;
+	ret->key_len = key_len;
+	ret->data_size = data_size;
 
 	return ret;
 }
 
-struct secvar_node *new_secvar(const char *key, uint64_t key_len,
+struct secvar *new_secvar(const char *key, uint64_t key_len,
 			       const char *data, uint64_t data_size,
 			       uint64_t flags)
 {
-	struct secvar_node *ret;
+	struct secvar *ret;
 
 	if (!key)
 		return NULL;
@@ -79,58 +79,57 @@ struct secvar_node *new_secvar(const char *key, uint64_t key_len,
 	if ((!data) && (data_size))
 		return NULL;
 
-	ret = alloc_secvar(data_size);
+	ret = alloc_secvar(key_len, data_size);
 	if (!ret)
 		return NULL;
 
-	ret->var->key_len = key_len;
-	ret->var->data_size = data_size;
-	memcpy(ret->var->key, key, key_len);
+	memcpy(ret->key, key, key_len);
 	ret->flags = flags;
 
 	if (data)
-		memcpy(ret->var->data, data, data_size);
+		memcpy(ret->data, data, data_size);
 
 	return ret;
 }
 
-int realloc_secvar(struct secvar_node *node, uint64_t size)
+int realloc_secvar(struct secvar *var, uint64_t size)
 {
 	void *tmp;
 
-	if (node->size >= size)
+	if (var->data_size >= size)
 		return 0;
 
-	tmp = zalloc(sizeof(struct secvar) + size);
+	tmp = zalloc(size);
 	if (!tmp)
 		return -1;
 
-	memcpy(tmp, node->var, sizeof(struct secvar) + node->size);
-	free(node->var);
-	node->var = tmp;
+	memcpy(tmp, var->data, var->data_size);
+	free(var->data);
+	var->data = tmp;
 
 	return 0;
 }
 
-void dealloc_secvar(struct secvar_node *node)
+void dealloc_secvar(struct secvar *var)
 {
-	if (!node)
+	if (!var)
 		return;
 
-	free(node->var);
-	free(node);
+	free(var->key);
+	free(var->data);
+	free(var);
 }
 
-struct secvar_node *find_secvar(const char *key, uint64_t key_len, struct list_head *bank)
+struct secvar *find_secvar(const char *key, uint64_t key_len, struct list_head *bank)
 {
-	struct secvar_node *node = NULL;
+	struct secvar *var = NULL;
 
-	list_for_each(bank, node, link) {
+	list_for_each(bank, var, link) {
 		// Prevent matching shorter key subsets / bail early
-		if (key_len != node->var->key_len)
+		if (key_len != var->key_len)
 			continue;
-		if (!memcmp(key, node->var->key, key_len))
-			return node;
+		if (!memcmp(key, var->key, key_len))
+			return var;
 	}
 
 	return NULL;
@@ -150,9 +149,9 @@ int is_key_empty(const char *key, uint64_t key_len)
 int list_length(struct list_head *bank)
 {
 	int ret = 0;
-	struct secvar_node *node;
+	struct secvar *var;
 
-	list_for_each(bank, node, link)
+	list_for_each(bank, var, link)
 		ret++;
 
 	return ret;