From 18e207cbc52110f22b22ab625e395dae24e67463 Mon Sep 17 00:00:00 2001
From: Rafal Wojtczuk <rafal@invisiblethingslab.com>
Date: Wed, 4 May 2011 17:10:01 +0200
Subject: [PATCH] qmemman: prefix variables read from xenstore with
 "untrusted_"

Additionally move all already existing checks to an already
existing is_meminfo_suspicious procedure.
---
 dom0/qmemman/qmemman.py        |  4 +--
 dom0/qmemman/qmemman_algo.py   | 65 ++++++++++++++++++++--------------
 dom0/qmemman/qmemman_server.py |  6 ++--
 3 files changed, 44 insertions(+), 31 deletions(-)

diff --git a/dom0/qmemman/qmemman.py b/dom0/qmemman/qmemman.py
index bf57986b..7c073636 100755
--- a/dom0/qmemman/qmemman.py
+++ b/dom0/qmemman/qmemman.py
@@ -96,8 +96,8 @@ class SystemState:
             time.sleep(self.BALOON_DELAY)
             niter = niter + 1
             
-    def refresh_meminfo(self, domid, val):
-        qmemman_algo.refresh_meminfo_for_domain(self.domdict[domid], val)
+    def refresh_meminfo(self, domid, untrusted_meminfo_key):
+        qmemman_algo.refresh_meminfo_for_domain(self.domdict[domid], untrusted_meminfo_key)
         self.do_balance()
 
     def is_balance_req_significant(self, memset_reqs, xenfree):
diff --git a/dom0/qmemman/qmemman_algo.py b/dom0/qmemman/qmemman_algo.py
index 1cda2c81..a7a3efbe 100755
--- a/dom0/qmemman/qmemman_algo.py
+++ b/dom0/qmemman/qmemman_algo.py
@@ -1,45 +1,58 @@
 import string
 
-def parse_meminfo(meminfo):
-    dict = {}
-    l1 = string.split(meminfo,"\n")
-    for i in l1:
-        l2 = string.split(i)
-        if len(l2) >= 2:
-            dict[string.rstrip(l2[0], ":")] = l2[1]
+#untrusted meminfo size is taken from xenstore key, thus its size is limited
+#so splits do not require excessive memory
+def parse_meminfo(untrusted_meminfo):
+    untrusted_dict = {}
+#split meminfo contents into lines
+    untrusted_lines = string.split(untrusted_meminfo,"\n")
+    for untrusted_lines_iterator in untrusted_lines:
+#split a single meminfo line into words
+        untrusted_words = string.split(untrusted_lines_iterator)
+        if len(untrusted_words) >= 2:
+            untrusted_dict[string.rstrip(untrusted_words[0], ":")] = untrusted_words[1]
 
+    return untrusted_dict
+
+def is_meminfo_suspicious(dom, untrusted_meminfo):
+    ret = False
+    
+#check whether the required keys exist and are not negative
     try:
         for i in ('MemTotal', 'MemFree', 'Buffers', 'Cached', 'SwapTotal', 'SwapFree'):
-            val = int(dict[i])*1024
+            val = int(untrusted_meminfo[i])*1024
             if (val < 0):
-                return None
-            dict[i] = val
+                ret = True
+            untrusted_meminfo[i] = val
     except:
-        return None
-
-    if dict['SwapTotal'] < dict['SwapFree']:
-        return None
-    return dict
-
-def is_suspicious(dom):
-    ret = False
-    if dom.meminfo['SwapTotal'] < dom.meminfo['SwapFree']:
         ret = True
-    if dom.meminfo['MemTotal'] < dom.meminfo['MemFree'] + dom.meminfo['Cached'] + dom.meminfo['Buffers']:
+
+    if not ret and untrusted_meminfo['SwapTotal'] < untrusted_meminfo['SwapFree']:
         ret = True
+    if not ret and untrusted_meminfo['MemTotal'] < untrusted_meminfo['MemFree'] + untrusted_meminfo['Cached'] + untrusted_meminfo['Buffers']:
+        ret = True
+#we could also impose some limits on all the above values
+#but it has little purpose - all the domain can gain by passing e.g. 
+#very large SwapTotal is that it will be assigned all free Xen memory
+#it can be achieved with legal values, too, and it will not allow to
+#starve existing domains, by design
     if ret:
-        print 'suspicious meminfo for domain', dom.id, 'mem actual', dom.memory_actual, dom.meminfo
+        print 'suspicious meminfo for domain', dom.id, 'mem actual', dom.memory_actual, untrusted_meminfo
     return ret
 
-def refresh_meminfo_for_domain(dom, xenstore_key):
-    meminfo = parse_meminfo(xenstore_key)
-    dom.meminfo = meminfo
-    if meminfo is None:
+def refresh_meminfo_for_domain(dom, untrusted_xenstore_key):
+    untrusted_meminfo = parse_meminfo(untrusted_xenstore_key)
+    if untrusted_meminfo is None:
+        dom.meminfo = None
         return
-    if is_suspicious(dom):
+#sanitize start
+    if is_meminfo_suspicious(dom, untrusted_meminfo):
+#sanitize end
         dom.meminfo = None
         dom.mem_used = None
     else:
+#sanitized, can assign
+        dom.meminfo = untrusted_meminfo
         dom.mem_used =  dom.meminfo['MemTotal'] - dom.meminfo['MemFree'] - dom.meminfo['Cached'] - dom.meminfo['Buffers'] + dom.meminfo['SwapTotal'] - dom.meminfo['SwapFree']
                         
 def prefmem(dom):
diff --git a/dom0/qmemman/qmemman_server.py b/dom0/qmemman/qmemman_server.py
index 52e35664..929aa93b 100755
--- a/dom0/qmemman/qmemman_server.py
+++ b/dom0/qmemman/qmemman_server.py
@@ -49,11 +49,11 @@ class XS_Watcher:
         global_lock.release()
 
     def request(self, domain_id):
-        ret = self.handle.read('', get_req_node(domain_id))
-        if ret == None or ret == '':
+        untrusted_meminfo_key = self.handle.read('', get_req_node(domain_id))
+        if untrusted_meminfo_key == None or untrusted_meminfo_key == '':
             return
         global_lock.acquire()
-        system_state.refresh_meminfo(domain_id, ret)
+        system_state.refresh_meminfo(domain_id, untrusted_meminfo_key)
         global_lock.release()
 
     def watch_loop(self):