Commit 562a1c0f7ef3fb ("tools/xenstore: dont unlink connection object
twice") introduced a potential use after free problem in
domain_cleanup(): after calling talloc_unlink() for domain->conn
domain->conn is set to NULL. The problem is that domain is registered
as talloc child of domain->conn, so it might be freed by the
talloc_unlink() call.

With Xenstore being single threaded there are normally no concurrent
memory allocations running and freeing a virtual memory area normally
doesn't result in that area no longer being accessible. A problem
could occur only in case either a signal received results in some
memory allocation done in the signal handler (SIGHUP is a primary
candidate leading to reopening the log file), or in case the talloc
framework would do some internal memory allocation during freeing of
the memory (which would lead to clobbering of the freed domain
structure).

Fixes: 562a1c0f7ef3fb ("tools/xenstore: dont unlink connection object twice")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_domain.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index baddaba5df..5858185211 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -214,6 +214,7 @@ static void domain_cleanup(void)
 {
 	xc_dominfo_t dominfo;
 	struct domain *domain;
+	struct connection *conn;
 	int notify = 0;
 
  again:
@@ -230,8 +231,10 @@ static void domain_cleanup(void)
 				continue;
 		}
 		if (domain->conn) {
-			talloc_unlink(talloc_autofree_context(), domain->conn);
+			/* domain is a talloc child of domain->conn. */
+			conn = domain->conn;
 			domain->conn = NULL;
+			talloc_unlink(talloc_autofree_context(), conn);
 			notify = 0; /* destroy_domain() fires the watch */
 			goto again;
 		}
-- 
2.16.4

Hi Juergen,

On 03/04/2020 13:03, Juergen Gross wrote:
> Commit 562a1c0f7ef3fb ("tools/xenstore: dont unlink connection object
> twice") introduced a potential use after free problem in
> domain_cleanup(): after calling talloc_unlink() for domain->conn
> domain->conn is set to NULL. The problem is that domain is registered
> as talloc child of domain->conn, so it might be freed by the
> talloc_unlink() call.
> 
> With Xenstore being single threaded there are normally no concurrent
> memory allocations running and freeing a virtual memory area normally
> doesn't result in that area no longer being accessible. A problem
> could occur only in case either a signal received results in some
> memory allocation done in the signal handler (SIGHUP is a primary
> candidate leading to reopening the log file), or in case the talloc
> framework would do some internal memory allocation during freeing of
> the memory (which would lead to clobbering of the freed domain
> structure).

Thank you for writing more context!

> 
> Fixes: 562a1c0f7ef3fb ("tools/xenstore: dont unlink connection object twice")
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Julien Grall writes ("Re: [PATCH v2] tools/xenstore: fix a use after free problem in xenstored"):
> On 03/04/2020 13:03, Juergen Gross wrote:
> > Commit 562a1c0f7ef3fb ("tools/xenstore: dont unlink connection object
> > twice") introduced a potential use after free problem in
> > domain_cleanup(): after calling talloc_unlink() for domain->conn
> > domain->conn is set to NULL. The problem is that domain is registered
> > as talloc child of domain->conn, so it might be freed by the
> > talloc_unlink() call.
> > 
> > With Xenstore being single threaded there are normally no concurrent
> > memory allocations running and freeing a virtual memory area normally
> > doesn't result in that area no longer being accessible. A problem
> > could occur only in case either a signal received results in some
> > memory allocation done in the signal handler (SIGHUP is a primary
> > candidate leading to reopening the log file), or in case the talloc
> > framework would do some internal memory allocation during freeing of
> > the memory (which would lead to clobbering of the freed domain
> > structure).
> 
> Thank you for writing more context!
> 
> > 
> > Fixes: 562a1c0f7ef3fb ("tools/xenstore: dont unlink connection object twice")
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Pushed, thanks both.

Ian.