* [PATCH] XEN: xenbus: integer overflow in process_msg()
@ 2012-01-03 19:42 Haogang Chen
2012-01-04 9:24 ` Ian Campbell
0 siblings, 1 reply; 16+ messages in thread
From: Haogang Chen @ 2012-01-03 19:42 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Jeremy Fitzhardinge, Ian Campbell
Cc: xen-devel, virtualization, linux-kernel, Haogang Chen
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. 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>
---
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);
--
1.7.5.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] XEN: xenbus: integer overflow in process_msg()
2012-01-03 19:42 [PATCH] XEN: xenbus: integer overflow in process_msg() Haogang Chen
@ 2012-01-04 9:24 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 9:24 UTC (permalink / raw)
To: Haogang Chen
Cc: Konrad Rzeszutek Wilk, Jeremy Fitzhardinge, xen-devel,
virtualization, linux-kernel
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);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] XEN: xenbus: integer overflow in process_msg()
@ 2012-01-04 9:24 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 9:24 UTC (permalink / raw)
To: Haogang Chen
Cc: Jeremy Fitzhardinge, xen-devel, virtualization, linux-kernel,
Konrad Rzeszutek Wilk
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);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] XEN: xenbus: integer overflow in process_msg()
2012-01-04 9:24 ` Ian Campbell
(?)
@ 2012-01-04 9:34 ` Ian Campbell
-1 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 9:34 UTC (permalink / raw)
To: Haogang Chen
Cc: Jeremy Fitzhardinge, xen-devel, virtualization, linux-kernel,
Konrad Rzeszutek Wilk
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] XEN: xenbus: integer overflow in process_msg()
@ 2012-01-04 9:34 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 9:34 UTC (permalink / raw)
To: Haogang Chen
Cc: Jeremy Fitzhardinge, xen-devel, linux-kernel,
Konrad Rzeszutek Wilk, virtualization
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] XEN: xenbus: integer overflow in process_msg()
@ 2012-01-04 9:34 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 9:34 UTC (permalink / raw)
To: Haogang Chen
Cc: Jeremy Fitzhardinge, xen-devel, linux-kernel,
Konrad Rzeszutek Wilk, virtualization
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 0/2] xen: Miscelaneous xenbus cleanups
2012-01-04 9:34 ` Ian Campbell
@ 2012-01-04 11:39 ` Ian Campbell
-1 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 11:39 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Jeremy Fitzhardinge, Haogang Chen, xen-devel, linux-kernel,
virtualization
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/2] xen: Miscelaneous xenbus cleanups
@ 2012-01-04 11:39 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 11:39 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Jeremy Fitzhardinge, Haogang Chen, virtualization, xen-devel,
linux-kernel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] xenbus: maximum buffer size is XENSTORE_PAYLOAD_MAX
2012-01-04 11:39 ` Ian Campbell
@ 2012-01-04 11:39 ` Ian Campbell
-1 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 11:39 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Ian Campbell, Haogang Chen, Konrad Rzeszutek Wilk,
Jeremy Fitzhardinge, xen-devel, virtualization, linux-kernel
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/2] xenbus: maximum buffer size is XENSTORE_PAYLOAD_MAX
@ 2012-01-04 11:39 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 11:39 UTC (permalink / raw)
Cc: Jeremy Fitzhardinge, xen-devel, Ian Campbell,
Konrad Rzeszutek Wilk, linux-kernel, virtualization,
Haogang Chen
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer
2012-01-04 11:39 ` Ian Campbell
@ 2012-01-04 11:39 ` Ian Campbell
-1 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 11:39 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Ian Campbell, Haogang Chen, Konrad Rzeszutek Wilk,
Jeremy Fitzhardinge, xen-devel, virtualization, linux-kernel
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer
@ 2012-01-04 11:39 ` Ian Campbell
0 siblings, 0 replies; 16+ messages in thread
From: Ian Campbell @ 2012-01-04 11:39 UTC (permalink / raw)
Cc: Jeremy Fitzhardinge, xen-devel, Ian Campbell,
Konrad Rzeszutek Wilk, linux-kernel, virtualization,
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 | 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer
2012-01-04 11:39 ` Ian Campbell
@ 2012-01-04 11:58 ` Jan Beulich
-1 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2012-01-04 11:58 UTC (permalink / raw)
To: Ian Campbell
Cc: Haogang Chen, Jeremy Fitzhardinge, virtualization, xen-devel,
Konrad Rzeszutek Wilk, linux-kernel
>>> 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>
> 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
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer
@ 2012-01-04 11:58 ` Jan Beulich
0 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2012-01-04 11:58 UTC (permalink / raw)
To: Ian Campbell
Cc: Haogang Chen, Jeremy Fitzhardinge, virtualization, xen-devel,
Konrad Rzeszutek Wilk, linux-kernel
>>> 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>
> 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
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer
2012-01-04 11:58 ` Jan Beulich
@ 2012-01-04 15:12 ` Konrad Rzeszutek Wilk
-1 siblings, 0 replies; 16+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-01-04 15:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian Campbell, Haogang Chen, Jeremy Fitzhardinge, virtualization,
xen-devel, linux-kernel
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!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer
@ 2012-01-04 15:12 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 16+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-01-04 15:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Jeremy Fitzhardinge, xen-devel, Ian Campbell, linux-kernel,
virtualization, Haogang Chen
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!
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-01-04 15:15 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-03 19:42 [PATCH] XEN: xenbus: integer overflow in process_msg() Haogang Chen
2012-01-04 9:24 ` Ian Campbell
2012-01-04 9:24 ` Ian Campbell
2012-01-04 9:34 ` Ian Campbell
2012-01-04 9:34 ` Ian Campbell
2012-01-04 9:34 ` Ian Campbell
2012-01-04 11:39 ` [PATCH 0/2] xen: Miscelaneous xenbus cleanups Ian Campbell
2012-01-04 11:39 ` Ian Campbell
2012-01-04 11:39 ` [PATCH 1/2] xenbus: maximum buffer size is XENSTORE_PAYLOAD_MAX Ian Campbell
2012-01-04 11:39 ` Ian Campbell
2012-01-04 11:39 ` [PATCH 2/2] xen/xenbus: don't reimplement kvasprintf via a fixed size buffer Ian Campbell
2012-01-04 11:39 ` Ian Campbell
2012-01-04 11:58 ` [Xen-devel] " Jan Beulich
2012-01-04 11:58 ` Jan Beulich
2012-01-04 15:12 ` Konrad Rzeszutek Wilk
2012-01-04 15:12 ` Konrad Rzeszutek Wilk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.