While this code should not be trigger-able from an unprivileged domain
it is called multiple times when the database gets inconsistent. This
means that a malicious guest able to corrupt the database will trigger
the leaks here.

Fix the leaks so that this code can be safely called from anywhere

Signed-off-by: David Kahurani <k.kahurani@gmail.com>
---
 tools/xenstored/core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 092de76a2e..edd07711db 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -2535,18 +2535,18 @@ static void clean_store(struct check_store_data *data)
 	domain_check_acc(data->domains);
 }
 
-int check_store_path(const char *name, struct check_store_data *data)
+int check_store_path(const void *ctx, const char *name, struct check_store_data *data)
 {
 	struct node *node;
 
-	node = read_node(NULL, NULL, name);
+	node = read_node(NULL, ctx, name);
 	if (!node) {
 		log("check_store: error %d reading special node '%s'", errno,
 		    name);
 		return errno;
 	}
 
-	return check_store_step(NULL, NULL, node, data);
+	return check_store_step(ctx, NULL, node, data);
 }
 
 void check_store(void)
@@ -2556,6 +2556,7 @@ void check_store(void)
 		.enoent = check_store_enoent,
 	};
 	struct check_store_data data;
+	void *ctx;
 
 	/* Don't free values (they are all void *1) */
 	data.reachable = create_hashtable(NULL, "checkstore", hash_from_key_fn,
@@ -2571,17 +2572,19 @@ void check_store(void)
 		goto out_hash;
 	}
 
+	ctx = talloc_new(NULL);
 	log("Checking store ...");
-	if (walk_node_tree(NULL, NULL, "/", &walkfuncs, &data)) {
+	if (walk_node_tree(ctx, NULL, "/", &walkfuncs, &data)) {
 		if (errno == ENOMEM)
 			log("check_store: ENOMEM");
-	} else if (!check_store_path("@introduceDomain", &data) &&
-		   !check_store_path("@releaseDomain", &data) &&
+	} else if (!check_store_path(ctx, "@introduceDomain", &data) &&
+		   !check_store_path(ctx, "@releaseDomain", &data) &&
 		   !check_transactions(data.reachable))
 		clean_store(&data);
 	log("Checking store complete.");
 
 	hashtable_destroy(data.domains);
+	talloc_free(ctx);
  out_hash:
 	hashtable_destroy(data.reachable);
 }
-- 
2.25.1

On 29.09.23 06:57, David Kahurani wrote:

I'd like the following paragraph added to the commit message:

   check_store() will leak the memory from reading the "@introduceDomain"
   and "@releaseDomain" nodes.

> While this code should not be trigger-able from an unprivileged domain
> it is called multiple times when the database gets inconsistent. This
> means that a malicious guest able to corrupt the database will trigger
> the leaks here.
> 
> Fix the leaks so that this code can be safely called from anywhere
> 

Fixes: 67617067f0b6 ("tools/xenstore: let check_store() check the accounting data")

> Signed-off-by: David Kahurani <k.kahurani@gmail.com>

With above additions (can probably be done while committing):

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

On 29/09/2023 6:56 am, Juergen Gross wrote:
> On 29.09.23 06:57, David Kahurani wrote:
>
> I'd like the following paragraph added to the commit message:
>
>   check_store() will leak the memory from reading the "@introduceDomain"
>   and "@releaseDomain" nodes.
>
>> While this code should not be trigger-able from an unprivileged domain
>> it is called multiple times when the database gets inconsistent. This
>> means that a malicious guest able to corrupt the database will trigger
>> the leaks here.
>>
>> Fix the leaks so that this code can be safely called from anywhere
>>
>
> Fixes: 67617067f0b6 ("tools/xenstore: let check_store() check the
> accounting data")
>
>> Signed-off-by: David Kahurani <k.kahurani@gmail.com>
>
> With above additions (can probably be done while committing):
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Can do, but this needs Henry's approval too at this point in 4.18

~Andrew

Hi all,

> On Sep 29, 2023, at 16:23, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 29/09/2023 6:56 am, Juergen Gross wrote:
>> On 29.09.23 06:57, David Kahurani wrote:
>> 
>> I'd like the following paragraph added to the commit message:
>> 
>>   check_store() will leak the memory from reading the "@introduceDomain"
>>   and "@releaseDomain" nodes.
>> 
>>> While this code should not be trigger-able from an unprivileged domain
>>> it is called multiple times when the database gets inconsistent. This
>>> means that a malicious guest able to corrupt the database will trigger
>>> the leaks here.
>>> 
>>> Fix the leaks so that this code can be safely called from anywhere
>>> 
>> 
>> Fixes: 67617067f0b6 ("tools/xenstore: let check_store() check the
>> accounting data")
>> 
>>> Signed-off-by: David Kahurani <k.kahurani@gmail.com>
>> 
>> With above additions (can probably be done while committing):
>> 
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Can do, but this needs Henry's approval too at this point in 4.18

This is definitely a patch that should go with 4.18 so:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry


> 
> ~Andrew