Re: oxenstored memory leak? seems related with XSA-38

["Liuqiming (John)" <john.liuqiming@huawei.com>]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

Here is my patch, please let me know if I did anything wrong. (this is my first patch for xen :)

Thanks.

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Friday, July 19, 2013 7:57 PM
> To: David Scott
> Cc: Liuqiming (John); andrew.cooper3@citrix.com; Yanqiangjun;
> xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] oxenstored memory leak? seems related with
> XSA-38
> 
> On Tue, 2013-07-16 at 10:56 +0100, David Scott wrote:
> > On 16/07/13 10:46, Ian Campbell wrote:
> > > On Mon, 2013-07-15 at 13:13 +0100, David Scott wrote:
> > >> Hi,
> > >>
> > >> On 05/07/13 10:07, Liuqiming (John) wrote:
> > >>>
> > >>> Here is my patch that try to fix this issue.
> > >>>
> > >>> The whole idea is: add check logic when read from IO ring, and if error
> happens mark the reading connection as "bad",
> > >>> Unless vm reboot, oxenstored will not handle message from this
> connection any more.
> > >>
> > >> I think detecting a bad client and avoiding wasting CPU time on it is a
> > >> good idea. Is this patch working well for you in your testing?
> > >>
> > >> In future I wonder whether we should add some form of request
> > >> rate-limiting in addition to the per-domain quotas, to prevent one
> > >> domain from taking more than it's fair share of xenstored time. I
> > >> imagine that a non-malicious domain can still keep xenstored busy with
> > >> 'legitimate' traffic (although hopefully it still provides services to
> > >> other guests, albeit more slowly)
> > >
> > > Is this an Ack for this patch, or (leaving aside future work) would you
> > > like to see changes before it gets applied?
> >
> > I'm happy with this current patch for now, so
> >
> > Acked-by: David Scott <dave.scott@eu.citrix.com>
> 
> Thanks.
> 
> John -- Please could you resubmit with a proper changelog and a
> Signed-off-by indicating that you agree to the Developer's Certificate
> of Origin (described in
> http://wiki.xen.org/wiki/Submitting_Xen_Patches). Please include Dave's
> ack as well (after your S-o-b).
> 
> Thanks,
> Ian.
> 


[-- Attachment #2: oxenstored.patch --]
[-- Type: application/octet-stream, Size: 4120 bytes --]

oxenstored: Protect oxenstored from malicious domains.

add check logic when read from IO ring, and if error happens, 
then mark the reading connection as "bad", Unless vm reboot, 
oxenstored will not handle message from this connection any more.

xs_ring_stubs.c: add a more strict check on ring reading 
connection.ml, domain.ml: add getter and setter for bad flag 
process.ml: if exception raised when reading from domain's ring, 
            mark this domain as "bad" 
xenstored.ml: if a domain is marked as "bad", do not handle it.

Signed-off-by: John Liu <john.liuqiming@huawei.com>

diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
index fdd9983..c2ca9b4 100644
--- a/tools/ocaml/libs/xb/xs_ring_stubs.c
+++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
@@ -45,6 +45,10 @@
 	cons = *(volatile uint32*)&intf->req_cons;
 	prod = *(volatile uint32*)&intf->req_prod;
 	xen_mb();
+	
+	if ((prod - cons) > XENSTORE_RING_SIZE)
+	    return -1;
+	
 	if (prod == cons)
 		return 0;
 	cons = MASK_XENSTORE_IDX(cons);
@@ -94,7 +98,7 @@
 	res = xs_ring_read(GET_C_STRUCT(interface),
 	                   String_val(buffer), Int_val(len));
 	if (res == -1)
-		caml_failwith("huh");
+		caml_failwith("bad connection");
 	result = Val_int(res);
 	CAMLreturn(result);
 }
diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index 32e2f2e..a53983d 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -38,6 +38,11 @@
 	mutable perm: Perms.Connection.t;
 }
 
+let mark_as_bad con = 
+	match con.dom with
+	|None -> ()
+	| Some domain -> Domain.mark_as_bad domain
+
 let get_path con =
 Printf.sprintf "/local/domain/%i/" (match con.dom with None -> 0 | Some d -> Domain.get_id d)
 
diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml
index 85ab282..444069d 100644
--- a/tools/ocaml/xenstored/domain.ml
+++ b/tools/ocaml/xenstored/domain.ml
@@ -27,6 +27,7 @@
 	interface: Xenmmap.mmap_interface;
 	eventchn: Event.t;
 	mutable port: Xeneventchn.t option;
+	mutable bad_client: bool;
 }
 
 let get_path dom = "/local/domain/" ^ (sprintf "%u" dom.id)
@@ -34,6 +35,9 @@
 let get_interface d = d.interface
 let get_mfn d = d.mfn
 let get_remote_port d = d.remote_port
+
+let is_bad_domain domain = domain.bad_client
+let mark_as_bad domain = domain.bad_client <- true
 
 let string_of_port = function
 | None -> "None"
@@ -68,7 +72,8 @@
 	remote_port = remote_port;
 	interface = interface;
 	eventchn = eventchn;
-	port = None
+	port = None;
+	bad_client = false
 }
 
 let is_dom0 d = d.id = 0
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index a4ff741..2267ddc 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -374,7 +374,17 @@
 	Logging.xb_answer ~ty ~tid ~con:(Connection.get_domstr con) data
 
 let do_input store cons doms con =
-	if Connection.do_input con then (
+	let newpacket = 
+		try
+			Connection.do_input con
+		with Failure exp ->
+			error "caught exception %s" exp;
+			error "got a bad client %s" (sprintf "%-8s" (Connection.get_domstr con));
+			Connection.mark_as_bad con;
+			false
+	in
+	
+	if newpacket then (
 		let packet = Connection.pop_in con in
 		let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
 		(* As we don't log IO, do not call an unnecessary sanitize_data 
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index 4045aed..438ecb9 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -50,9 +50,10 @@
 
 let process_domains store cons domains =
 	let do_io_domain domain =
-		let con = Connections.find_domain cons (Domain.get_id domain) in
-		Process.do_input store cons domains con;
-		Process.do_output store cons domains con in
+		if not (Domain.is_bad_domain domain) then
+			let con = Connections.find_domain cons (Domain.get_id domain) in
+				Process.do_input store cons domains con;
+				Process.do_output store cons domains con in
 	Domains.iter domains do_io_domain
 
 let sigusr1_handler store =

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel