tools/ocaml: oxenstored: Be more paranoid about ring reading

oxenstored makes use of the OCaml Xenbus bindings, in which the
function xs_ring_read in tools/ocaml/libs/xb/xs_ring_stubs.c is used
to read from the shared memory Xenstore ring.

This function does not correctly handle all possible (prod, cons)
states when MASK_XENSTORE_IDX(prod) > MASK_XENSTORE_IDX(cons).

The root cause is the use of the unmasked values of prod and cons to
calculate to_read.  If prod is set to an out-of-range value, the ring
peer can cause to_read to be too large or even negative.  This allows
the ring peer to force oxenstored to read and write out of range for
the buffers leading to a crash or possibly to privilege escalation.

Correct this by masking the values of cons and prod at the start, so
we only deal with masked values.  This makes the logic simpler, as
semantically inappropriate values of the upper bits of the ring
pointers are simply ignored.

The same vulnerability does not exist in the ring writer because the
only use made of the unmasked value is the check which prevents the
prod pointer overtaking the cons pointer.  A ring peer which defeats
this check will suffer only lost data.


However, additionally, precautions need to be taken to ensure that
req_cons and req_prod are only read once in each function.  Without
the use of volatile or some asm construct, the compiler can "prove"
that req_cons and req_prod do not change unexpectedly and is permitted
to "amplify" the read of (say) req_cons into two reads at different
times, giving two different values for use as cons, and then use the
two sources of cons interchangeably.  (The use of xen_mb() does not
forbid this.)

Therefore do the reads of req_cons and req_prod through a volatile
pointer in both xs_ring_read and xs_ring_write.

This is currently believed to be a theoretical vulnerability as we are
not aware of any compilers which amplify reads in this way.


This is a security issue, part of XSA-38 / CVE-2013-0215.


Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Tested-by: Matthew Daley <mattjd@gmail.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

1 files changed, 10 insertions, 8 deletions

diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
index 00414c5353..4888ac5631 100644
--- a/tools/ocaml/libs/xb/xs_ring_stubs.c
+++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
@@ -39,21 +39,23 @@ static int xs_ring_read(struct mmap_interface *interface,
                              char *buffer, int len)
 {
 	struct xenstore_domain_interface *intf = interface->addr;
-	XENSTORE_RING_IDX cons, prod;
+	XENSTORE_RING_IDX cons, prod; /* offsets only */
 	int to_read;
 
-	cons = intf->req_cons;
-	prod = intf->req_prod;
+	cons = *(volatile uint32*)&intf->req_cons;
+	prod = *(volatile uint32*)&intf->req_prod;
 	xen_mb();
+	cons = MASK_XENSTORE_IDX(cons);
+	prod = MASK_XENSTORE_IDX(prod);
 	if (prod == cons)
 		return 0;
-	if (MASK_XENSTORE_IDX(prod) > MASK_XENSTORE_IDX(cons)) 
+	if (prod > cons)
 		to_read = prod - cons;
 	else
-		to_read = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(cons);
+		to_read = XENSTORE_RING_SIZE - cons;
 	if (to_read < len)
 		len = to_read;
-	memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), len);
+	memcpy(buffer, intf->req + cons, len);
 	xen_mb();
 	intf->req_cons += len;
 	return len;
@@ -66,8 +68,8 @@ static int xs_ring_write(struct mmap_interface *interface,
 	XENSTORE_RING_IDX cons, prod;
 	int can_write;
 
-	cons = intf->rsp_cons;
-	prod = intf->rsp_prod;
+	cons = *(volatile uint32*)&intf->rsp_cons;
+	prod = *(volatile uint32*)&intf->rsp_prod;
 	xen_mb();
 	if ( (prod - cons) >= XENSTORE_RING_SIZE )
 		return 0;