[PATCH 0/3] xen: fix some minor bugs and cleanup of xenbus

[PATCH 0/3] xen: fix some minor bugs and cleanup of xenbus

[Juergen Gross]

Do some minor bug fixes and cleanup of xenbus driver.

Juergen Gross (3):
  xen: xenbus driver must not accept invalid transaction ids
  xen: return xenstore command failures via response instead of rc
  xen: remove stale xs_input_avail() from header

 drivers/xen/xenbus/xenbus_comms.h        |  1 -
 drivers/xen/xenbus/xenbus_dev_frontend.c | 49 ++++++++++++++++++--------------
 2 files changed, 28 insertions(+), 22 deletions(-)

-- 
2.10.2

[PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids

[Juergen Gross]

When accessing Xenstore in a transaction the user is specifying a
transaction id which he normally obtained from Xenstore when starting
the transaction. Xenstore is validating a transaction id against all
known transaction ids of the connection the request came in. As all
requests of a domain not being the one where Xenstore lives share
one connection, validation of transaction ids of different users of
Xenstore in that domain should be done by the kernel of that domain
being the multiplexer between the Xenstore users in that domain and
Xenstore.

In order to prohibit one Xenstore user to be able to "hijack" a
transaction from another user the xenbus driver has to verify a
given transaction id against all known transaction ids of the user
before forwarding it to Xenstore.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xenbus/xenbus_dev_frontend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index 6c0ead4..a068281 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -316,7 +316,7 @@ static int xenbus_write_transaction(unsigned msg_type,
 			rc = -ENOMEM;
 			goto out;
 		}
-	} else if (msg_type == XS_TRANSACTION_END) {
+	} else if (u->u.msg.tx_id != 0) {
 		list_for_each_entry(trans, &u->transactions, list)
 			if (trans->handle.id == u->u.msg.tx_id)
 				break;
-- 
2.10.2

Re: [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids

[Boris Ostrovsky]

On 12/22/2016 02:19 AM, Juergen Gross wrote:
> When accessing Xenstore in a transaction the user is specifying a
> transaction id which he normally obtained from Xenstore when starting
> the transaction. Xenstore is validating a transaction id against all
> known transaction ids of the connection the request came in. As all
> requests of a domain not being the one where Xenstore lives share
> one connection, validation of transaction ids of different users of
> Xenstore in that domain should be done by the kernel of that domain
> being the multiplexer between the Xenstore users in that domain and
> Xenstore.
>
> In order to prohibit one Xenstore user to be able to "hijack" a
> transaction from another user the xenbus driver has to verify a
> given transaction id against all known transaction ids of the user
> before forwarding it to Xenstore.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>


Should this go to stable trees as well?


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids

[Juergen Gross]

On 22/12/16 16:38, Boris Ostrovsky wrote:
> On 12/22/2016 02:19 AM, Juergen Gross wrote:
>> When accessing Xenstore in a transaction the user is specifying a
>> transaction id which he normally obtained from Xenstore when starting
>> the transaction. Xenstore is validating a transaction id against all
>> known transaction ids of the connection the request came in. As all
>> requests of a domain not being the one where Xenstore lives share
>> one connection, validation of transaction ids of different users of
>> Xenstore in that domain should be done by the kernel of that domain
>> being the multiplexer between the Xenstore users in that domain and
>> Xenstore.
>>
>> In order to prohibit one Xenstore user to be able to "hijack" a
>> transaction from another user the xenbus driver has to verify a
>> given transaction id against all known transaction ids of the user
>> before forwarding it to Xenstore.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> 
> Should this go to stable trees as well?

I don't think it is necessary. First I thought this could be a security
problem, but any user who could make use of that problem could easily
trash complete Xenstore, so there are no additional security concerns
with this "bug" not being handled.

After all it is just a matter of avoiding problems due to buggy Xenstore
users which are probably not existing at all. :-)

> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Thanks,

Juergen

[PATCH 2/3] xen: return xenstore command failures via response instead of rc

[Juergen Gross]

When the xenbus driver does some special handling for a Xenstore
command any error condition related to the command should be returned
via an error response instead of letting the related write operation
fail. Otherwise the user land handler might take wrong decisions
assuming the connection to Xenstore is broken.

While at it try to return the same error values xenstored would
return for those cases.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xenbus/xenbus_dev_frontend.c | 47 ++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index a068281..79130b3 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -302,6 +302,29 @@ static void watch_fired(struct xenbus_watch *watch,
 	mutex_unlock(&adap->dev_data->reply_mutex);
 }
 
+static int xenbus_command_reply(struct xenbus_file_priv *u,
+				unsigned int msg_type, const char *reply)
+{
+	struct {
+		struct xsd_sockmsg hdr;
+		const char body[16];
+	} msg;
+	int rc;
+
+	msg.hdr = u->u.msg;
+	msg.hdr.type = msg_type;
+	msg.hdr.len = strlen(reply) + 1;
+	if (msg.hdr.len > sizeof(msg.body))
+		return -E2BIG;
+
+	mutex_lock(&u->reply_mutex);
+	rc = queue_reply(&u->read_buffers, &msg, sizeof(msg.hdr) + msg.hdr.len);
+	wake_up(&u->read_waitq);
+	mutex_unlock(&u->reply_mutex);
+
+	return rc;
+}
+
 static int xenbus_write_transaction(unsigned msg_type,
 				    struct xenbus_file_priv *u)
 {
@@ -321,7 +344,7 @@ static int xenbus_write_transaction(unsigned msg_type,
 			if (trans->handle.id == u->u.msg.tx_id)
 				break;
 		if (&trans->list == &u->transactions)
-			return -ESRCH;
+			return xenbus_command_reply(u, XS_ERROR, "ENOENT");
 	}
 
 	reply = xenbus_dev_request_and_reply(&u->u.msg);
@@ -372,12 +395,12 @@ static int xenbus_write_watch(unsigned msg_type, struct xenbus_file_priv *u)
 	path = u->u.buffer + sizeof(u->u.msg);
 	token = memchr(path, 0, u->u.msg.len);
 	if (token == NULL) {
-		rc = -EILSEQ;
+		rc = xenbus_command_reply(u, XS_ERROR, "EINVAL");
 		goto out;
 	}
 	token++;
 	if (memchr(token, 0, u->u.msg.len - (token - path)) == NULL) {
-		rc = -EILSEQ;
+		rc = xenbus_command_reply(u, XS_ERROR, "EINVAL");
 		goto out;
 	}
 
@@ -411,23 +434,7 @@ static int xenbus_write_watch(unsigned msg_type, struct xenbus_file_priv *u)
 	}
 
 	/* Success.  Synthesize a reply to say all is OK. */
-	{
-		struct {
-			struct xsd_sockmsg hdr;
-			char body[3];
-		} __packed reply = {
-			{
-				.type = msg_type,
-				.len = sizeof(reply.body)
-			},
-			"OK"
-		};
-
-		mutex_lock(&u->reply_mutex);
-		rc = queue_reply(&u->read_buffers, &reply, sizeof(reply));
-		wake_up(&u->read_waitq);
-		mutex_unlock(&u->reply_mutex);
-	}
+	rc = xenbus_command_reply(u, msg_type, "OK");
 
 out:
 	return rc;
-- 
2.10.2

Re: [PATCH 2/3] xen: return xenstore command failures via response instead of rc

[Boris Ostrovsky]

On 12/22/2016 02:19 AM, Juergen Gross wrote:
> When the xenbus driver does some special handling for a Xenstore
> command any error condition related to the command should be returned
> via an error response instead of letting the related write operation
> fail. Otherwise the user land handler might take wrong decisions
> assuming the connection to Xenstore is broken.

Do we expect the user to always read the reply if no error code was
returned?

-boris

>
> While at it try to return the same error values xenstored would
> return for those cases.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xenbus/xenbus_dev_frontend.c | 47 ++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
> index a068281..79130b3 100644
> --- a/drivers/xen/xenbus/xenbus_dev_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
> @@ -302,6 +302,29 @@ static void watch_fired(struct xenbus_watch *watch,
>  	mutex_unlock(&adap->dev_data->reply_mutex);
>  }
>  
> +static int xenbus_command_reply(struct xenbus_file_priv *u,
> +				unsigned int msg_type, const char *reply)
> +{
> +	struct {
> +		struct xsd_sockmsg hdr;
> +		const char body[16];
> +	} msg;
> +	int rc;
> +
> +	msg.hdr = u->u.msg;
> +	msg.hdr.type = msg_type;
> +	msg.hdr.len = strlen(reply) + 1;
> +	if (msg.hdr.len > sizeof(msg.body))
> +		return -E2BIG;
> +
> +	mutex_lock(&u->reply_mutex);
> +	rc = queue_reply(&u->read_buffers, &msg, sizeof(msg.hdr) + msg.hdr.len);
> +	wake_up(&u->read_waitq);
> +	mutex_unlock(&u->reply_mutex);
> +
> +	return rc;
> +}
> +
>  static int xenbus_write_transaction(unsigned msg_type,
>  				    struct xenbus_file_priv *u)
>  {
> @@ -321,7 +344,7 @@ static int xenbus_write_transaction(unsigned msg_type,
>  			if (trans->handle.id == u->u.msg.tx_id)
>  				break;
>  		if (&trans->list == &u->transactions)
> -			return -ESRCH;
> +			return xenbus_command_reply(u, XS_ERROR, "ENOENT");
>  	}
>  
>  	reply = xenbus_dev_request_and_reply(&u->u.msg);
> @@ -372,12 +395,12 @@ static int xenbus_write_watch(unsigned msg_type, struct xenbus_file_priv *u)
>  	path = u->u.buffer + sizeof(u->u.msg);
>  	token = memchr(path, 0, u->u.msg.len);
>  	if (token == NULL) {
> -		rc = -EILSEQ;
> +		rc = xenbus_command_reply(u, XS_ERROR, "EINVAL");
>  		goto out;
>  	}
>  	token++;
>  	if (memchr(token, 0, u->u.msg.len - (token - path)) == NULL) {
> -		rc = -EILSEQ;
> +		rc = xenbus_command_reply(u, XS_ERROR, "EINVAL");
>  		goto out;
>  	}
>  
> @@ -411,23 +434,7 @@ static int xenbus_write_watch(unsigned msg_type, struct xenbus_file_priv *u)
>  	}
>  
>  	/* Success.  Synthesize a reply to say all is OK. */
> -	{
> -		struct {
> -			struct xsd_sockmsg hdr;
> -			char body[3];
> -		} __packed reply = {
> -			{
> -				.type = msg_type,
> -				.len = sizeof(reply.body)
> -			},
> -			"OK"
> -		};
> -
> -		mutex_lock(&u->reply_mutex);
> -		rc = queue_reply(&u->read_buffers, &reply, sizeof(reply));
> -		wake_up(&u->read_waitq);
> -		mutex_unlock(&u->reply_mutex);
> -	}
> +	rc = xenbus_command_reply(u, msg_type, "OK");
>  
>  out:
>  	return rc;

Re: [PATCH 2/3] xen: return xenstore command failures via response instead of rc

[Juergen Gross]

On 22/12/16 16:49, Boris Ostrovsky wrote:
> On 12/22/2016 02:19 AM, Juergen Gross wrote:
>> When the xenbus driver does some special handling for a Xenstore
>> command any error condition related to the command should be returned
>> via an error response instead of letting the related write operation
>> fail. Otherwise the user land handler might take wrong decisions
>> assuming the connection to Xenstore is broken.
> 
> Do we expect the user to always read the reply if no error code was
> returned?

Absolutely, yes. This is how error reporting of Xenstore is
working.


Juergen

Re: [PATCH 2/3] xen: return xenstore command failures via response instead of rc

[Boris Ostrovsky]

On 12/22/2016 10:55 AM, Juergen Gross wrote:
> On 22/12/16 16:49, Boris Ostrovsky wrote:
>> On 12/22/2016 02:19 AM, Juergen Gross wrote:
>>> When the xenbus driver does some special handling for a Xenstore
>>> command any error condition related to the command should be returned
>>> via an error response instead of letting the related write operation
>>> fail. Otherwise the user land handler might take wrong decisions
>>> assuming the connection to Xenstore is broken.
>> Do we expect the user to always read the reply if no error code was
>> returned?
> Absolutely, yes. This is how error reporting of Xenstore is
> working.

Oh, right --- I was thinking about the string that you are passing back
but not the message type (XS_ERROR).

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

[PATCH 3/3] xen: remove stale xs_input_avail() from header

[Juergen Gross]

In drivers/xen/xenbus/xenbus_comms.h there is a stale declaration of
xs_input_avail(). Remove it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xenbus/xenbus_comms.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/xen/xenbus/xenbus_comms.h b/drivers/xen/xenbus/xenbus_comms.h
index e74f9c1..867a2e4 100644
--- a/drivers/xen/xenbus/xenbus_comms.h
+++ b/drivers/xen/xenbus/xenbus_comms.h
@@ -42,7 +42,6 @@ int xb_write(const void *data, unsigned len);
 int xb_read(void *data, unsigned len);
 int xb_data_to_read(void);
 int xb_wait_for_data_to_read(void);
-int xs_input_avail(void);
 extern struct xenstore_domain_interface *xen_store_interface;
 extern int xen_store_evtchn;
 extern enum xenstore_init xen_store_domain_type;
-- 
2.10.2

Re: [PATCH 3/3] xen: remove stale xs_input_avail() from header

[Boris Ostrovsky]

On 12/22/2016 02:19 AM, Juergen Gross wrote:
> In drivers/xen/xenbus/xenbus_comms.h there is a stale declaration of
> xs_input_avail(). Remove it.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
>


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>