linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] xen: fix some minor bugs and cleanup of xenbus
@ 2016-12-22  7:19 Juergen Gross
  2016-12-22  7:19 ` [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids Juergen Gross
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Juergen Gross @ 2016-12-22  7:19 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: boris.ostrovsky, 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids
  2016-12-22  7:19 [PATCH 0/3] xen: fix some minor bugs and cleanup of xenbus Juergen Gross
@ 2016-12-22  7:19 ` Juergen Gross
  2016-12-22 15:38   ` Boris Ostrovsky
  2016-12-22  7:19 ` [PATCH 2/3] xen: return xenstore command failures via response instead of rc Juergen Gross
  2016-12-22  7:19 ` [PATCH 3/3] xen: remove stale xs_input_avail() from header Juergen Gross
  2 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2016-12-22  7:19 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: boris.ostrovsky, 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] xen: return xenstore command failures via response instead of rc
  2016-12-22  7:19 [PATCH 0/3] xen: fix some minor bugs and cleanup of xenbus Juergen Gross
  2016-12-22  7:19 ` [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids Juergen Gross
@ 2016-12-22  7:19 ` Juergen Gross
  2016-12-22 15:49   ` Boris Ostrovsky
  2016-12-22  7:19 ` [PATCH 3/3] xen: remove stale xs_input_avail() from header Juergen Gross
  2 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2016-12-22  7:19 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: boris.ostrovsky, 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] xen: remove stale xs_input_avail() from header
  2016-12-22  7:19 [PATCH 0/3] xen: fix some minor bugs and cleanup of xenbus Juergen Gross
  2016-12-22  7:19 ` [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids Juergen Gross
  2016-12-22  7:19 ` [PATCH 2/3] xen: return xenstore command failures via response instead of rc Juergen Gross
@ 2016-12-22  7:19 ` Juergen Gross
  2016-12-22 15:50   ` Boris Ostrovsky
  2 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2016-12-22  7:19 UTC (permalink / raw)
  To: linux-kernel, xen-devel; +Cc: boris.ostrovsky, 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids
  2016-12-22  7:19 ` [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids Juergen Gross
@ 2016-12-22 15:38   ` Boris Ostrovsky
  2016-12-22 15:51     ` Juergen Gross
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2016-12-22 15:38 UTC (permalink / raw)
  To: Juergen Gross, linux-kernel, xen-devel

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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] xen: return xenstore command failures via response instead of rc
  2016-12-22  7:19 ` [PATCH 2/3] xen: return xenstore command failures via response instead of rc Juergen Gross
@ 2016-12-22 15:49   ` Boris Ostrovsky
  2016-12-22 15:55     ` Juergen Gross
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2016-12-22 15:49 UTC (permalink / raw)
  To: Juergen Gross, linux-kernel, xen-devel

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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] xen: remove stale xs_input_avail() from header
  2016-12-22  7:19 ` [PATCH 3/3] xen: remove stale xs_input_avail() from header Juergen Gross
@ 2016-12-22 15:50   ` Boris Ostrovsky
  0 siblings, 0 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2016-12-22 15:50 UTC (permalink / raw)
  To: Juergen Gross, linux-kernel, xen-devel

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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids
  2016-12-22 15:38   ` Boris Ostrovsky
@ 2016-12-22 15:51     ` Juergen Gross
  0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2016-12-22 15:51 UTC (permalink / raw)
  To: Boris Ostrovsky, linux-kernel, xen-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] xen: return xenstore command failures via response instead of rc
  2016-12-22 15:49   ` Boris Ostrovsky
@ 2016-12-22 15:55     ` Juergen Gross
  2016-12-22 16:00       ` Boris Ostrovsky
  0 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2016-12-22 15:55 UTC (permalink / raw)
  To: Boris Ostrovsky, linux-kernel, xen-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] xen: return xenstore command failures via response instead of rc
  2016-12-22 15:55     ` Juergen Gross
@ 2016-12-22 16:00       ` Boris Ostrovsky
  0 siblings, 0 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2016-12-22 16:00 UTC (permalink / raw)
  To: Juergen Gross, linux-kernel, xen-devel

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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-12-22 16:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-22  7:19 [PATCH 0/3] xen: fix some minor bugs and cleanup of xenbus Juergen Gross
2016-12-22  7:19 ` [PATCH 1/3] xen: xenbus driver must not accept invalid transaction ids Juergen Gross
2016-12-22 15:38   ` Boris Ostrovsky
2016-12-22 15:51     ` Juergen Gross
2016-12-22  7:19 ` [PATCH 2/3] xen: return xenstore command failures via response instead of rc Juergen Gross
2016-12-22 15:49   ` Boris Ostrovsky
2016-12-22 15:55     ` Juergen Gross
2016-12-22 16:00       ` Boris Ostrovsky
2016-12-22  7:19 ` [PATCH 3/3] xen: remove stale xs_input_avail() from header Juergen Gross
2016-12-22 15:50   ` Boris Ostrovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).