From fe72fe16815e15a8c2fff778961a784afd1ab09d Mon Sep 17 00:00:00 2001
From: Pawel Marczewski <pawel@invisiblethingslab.com>
Date: Wed, 22 Jan 2020 11:10:50 +0100
Subject: [PATCH] Fix overlapping block device names

This was caused by a change in Jinja template engine that breaks
assignments like {% set i = i + 1 %} in a loop.

Jinja 2.10 introduces a "namespace" object for this use case, but
unfortunately dom0-fc25 uses 2.8.1.

See:

https://github.com/pallets/jinja/issues/641
https://github.com/pallets/jinja/pull/684

Fixes QubesOS/qubes-issues#5551.
---
 qubes/tests/vm/qubesvm.py           | 144 ++++++++++++++++++++++++++++
 templates/libvirt/devices/block.xml |   4 +-
 templates/libvirt/xen.xml           |  18 +++-
 3 files changed, 159 insertions(+), 7 deletions(-)

diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py
index d64c920c..ff1da756 100644
--- a/qubes/tests/vm/qubesvm.py
+++ b/qubes/tests/vm/qubesvm.py
@@ -1469,6 +1469,150 @@ class TC_90_QubesVM(QubesVMTestsMixin, qubes.tests.QubesTestCase):
                     extra_ip='<ip address="{}::a89:1" family=\'ipv6\'/>'.format(
                         qubes.config.qubes_ipv6_prefix.replace(':0000', '')))))
 
+    def test_615_libvirt_xml_block_devices(self):
+        expected = '''<domain type="xen">
+        <name>test-inst-test</name>
+        <uuid>7db78950-c467-4863-94d1-af59806384ea</uuid>
+        <memory unit="MiB">400</memory>
+        <currentMemory unit="MiB">400</currentMemory>
+        <vcpu placement="static">2</vcpu>
+        <cpu mode='host-passthrough'>
+            <!-- disable nested HVM -->
+            <feature name='vmx' policy='disable'/>
+            <feature name='svm' policy='disable'/>
+            <!-- disable SMAP inside VM, because of Linux bug -->
+            <feature name='smap' policy='disable'/>
+        </cpu>
+        <os>
+            <type arch="x86_64" machine="xenfv">hvm</type>
+                <!--
+                     For the libxl backend libvirt switches between OVMF (UEFI)
+                     and SeaBIOS based on the loader type. This has nothing to
+                     do with the hvmloader binary.
+                -->
+            <loader type="rom">hvmloader</loader>
+            <boot dev="cdrom" />
+            <boot dev="hd" />
+        </os>
+        <features>
+            <pae/>
+            <acpi/>
+            <apic/>
+            <viridian/>
+        </features>
+        <clock offset="variable" adjustment="0" basis="localtime" />
+        <on_poweroff>destroy</on_poweroff>
+        <on_reboot>destroy</on_reboot>
+        <on_crash>destroy</on_crash>
+        <devices>
+            <disk type="block" device="disk">
+                <driver name="phy" />
+                <source dev="/dev/loop0" />
+                <target dev="xvda" />
+                <backenddomain name="dom0" />
+                <script path="/tmp/script" />
+            </disk>
+            <disk type="block" device="disk">
+                <driver name="phy" />
+                <source dev="/dev/loop1" />
+                <target dev="xvde" />
+                <backenddomain name="dom0" />
+            </disk>
+            <disk type="block" device="disk">
+                <driver name="phy" />
+                <source dev="/dev/loop2" />
+                <target dev="xvdf" />
+                <backenddomain name="dom0" />
+            </disk>
+
+            <disk type="block" device="disk">
+                <driver name="phy" />
+                <source dev="/dev/sdb" />
+                <target dev="xvdl" />
+            </disk>
+            <disk type="block" device="cdrom">
+                <driver name="phy" />
+                <source dev="/dev/sda" />
+                <!-- prefer xvdd for CDROM -->
+                <target dev="xvdd" />
+            </disk>
+            <disk type="block" device="disk">
+                <driver name="phy" />
+                <source dev="/dev/loop0" />
+                <target dev="xvdi" />
+                <backenddomain name="backend0" />
+            </disk>
+            <disk type="block" device="disk">
+                <driver name="phy" />
+                <source dev="/dev/loop0" />
+                <target dev="xvdj" />
+                <backenddomain name="backend1" />
+            </disk>
+            <!-- server_ip is the address of stubdomain. It hosts it's own DNS server. -->
+            <emulator type="stubdom-linux" />
+            <input type="tablet" bus="usb"/>
+            <video>
+                <model type="vga"/>
+            </video>
+            <graphics type="qubes"/>
+            <console type="pty">
+                <target type="xen" port="0"/>
+            </console>
+        </devices>
+        </domain>
+        '''
+        my_uuid = '7db78950-c467-4863-94d1-af59806384ea'
+        vm = self.get_vm(uuid=my_uuid)
+        vm.netvm = None
+        vm.virt_mode = 'hvm'
+        vm.volumes['root'] = unittest.mock.Mock(**{
+            'block_device.return_value.name': 'root',
+            'block_device.return_value.path': '/dev/loop0',
+            'block_device.return_value.devtype': 'disk',
+            'block_device.return_value.domain': 'dom0',
+            'block_device.return_value.script': '/tmp/script',
+        })
+        vm.volumes['other'] = unittest.mock.Mock(**{
+            'block_device.return_value.name': 'other',
+            'block_device.return_value.path': '/dev/loop1',
+            'block_device.return_value.devtype': 'disk',
+            'block_device.return_value.domain': 'dom0',
+            'block_device.return_value.script': None,
+        })
+        vm.volumes['other2'] = unittest.mock.Mock(**{
+            'block_device.return_value.name': 'other',
+            'block_device.return_value.path': '/dev/loop2',
+            'block_device.return_value.devtype': 'disk',
+            'block_device.return_value.domain': 'dom0',
+            'block_device.return_value.script': None,
+        })
+        assignments = [
+            unittest.mock.Mock(**{
+                'options': {'frontend-dev': 'xvdl'},
+                'device.device_node': '/dev/sdb',
+                'device.backend_domain.name': 'dom0',
+            }),
+            unittest.mock.Mock(**{
+                'options': {'devtype': 'cdrom'},
+                'device.device_node': '/dev/sda',
+                'device.backend_domain.name': 'dom0',
+            }),
+            unittest.mock.Mock(**{
+                'options': {'read-only': True},
+                'device.device_node': '/dev/loop0',
+                'device.backend_domain.name': 'backend0',
+            }),
+            unittest.mock.Mock(**{
+                'options': {},
+                'device.device_node': '/dev/loop0',
+                'device.backend_domain.name': 'backend1',
+            }),
+        ]
+        vm.devices['block'].assignments = lambda persistent: assignments
+        libvirt_xml = vm.create_config_file()
+        self.assertXMLEqual(lxml.etree.XML(libvirt_xml),
+            lxml.etree.XML(expected))
+
     @unittest.mock.patch('qubes.utils.get_timezone')
     @unittest.mock.patch('qubes.utils.urandom')
     @unittest.mock.patch('qubes.vm.qubesvm.QubesVM.untrusted_qdb')
diff --git a/templates/libvirt/devices/block.xml b/templates/libvirt/devices/block.xml
index add6cb3c..e085c635 100644
--- a/templates/libvirt/devices/block.xml
+++ b/templates/libvirt/devices/block.xml
@@ -7,8 +7,8 @@
         <!-- prefer xvdd for CDROM -->
         <target dev="xvdd" />
     {%- else %}
-        <target dev="xvd{{dd[i]}}" />
-        {% set i = i + 1 %}
+        <target dev="xvd{{dd[counter.i]}}" />
+        {% if counter.update({'i': counter.i + 1}) %}{% endif %}
     {%- endif %}
 
     {%- if options.get('read-only', 'no') == 'yes' %}
diff --git a/templates/libvirt/xen.xml b/templates/libvirt/xen.xml
index 3622f537..24200b2a 100644
--- a/templates/libvirt/xen.xml
+++ b/templates/libvirt/xen.xml
@@ -96,7 +96,15 @@
 
     <devices>
         {% block devices %}
-            {% set i = 0 %}
+            {#
+                HACK: The letter counter is implemented in this way because
+                Jinja does not allow you to increment variables in a loop
+                anymore. As of Jinja 2.10, we will be able to replace this
+                with:
+                {% set counter = namespace(i=0) %}
+                {% set counter.i = counter.i + 1 %}
+            #}
+            {% set counter = {'i': 0} %}
             {# TODO Allow more volumes out of the box #}
             {% set dd = ['e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p',
                 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y']
@@ -114,8 +122,8 @@
                     {% elif device.name == 'kernel' %}
                         <target dev="xvdd" />
                     {% else %}
-                        <target dev="xvd{{dd[i]}}" />
-                        {% set i = i + 1 %}
+                        <target dev="xvd{{dd[counter.i]}}" />
+                        {% if counter.update({'i': counter.i + 1}) %}{% endif %}
                     {% endif %}
 
                     {% if not device.rw %}
@@ -133,7 +141,7 @@
             {% endfor %}
 
             {# start external devices from xvdi #}
-            {% set i = 4 %}
+            {% set counter = {'i': 4} %}
             {% for assignment in vm.devices.block.assignments(True) %}
                 {% set device = assignment.device %}
                 {% set options = assignment.options %}
@@ -185,5 +193,5 @@
         {% endblock %}
     </devices>
 </domain>
- 
+
 <!-- vim: set ft=jinja ts=4 sts=4 sw=4 et tw=80 : -->