| Message ID | 20230929045724.6844-1-k.kahurani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xenstore: Avoid leaking memory in check_store | expand |
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
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); }
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(-)