[PATCH] xenstored: allow guests to reintroduce themselves

[PATCH] xenstored: allow guests to reintroduce themselves

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1312202176 -7200
# Node ID edb96c34f4a638e8ba97933b6bd76ff72836353e
# Parent  0f36c2eec2e1576b4db6538b5f22d625587c1a15
xenstored: allow guests to reintroduce themselves

During kexec all old watches have to be removed, otherwise the new
kernel will receive unexpected events. Allow a guest to introduce itself
and cleanup all of its watches.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 0f36c2eec2e1 -r edb96c34f4a6 tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -315,7 +315,7 @@ void do_introduce(struct connection *con
 {
 	struct domain *domain;
 	char *vec[3];
-	unsigned int domid;
+	unsigned int domid, target;
 	unsigned long mfn;
 	evtchn_port_t port;
 	int rc;
@@ -326,7 +326,7 @@ void do_introduce(struct connection *con
 		return;
 	}
 
-	if (conn->id != 0 || !conn->can_write) {
+	if (!conn->can_write) {
 		send_error(conn, EACCES);
 		return;
 	}
@@ -340,19 +340,26 @@ void do_introduce(struct connection *con
 		send_error(conn, EINVAL);
 		return;
 	}
+	/* Allow guest to reset all watches */
+	if (domid != DOMID_SELF && conn->id != 0) {
+		send_error(conn, EACCES);
+		return;
+	}
 
-	domain = find_domain_by_domid(domid);
+	target = domid == DOMID_SELF ? conn->id : domid;
+
+	domain = find_domain_by_domid(target);
 
 	if (domain == NULL) {
 		interface = xc_map_foreign_range(
-			*xc_handle, domid,
+			*xc_handle, target,
 			getpagesize(), PROT_READ|PROT_WRITE, mfn);
 		if (!interface) {
 			send_error(conn, errno);
 			return;
 		}
 		/* Hang domain off "in" until we're finished. */
-		domain = new_domain(in, domid, port);
+		domain = new_domain(in, target, port);
 		if (!domain) {
 			munmap(interface, getpagesize());
 			send_error(conn, errno);
@@ -365,11 +372,11 @@ void do_introduce(struct connection *con
 		talloc_steal(domain->conn, domain);
 
 		fire_watches(NULL, "@introduceDomain", false);
-	} else if ((domain->mfn == mfn) && (domain->conn != conn)) {
+	} else if ((domain->mfn == mfn) && ((domain->conn != conn) || domid == DOMID_SELF)) {
 		/* Use XS_INTRODUCE for recreating the xenbus event-channel. */
 		if (domain->port)
 			xc_evtchn_unbind(xce_handle, domain->port);
-		rc = xc_evtchn_bind_interdomain(xce_handle, domid, port);
+		rc = xc_evtchn_bind_interdomain(xce_handle, target, port);
 		domain->port = (rc == -1) ? 0 : rc;
 		domain->remote_port = port;
 	} else {

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Ian Campbell]

On Mon, 2011-08-01 at 13:38 +0100, Olaf Hering wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1312202176 -7200
> # Node ID edb96c34f4a638e8ba97933b6bd76ff72836353e
> # Parent  0f36c2eec2e1576b4db6538b5f22d625587c1a15
> xenstored: allow guests to reintroduce themselves
> 
> During kexec all old watches have to be removed, otherwise the new
> kernel will receive unexpected events. Allow a guest to introduce itself
> and cleanup all of its watches.

Just out of interest what happens if a guest tries to use this operation
on an older xenstored which does not support it? I guess it gets an
enosys type response?

Is there any way we can arrange to probe for this feature in order to
fail to register for kexec/kdump early on rather than failing at the
point where we attempt to actually kexec (where failure might come as
rather an unpleasant surprise). I don't think we've historically had a
mechanism for negotiating features with xenstored itself so I'm not sure
what would be best here. Perhaps xenstored itself should
expose /xenstored/feature-FOO nodes?

Ian.

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> diff -r 0f36c2eec2e1 -r edb96c34f4a6 tools/xenstore/xenstored_domain.c
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -315,7 +315,7 @@ void do_introduce(struct connection *con
>  {
>  	struct domain *domain;
>  	char *vec[3];
> -	unsigned int domid;
> +	unsigned int domid, target;
>  	unsigned long mfn;
>  	evtchn_port_t port;
>  	int rc;
> @@ -326,7 +326,7 @@ void do_introduce(struct connection *con
>  		return;
>  	}
>  
> -	if (conn->id != 0 || !conn->can_write) {
> +	if (!conn->can_write) {
>  		send_error(conn, EACCES);
>  		return;
>  	}
> @@ -340,19 +340,26 @@ void do_introduce(struct connection *con
>  		send_error(conn, EINVAL);
>  		return;
>  	}
> +	/* Allow guest to reset all watches */
> +	if (domid != DOMID_SELF && conn->id != 0) {
> +		send_error(conn, EACCES);
> +		return;
> +	}
>  
> -	domain = find_domain_by_domid(domid);
> +	target = domid == DOMID_SELF ? conn->id : domid;
> +
> +	domain = find_domain_by_domid(target);
>  
>  	if (domain == NULL) {
>  		interface = xc_map_foreign_range(
> -			*xc_handle, domid,
> +			*xc_handle, target,
>  			getpagesize(), PROT_READ|PROT_WRITE, mfn);
>  		if (!interface) {
>  			send_error(conn, errno);
>  			return;
>  		}
>  		/* Hang domain off "in" until we're finished. */
> -		domain = new_domain(in, domid, port);
> +		domain = new_domain(in, target, port);
>  		if (!domain) {
>  			munmap(interface, getpagesize());
>  			send_error(conn, errno);
> @@ -365,11 +372,11 @@ void do_introduce(struct connection *con
>  		talloc_steal(domain->conn, domain);
>  
>  		fire_watches(NULL, "@introduceDomain", false);
> -	} else if ((domain->mfn == mfn) && (domain->conn != conn)) {
> +	} else if ((domain->mfn == mfn) && ((domain->conn != conn) || domid == DOMID_SELF)) {
>  		/* Use XS_INTRODUCE for recreating the xenbus event-channel. */
>  		if (domain->port)
>  			xc_evtchn_unbind(xce_handle, domain->port);
> -		rc = xc_evtchn_bind_interdomain(xce_handle, domid, port);
> +		rc = xc_evtchn_bind_interdomain(xce_handle, target, port);
>  		domain->port = (rc == -1) ? 0 : rc;
>  		domain->remote_port = port;
>  	} else {
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Olaf Hering]

On Tue, Aug 09, Ian Campbell wrote:

> On Mon, 2011-08-01 at 13:38 +0100, Olaf Hering wrote:
> > # HG changeset patch
> > # User Olaf Hering <olaf@aepfle.de>
> > # Date 1312202176 -7200
> > # Node ID edb96c34f4a638e8ba97933b6bd76ff72836353e
> > # Parent  0f36c2eec2e1576b4db6538b5f22d625587c1a15
> > xenstored: allow guests to reintroduce themselves
> > 
> > During kexec all old watches have to be removed, otherwise the new
> > kernel will receive unexpected events. Allow a guest to introduce itself
> > and cleanup all of its watches.
> 
> Just out of interest what happens if a guest tries to use this operation
> on an older xenstored which does not support it? I guess it gets an
> enosys type response?

It does, conn->id is not zero and the modified function returns early.

> Is there any way we can arrange to probe for this feature in order to
> fail to register for kexec/kdump early on rather than failing at the
> point where we attempt to actually kexec (where failure might come as
> rather an unpleasant surprise). I don't think we've historically had a
> mechanism for negotiating features with xenstored itself so I'm not sure
> what would be best here. Perhaps xenstored itself should
> expose /xenstored/feature-FOO nodes?

This patch is only for kexec boots, with kdump the crash in the kdump
kernel may happen as well but so far I have not seen it. Maybe because
the kdump kernel runs in its own memory range.

If you prefer, kexec can be modified to check for certain xenstored
properties.

Olaf

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Ian Campbell]

On Tue, 2011-08-09 at 10:17 +0100, Olaf Hering wrote:
> On Tue, Aug 09, Ian Campbell wrote:
> 
> > On Mon, 2011-08-01 at 13:38 +0100, Olaf Hering wrote:
> > > # HG changeset patch
> > > # User Olaf Hering <olaf@aepfle.de>
> > > # Date 1312202176 -7200
> > > # Node ID edb96c34f4a638e8ba97933b6bd76ff72836353e
> > > # Parent  0f36c2eec2e1576b4db6538b5f22d625587c1a15
> > > xenstored: allow guests to reintroduce themselves
> > > 
> > > During kexec all old watches have to be removed, otherwise the new
> > > kernel will receive unexpected events. Allow a guest to introduce itself
> > > and cleanup all of its watches.
> > 
> > Just out of interest what happens if a guest tries to use this operation
> > on an older xenstored which does not support it? I guess it gets an
> > enosys type response?
> 
> It does, conn->id is not zero and the modified function returns early.
> 
> > Is there any way we can arrange to probe for this feature in order to
> > fail to register for kexec/kdump early on rather than failing at the
> > point where we attempt to actually kexec (where failure might come as
> > rather an unpleasant surprise). I don't think we've historically had a
> > mechanism for negotiating features with xenstored itself so I'm not sure
> > what would be best here. Perhaps xenstored itself should
> > expose /xenstored/feature-FOO nodes?
> 
> This patch is only for kexec boots, with kdump the crash in the kdump
> kernel may happen as well but so far I have not seen it. Maybe because
> the kdump kernel runs in its own memory range.

Right, part of the check for an existing watch is to check the "token"
and since the token is usually a pointer to kernel memory it will
probably be different for the kdump kernel. Although not necessarily if
you are unlucky or your kdump kernel is the same as the crashing kernel
but in different physical address (since it is the virtual addresses in
the pointer which matter).

> If you prefer, kexec can be modified to check for certain xenstored
> properties.

That might be a good place to check, although I think we are still
missing the mechanism to expose such properties...

Ian.

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Olaf Hering]

On Tue, Aug 09, Ian Campbell wrote:

> > This patch is only for kexec boots, with kdump the crash in the kdump
> > kernel may happen as well but so far I have not seen it. Maybe because
> > the kdump kernel runs in its own memory range.

I think the kernel still has some bug with watch handling, since the
crash is (in my testing) a jump to NULL. Its worth debugging that part.

> > If you prefer, kexec can be modified to check for certain xenstored
> > properties.
> 
> That might be a good place to check, although I think we are still
> missing the mechanism to expose such properties...

Is there no way for an application running in a guest to query xenstore
properties?

Olaf

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Vincent Hanquez]

On 08/01/2011 01:38 PM, Olaf Hering wrote:
> # HG changeset patch
> # User Olaf Hering<olaf@aepfle.de>
> # Date 1312202176 -7200
> # Node ID edb96c34f4a638e8ba97933b6bd76ff72836353e
> # Parent  0f36c2eec2e1576b4db6538b5f22d625587c1a15
> xenstored: allow guests to reintroduce themselves
>
> During kexec all old watches have to be removed, otherwise the new
> kernel will receive unexpected events. Allow a guest to introduce itself
> and cleanup all of its watches.

What about security wise ?

Guest userspace suddenly becomes able to do this operation (and DoS themself)
where they used to be limited to normal read/write/.. operations.

Also you're changing the C xenstored behavior without changing
the OCaml one.

-- 
Vincent

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Keir Fraser]

On 09/08/2011 11:08, "Vincent Hanquez" <vincent.hanquez@eu.citrix.com>
wrote:

>> xenstored: allow guests to reintroduce themselves
>> 
>> During kexec all old watches have to be removed, otherwise the new
>> kernel will receive unexpected events. Allow a guest to introduce itself
>> and cleanup all of its watches.
> 
> What about security wise ?
> 
> Guest userspace suddenly becomes able to do this operation (and DoS themself)
> where they used to be limited to normal read/write/.. operations.

Guest userspace can already DoS the guest if it has access to xenstore, by
messing with xenbus I/O connections, for example.

 -- Keir

> Also you're changing the C xenstored behavior without changing
> the OCaml one.

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Vincent Hanquez]

On 08/09/2011 11:14 AM, Keir Fraser wrote:
> On 09/08/2011 11:08, "Vincent Hanquez"<vincent.hanquez@eu.citrix.com>
> wrote:
>
>>> xenstored: allow guests to reintroduce themselves
>>>
>>> During kexec all old watches have to be removed, otherwise the new
>>> kernel will receive unexpected events. Allow a guest to introduce itself
>>> and cleanup all of its watches.
>>
>> What about security wise ?
>>
>> Guest userspace suddenly becomes able to do this operation (and DoS themself)
>> where they used to be limited to normal read/write/.. operations.
>
> Guest userspace can already DoS the guest if it has access to xenstore, by
> messing with xenbus I/O connections, for example.

How so ?
It seems we validate userspace packets (at least on linux) before actually putting them on the ring.

-- 
Vincent

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Keir Fraser]

On 09/08/2011 11:50, "Vincent Hanquez" <vincent.hanquez@eu.citrix.com>
wrote:

> On 08/09/2011 11:14 AM, Keir Fraser wrote:
>> On 09/08/2011 11:08, "Vincent Hanquez"<vincent.hanquez@eu.citrix.com>
>> wrote:
>> 
>>>> xenstored: allow guests to reintroduce themselves
>>>> 
>>>> During kexec all old watches have to be removed, otherwise the new
>>>> kernel will receive unexpected events. Allow a guest to introduce itself
>>>> and cleanup all of its watches.
>>> 
>>> What about security wise ?
>>> 
>>> Guest userspace suddenly becomes able to do this operation (and DoS
>>> themself)
>>> where they used to be limited to normal read/write/.. operations.
>> 
>> Guest userspace can already DoS the guest if it has access to xenstore, by
>> messing with xenbus I/O connections, for example.
> 
> How so ?
> It seems we validate userspace packets (at least on linux) before actually
> putting them on the ring.

I don't believe we filter which nodes can be written by userspace. So can
just mess with things like the frontend connection state node for
block/network connections, or whatever. Be imaginative -- there's no doubt
lots of scope for screwing up xenbus connections by fooling around with the
frontend state. If userspace connections to xenbus were not trusted, we'd
need a lot more filtering than we have.

 -- Keir

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Vincent Hanquez]

On 08/09/2011 12:00 PM, Keir Fraser wrote:
>If userspace connections to xenbus were not trusted, we'd
> need a lot more filtering than we have.

I don't think people that are using it in guest userspace (quite liberally)
have necessarily realized this.

-- 
Vincent

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Keir Fraser]

On 09/08/2011 12:18, "Vincent Hanquez" <vincent.hanquez@eu.citrix.com>
wrote:

> On 08/09/2011 12:00 PM, Keir Fraser wrote:
>> If userspace connections to xenbus were not trusted, we'd
>> need a lot more filtering than we have.
> 
> I don't think people that are using it in guest userspace (quite liberally)
> have necessarily realized this.

Well, you do need to be root (at least by default) to access the xenstore
device, and there are myriad other ways for a root process to break the
guest. Admittedly you could start as root and then deprivilege yourself, in
which case the xenstore conenction would be an ongoing point of excess
privilege.

Do you have any examples of projects which could run with much lesser
privilege, and very constrained xenstore access, if a suitably controlled
xenstore interface was provided?

 -- Keir

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Vincent Hanquez]

On 08/09/2011 12:31 PM, Keir Fraser wrote:
> Do you have any examples of projects which could run with much lesser
> privilege, and very constrained xenstore access, if a suitably controlled
> xenstore interface was provided?

There's a bunch of program that doesn't need much more than read/write to a specific limited part of xenstore.

- Guest agents (reporting stats usually)
- things listening to some actions (snapshot yourself, export some storage thing, etc..)

Perhaps a variant of the restrict packet would be enough to drop some privileges
of the xenbus connection (at connection time) to read/write to a specific path.

-- 
Vincent

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Vincent Hanquez]

On 08/09/2011 11:14 AM, Keir Fraser wrote:
> On 09/08/2011 11:08, "Vincent Hanquez"<vincent.hanquez@eu.citrix.com>
> wrote:
>
>>> xenstored: allow guests to reintroduce themselves
>>>
>>> During kexec all old watches have to be removed, otherwise the new
>>> kernel will receive unexpected events. Allow a guest to introduce itself
>>> and cleanup all of its watches.
>>
>> What about security wise ?
>>
>> Guest userspace suddenly becomes able to do this operation (and DoS themself)
>> where they used to be limited to normal read/write/.. operations.
>
> Guest userspace can already DoS the guest if it has access to xenstore, by
> messing with xenbus I/O connections, for example.

Re-reading i suspect you mean the xenbus devices here.
In that case, yes, you can probably do some damages here already.


-- 
Vincent

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Olaf Hering]

On Tue, Aug 09, Vincent Hanquez wrote:

> What about security wise ?

Its not about security, just the usual UNIX gun->foot thing.

> Guest userspace suddenly becomes able to do this operation (and DoS themself)
> where they used to be limited to normal read/write/.. operations.

The guest userspace does most likely not talk to xenstored directly.
Whatever acts as the proxy could filter the XS_INTRODUCE command.

> Also you're changing the C xenstored behavior without changing
> the OCaml one.

I better leave that to the maintainers of that code.

Olaf

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Ian Jackson]

Olaf Hering writes ("[Xen-devel] [PATCH] xenstored: allow guests to reintroduce themselves"):
> xenstored: allow guests to reintroduce themselves
> 
> During kexec all old watches have to be removed, otherwise the new
> kernel will receive unexpected events. Allow a guest to introduce itself
> and cleanup all of its watches.

If the purpose of this is simply to allow the guest to discard all its
watches, I think a new xenstore command is a better plan than this
change.

In particular, previously, only dom0 was allowed to use INTRODUCE.

I'm not convinced that we have fully throught through the implications
of allowing guests to do it under some circumstances, and I can see no
reason for trying to think about those implications when allowing a
guest to delete all its watches would be straightforward.

Ian.

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Ian Jackson]

I wrote:
> If the purpose of this is simply to allow the guest to discard all its
> watches, I think a new xenstore command is a better plan than this
> change.
> 
> In particular, previously, only dom0 was allowed to use INTRODUCE.
> 
> I'm not convinced that we have fully throught through the implications
> of allowing guests to do it under some circumstances, and I can see no
> reason for trying to think about those implications when allowing a
> guest to delete all its watches would be straightforward.

Also, whatever you do, the xenstore protocol is currently properly
documented (!) in docs/misc/xenstore.txt.

If you are making a protocol change your patch should modify the
protocol document to fully explain the new syntax and/or semantics, as
applicable.

Ian.

Re: [PATCH] xenstored: allow guests to reintroduce themselves

[Ian Jackson]

Olaf Hering writes ("[Xen-devel] [PATCH] xenstored: allow guests to reintroduce themselves"):
> xenstored: allow guests to reintroduce themselves
> 
> During kexec all old watches have to be removed, otherwise the new
> kernel will receive unexpected events. Allow a guest to introduce itself
> and cleanup all of its watches.

I see this patch was already applied.  For the reasons explained in
the thread by myself and others, I don't think it was ready.  It
doesn't seem to have had an appropriate ack.  I have therefore
reverted it, pending further discussion.

Thanks,
Ian.