| From 9e7860cee18241633eddb36a4c34c7b61d8cecbc Mon Sep 17 00:00:00 2001 |
| From: Ian Campbell <Ian.Campbell@citrix.com> |
| Date: Wed, 4 Jan 2012 09:34:49 +0000 |
| Subject: xen/xenbus: Reject replies with payload > XENSTORE_PAYLOAD_MAX. |
| |
| From: Ian Campbell <Ian.Campbell@citrix.com> |
| |
| commit 9e7860cee18241633eddb36a4c34c7b61d8cecbc upstream. |
| |
| Haogang Chen found out that: |
| |
| 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. |
| |
| And Ian when read the API docs found that: |
| 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 this patch checks against that instead. |
| |
| 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> |
| Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> |
| |
| --- |
| drivers/xen/xenbus/xenbus_xs.c | 6 ++++++ |
| include/xen/interface/io/xs_wire.h | 3 +++ |
| 2 files changed, 9 insertions(+) |
| |
| --- a/drivers/xen/xenbus/xenbus_xs.c |
| +++ b/drivers/xen/xenbus/xenbus_xs.c |
| @@ -766,6 +766,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); |
| --- a/include/xen/interface/io/xs_wire.h |
| +++ b/include/xen/interface/io/xs_wire.h |
| @@ -84,4 +84,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 */ |