Re: [PATCH] XEN: xenbus: integer overflow in process_msg()

Re: [PATCH] XEN: xenbus: integer overflow in process_msg()

[Ian Campbell]

On Tue, 2012-01-03 at 19:42 +0000, Haogang Chen wrote:
> There is a potential integer overflow in process_msg() that could result
> in cross-domain attack.
> 
> 	body = kmalloc(msg->hdr.len + 1, GFP_NOIO | __GFP_HIGH);
> 
> When a malicious guest passes 0xffffffff in msg->hdr.len, the subsequent
> call to xb_read() would write to a zero-length buffer.

The other end of this connection is always the xenstore backend daemon
so there is no guest (malicious or otherwise) which can do this. The
xenstore daemon is a trusted component in the system.

However this seem like a reasonable robustness improvement so we should
have it.

> This causes
> kernel oops in the receiving guest and hangs its xenbus kernel thread.
> The patch returns -EINVAL in that case.
> 
> Signed-off-by: Haogang Chen <haogangchen@gmail.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  drivers/xen/xenbus/xenbus_xs.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
> index ede860f..e32aefb 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -801,6 +801,12 @@ static int process_msg(void)
>  		goto out;
>  	}
>  
> +	if (msg->hdr.len == UINT_MAX) {
> +		kfree(msg);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	body = kmalloc(msg->hdr.len + 1, GFP_NOIO | __GFP_HIGH);
>  	if (body == NULL) {
>  		kfree(msg);

Re: [PATCH] XEN: xenbus: integer overflow in process_msg()

[Ian Campbell]

On Wed, 2012-01-04 at 09:24 +0000, Ian Campbell wrote:
> On Tue, 2012-01-03 at 19:42 +0000, Haogang Chen wrote:
> > There is a potential integer overflow in process_msg() that could result
> > in cross-domain attack.
> > 
> > 	body = kmalloc(msg->hdr.len + 1, GFP_NOIO | __GFP_HIGH);
> > 
> > When a malicious guest passes 0xffffffff in msg->hdr.len, the subsequent
> > call to xb_read() would write to a zero-length buffer.
> 
> The other end of this connection is always the xenstore backend daemon
> so there is no guest (malicious or otherwise) which can do this. The
> xenstore daemon is a trusted component in the system.
> 
> However this seem like a reasonable robustness improvement so we should
> have it.

Actually, rereading docs/misc/xenstore.txt I see:
        The payload length (len field of the header) is limited to 4096
        (XENSTORE_PAYLOAD_MAX) in both directions.  If a client exceeds the
        limit, its xenstored connection will be immediately killed by
        xenstored, which is usually catastrophic from the client's point of
        view.  Clients (particularly domains, which cannot just reconnect)
        should avoid this.

So probably we actually want (untested, but seems obvious enough):

8<---------------------------------------------------------

From a010c48fe67ef15495884c265ec6af8a688795f8 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Wed, 4 Jan 2012 09:29:41 +0000
Subject: [PATCH] xen/xenbus: Reject replies with payload > XENSTORE_PAYLOAD_MAX.

This also avoids a potential integer overflow pointed out by Haogang Chen.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Haogang Chen <haogangchen@gmail.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/xen/xenbus/xenbus_xs.c     |    6 ++++++
 include/xen/interface/io/xs_wire.h |    3 +++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index b3b8f2f..6f0121e 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -810,6 +810,12 @@ static int process_msg(void)
 		goto out;
 	}
 
+	if (msg->hdr.len > XENSTORE_PAYLOAD_MAX) {
+		kfree(msg);
+		err = -EINVAL;
+		goto out;
+	}
+
 	body = kmalloc(msg->hdr.len + 1, GFP_NOIO | __GFP_HIGH);
 	if (body == NULL) {
 		kfree(msg);
diff --git a/include/xen/interface/io/xs_wire.h b/include/xen/interface/io/xs_wire.h
index f0b6890..3c1877c 100644
--- a/include/xen/interface/io/xs_wire.h
+++ b/include/xen/interface/io/xs_wire.h
@@ -88,4 +88,7 @@ struct xenstore_domain_interface {
     XENSTORE_RING_IDX rsp_cons, rsp_prod;
 };
 
+/* Violating this is very bad.  See docs/misc/xenstore.txt. */
+#define XENSTORE_PAYLOAD_MAX 4096
+
 #endif /* _XS_WIRE_H */
-- 
1.7.2.5

[PATCH 0/2] xen: Miscelaneous xenbus cleanups

[Ian Campbell]

Just a couple of things I noticed while reviewing Haogang's patch.
Applies on top of my suggested replacement for that patch (in
<1325669689.25206.181.camel@zakaz.uk.xensource.com>).

Not extensively tested but I did run it in dom0 and start both and HVM
and PV guest.

Ian.

[PATCH 1/2] xenbus: maximum buffer size is XENSTORE_PAYLOAD_MAX

[Ian Campbell]

Use this now that it is defined even though it happens to be == PAGE_SIZE.

The code which takes requests from userspace already validates against the size
of this buffer so no further checks are required to ensure that userspace
requests comply with the protocol in this respect.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Haogang Chen <haogangchen@gmail.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/xen/xenbus/xenbus_dev_frontend.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index fb30cff..1fe4324 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -104,7 +104,7 @@ struct xenbus_file_priv {
 	unsigned int len;
 	union {
 		struct xsd_sockmsg msg;
-		char buffer[PAGE_SIZE];
+		char buffer[XENSTORE_PAYLOAD_MAX];
 	} u;
 
 	/* Response queue. */
-- 
1.7.2.5

[PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Haogang Chen <haogangchen@gmail.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/xen/xenbus/xenbus_xs.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index 6f0121e..226d1ac 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -532,21 +532,18 @@ int xenbus_printf(struct xenbus_transaction t,
 {
 	va_list ap;
 	int ret;
-#define PRINTF_BUFFER_SIZE 4096
-	char *printf_buffer;
-
-	printf_buffer = kmalloc(PRINTF_BUFFER_SIZE, GFP_NOIO | __GFP_HIGH);
-	if (printf_buffer == NULL)
-		return -ENOMEM;
+	char *buf;
 
 	va_start(ap, fmt);
-	ret = vsnprintf(printf_buffer, PRINTF_BUFFER_SIZE, fmt, ap);
+	buf = kvasprintf(GFP_NOIO | __GFP_HIGH, fmt, ap);
 	va_end(ap);
 
-	BUG_ON(ret > PRINTF_BUFFER_SIZE-1);
-	ret = xenbus_write(t, dir, node, printf_buffer);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = xenbus_write(t, dir, node, buf);
 
-	kfree(printf_buffer);
+	kfree(buf);
 
 	return ret;
 }
-- 
1.7.2.5

Re: [Xen-devel] [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer

[Konrad Rzeszutek Wilk]

On Wed, Jan 04, 2012 at 11:58:56AM +0000, Jan Beulich wrote:
> >>> On 04.01.12 at 12:39, Ian Campbell <ian.campbell@citrix.com> wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>

OK, took:

Ian Campbell (3):
      xen/xenbus: Reject replies with payload > XENSTORE_PAYLOAD_MAX.
      xenbus: maximum buffer size is XENSTORE_PAYLOAD_MAX
      xen/xenbus: don't reimplement kvasprintf via a fixed size buffer

And put the ' xen/xenbus: Reject replies with payload > XENSTORE_PAYLOAD_MAX'
on the stable@kernel.org as well.

Thanks!