lkml.org 
[lkml]   [2012]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] XEN: xenbus: integer overflow in process_msg()
From
Date
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




\
 
 \ /
  Last update: 2012-01-04 10:37    [W:0.056 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site