tools/xenstore: domain can sometimes disappear when destroying connection

There is a circular link formed between domain and a connection. In certain circustances, when conn is freed, domain is also freed, which leads to use

after free when trying to set the conn field in domain to null.

Signed-off-by Petre Eftime <[email protected]>

---
 tools/xenstore/xenstored_domain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

tools/xenstore/xenstored_domain.c +0 -0

index fa6655033a..f085d40476 100644

tools/xenstore/xenstored_domain.c +7 -1

 	int notify = 0;
 
  again:

@@ -238,8 +239,14 @@ static void domain_cleanup(void)
 				continue;
 		}
 		if (domain->conn) {
-			talloc_unlink(talloc_autofree_context(), domain->conn);
+			/*
+			 * In certain circumstances conn owns domain and
+			 * domain will be freed when conn is unlinked.
+			 */
+			tmp_conn = domain->conn;
 			domain->conn = NULL;
+
+			talloc_unlink(talloc_autofree_context(), tmp_conn);
 			notify = 0; /* destroy_domain() fires the watch */
 			goto again;
 		}

Signature · 6 lines

2.16.5




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

There is a circular link formed between domain and a connection. In certain circustances, when conn is freed, domain is also freed, which leads to use after free when trying to set the conn field in domain to null.

Signed-off-by Petre Eftime <[email protected]>

---
 tools/xenstore/xenstored_domain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

tools/xenstore/xenstored_domain.c +8 -1

index fa6655033a..f085d40476 100644
@@ -222,6 +222,7 @@ static void domain_cleanup(void)
 {
 	xc_dominfo_t dominfo;
 	struct domain *domain;
+	struct connection *tmp_conn;
 	int notify = 0;
 
  again:

@@ -238,8 +239,14 @@ static void domain_cleanup(void)
 				continue;
 		}
 		if (domain->conn) {
-			talloc_unlink(talloc_autofree_context(), domain->conn);
+			/*
+			 * In certain circumstances conn owns domain and
+			 * domain will be freed when conn is unlinked.
+			 */
+			tmp_conn = domain->conn;
 			domain->conn = NULL;
+
+			talloc_unlink(talloc_autofree_context(), tmp_conn);
 			notify = 0; /* destroy_domain() fires the watch */
 			goto again;
 		}

Signature · 6 lines

2.16.5




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

I didn't go extremely deep in my debugging, as the talloc library is a bit difficult to debug, but under the do_introduce function you have these two lines:

                 /* Now domain belongs to its connection. */
                 talloc_steal(domain->conn, domain);

After these happen, destroying the domain leads to a SIGSEGV in xenstored, as when conn gets freed, so does domain, which ends up in a use-after-free.

I've posted the patch with the fixed text.

Best,
Petre

On 2018-11-29, 14:54, "Wei Liu" <[email protected]> wrote:

    On Mon, Nov 26, 2018 at 01:22:04PM +0000, Petre Eftime wrote:
    > There is a circular link formed between domain and a connection. In certain
    > circustances, when conn is freed, domain is also freed, which leads to use
    > after free when trying to set the conn field in domain to null.
    
    Actually, can you provide more context on this? When will the circular
    link happen?
    
    Wei.
    
    > 
    > Signed-off-by: Petre Eftime <[email protected]>
    > ---
    >  tools/xenstore/xenstored_domain.c | 9 ++++++++-
    >  1 file changed, 8 insertions(+), 1 deletion(-)
    > 
    > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
    > index fa6655033a..f085d40476 100644
    > --- a/tools/xenstore/xenstored_domain.c
    > +++ b/tools/xenstore/xenstored_domain.c
    > @@ -222,6 +222,7 @@ static void domain_cleanup(void)
    >  {
    >  	xc_dominfo_t dominfo;
    >  	struct domain *domain;
    > +	struct connection *tmp_conn;
    >  	int notify = 0;
    >  
    >   again:
    > @@ -238,8 +239,14 @@ static void domain_cleanup(void)
    >  				continue;
    >  		}
    >  		if (domain->conn) {
    > -			talloc_unlink(talloc_autofree_context(), domain->conn);
    > +			/*
    > +			 * In certain circumstances conn owns domain and
    > +			 * domain will be freed when conn is unlinked.
    > +			 */
    > +			tmp_conn = domain->conn;
    >  			domain->conn = NULL;
    > +
    > +			talloc_unlink(talloc_autofree_context(), tmp_conn);
    >  			notify = 0; /* destroy_domain() fires the watch */
    >  			goto again;
    >  		}
    > -- 
    > 2.16.5
    > 
    > 
    > 
    > 
    > Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
    > 

Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

Quoted 3 lines · Petre

I didn't go extremely deep in my debugging, as the talloc library is a bit difficult to debug, but under the do_introduce function you have these two lines:

difficult to debug> I completely agree. ;-)

Quoted 1 line

Quoted 3 lines

                 /* Now domain belongs to its connection. */
                 talloc_steal(domain->conn, domain);

Thanks, I think this is what I missed. This is useful information. I think it should go into the commit message if we end up committing your patch.