[libvirt] [PATCH 0/7] security: Allow disabling security per VM
Enabling a security driver in qemu.conf is currently all or nothing. The option to disable security on a per VM basis can be a useful debugging tool or work around for frustrated users. Patches 1-3 and 5-6 are prep and cleanup work. Patch 4 fixes an easily triggerable segfault when defining a domain in qemu. Patch 7 is the actual feature. Cole Robinson (7): tests: Add qemuxml2xml tests for <seclabel> handling security: Use virDomainSeclabelDefClear security: Add virSecurityIsSpecifiedDriver qemu: Fix segfault if defining a domain without <seclabel> domain: Handle seclabel model with an enum domain: Always validate seclabel model security: Allow disabling security on a per VM basis cfg.mk | 1 + docs/schemas/domain.rng | 13 ++- src/conf/domain_conf.c | 69 ++++++++++----- src/conf/domain_conf.h | 15 +++- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 4 +- src/security/security_apparmor.c | 31 ++----- src/security/security_driver.c | 25 ++++++ src/security/security_driver.h | 3 + src/security/security_manager.c | 90 +++++++++++++------- src/security/security_selinux.c | 50 +++--------- tests/domainschematest | 2 +- .../qemuxml2xml-balloon-device-auto-out.xml | 25 ++++++ .../qemuxml2xml-channel-virtio-auto-out.xml | 54 ++++++++++++ .../qemuxml2xml-console-compat-auto-out.xml | 31 +++++++ .../qemuxml2xml-console-virtio-out.xml | 29 ++++++ .../qemuxml2xml-disk-scsi-device-auto-out.xml | 31 +++++++ .../qemuxml2xml-seclabel-dynamic-in.xml | 24 +++++ .../qemuxml2xml-seclabel-dynamic-out.xml | 21 +++++ .../qemuxml2xml-seclabel-model-none-in.xml | 21 +++++ .../qemuxml2xml-seclabel-model-none-out.xml | 21 +++++ .../qemuxml2xml-seclabel-static-in.xml | 24 +++++ .../qemuxml2xml-seclabel-static-out.xml | 23 +++++ .../qemuxml2xmlout-balloon-device-auto.xml | 25 ------ .../qemuxml2xmlout-channel-virtio-auto.xml | 54 ------------ .../qemuxml2xmlout-console-compat-auto.xml | 31 ------- .../qemuxml2xmlout-console-virtio.xml | 29 ------ .../qemuxml2xmlout-disk-scsi-device-auto.xml | 31 ------- tests/qemuxml2xmltest.c | 26 ++++-- 29 files changed, 501 insertions(+), 304 deletions(-) create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-balloon-device-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-channel-virtio-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-console-compat-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-console-virtio-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-disk-scsi-device-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-out.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml -- 1.7.3.2
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/schemas/domain.rng | 13 ++++- tests/domainschematest | 2 +- .../qemuxml2xml-balloon-device-auto-out.xml | 25 +++++++++ .../qemuxml2xml-channel-virtio-auto-out.xml | 54 ++++++++++++++++++++ .../qemuxml2xml-console-compat-auto-out.xml | 31 +++++++++++ .../qemuxml2xml-console-virtio-out.xml | 29 +++++++++++ .../qemuxml2xml-disk-scsi-device-auto-out.xml | 31 +++++++++++ .../qemuxml2xml-seclabel-dynamic-in.xml | 24 +++++++++ .../qemuxml2xml-seclabel-dynamic-out.xml | 20 +++++++ .../qemuxml2xml-seclabel-static-in.xml | 24 +++++++++ .../qemuxml2xml-seclabel-static-out.xml | 23 ++++++++ .../qemuxml2xmlout-balloon-device-auto.xml | 25 --------- .../qemuxml2xmlout-channel-virtio-auto.xml | 54 -------------------- .../qemuxml2xmlout-console-compat-auto.xml | 31 ----------- .../qemuxml2xmlout-console-virtio.xml | 29 ----------- .../qemuxml2xmlout-disk-scsi-device-auto.xml | 31 ----------- tests/qemuxml2xmltest.c | 25 +++++++--- 17 files changed, 290 insertions(+), 181 deletions(-) create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-balloon-device-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-channel-virtio-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-console-compat-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-console-virtio-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-disk-scsi-device-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-out.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index a524e4b..9c1e9bb 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -61,9 +61,16 @@ <value>static</value> </choice> </attribute> - <element name="label"> - <text/> - </element> + <optional> + <element name="label"> + <text/> + </element> + </optional> + <optional> + <element name="imagelabel"> + <text/> + </element> + </optional> </element> </define> <define name="hvs"> diff --git a/tests/domainschematest b/tests/domainschematest index 7557cef..085ef93 100755 --- a/tests/domainschematest +++ b/tests/domainschematest @@ -4,7 +4,7 @@ . $srcdir/test-lib.sh . $abs_srcdir/schematestutils.sh -DIRS="domainschemadata qemuxml2argvdata sexpr2xmldata xmconfigdata xml2sexprdata qemuxml2xmloutdata" +DIRS="domainschemadata qemuxml2argvdata sexpr2xmldata xmconfigdata xml2sexprdata qemuxml2xmldata" SCHEMA="domain.rng" check_schema "$DIRS" "$SCHEMA" diff --git a/tests/qemuxml2xmldata/qemuxml2xml-balloon-device-auto-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-balloon-device-auto-out.xml new file mode 100644 index 0000000..ed91e37 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-balloon-device-auto-out.xml @@ -0,0 +1,25 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-channel-virtio-auto-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-channel-virtio-auto-out.xml new file mode 100644 index 0000000..7990374 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-channel-virtio-auto-out.xml @@ -0,0 +1,54 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='virtio-serial' index='0' ports='16' vectors='4'/> + <controller type='virtio-serial' index='1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </controller> + <controller type='virtio-serial' index='2'/> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.0'/> + <address type='virtio-serial' controller='0' bus='0' port='0'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.foo'/> + <address type='virtio-serial' controller='1' bus='0' port='0'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.bar'/> + <address type='virtio-serial' controller='1' bus='0' port='3'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.wizz'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.ooh'/> + <address type='virtio-serial' controller='1' bus='0' port='4'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.lla'/> + <address type='virtio-serial' controller='2' bus='0' port='0'/> + </channel> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-console-compat-auto-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-console-compat-auto-out.xml new file mode 100644 index 0000000..9591c87 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-console-compat-auto-out.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-console-virtio-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-console-virtio-out.xml new file mode 100644 index 0000000..431dd34 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-console-virtio-out.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='virtio-serial' index='0'/> + <console type='pty'> + <target type='virtio' port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-disk-scsi-device-auto-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-disk-scsi-device-auto-out.xml new file mode 100644 index 0000000..a250940 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-disk-scsi-device-auto-out.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='scsi' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-in.xml b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-in.xml new file mode 100644 index 0000000..5cbac90 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-in.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='dynamic' model='selinux'> + <label>foolabel</label> + <imagelabel>fooimagelabel</imagelabel> + </seclabel> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml new file mode 100644 index 0000000..8b344d7 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml @@ -0,0 +1,20 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-in.xml b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-in.xml new file mode 100644 index 0000000..0837a68 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-in.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='static' model='apparmor'> + <label>foolabel</label> + <imagelabel>fooimagelabel</imagelabel> + </seclabel> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-out.xml new file mode 100644 index 0000000..9954175 --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-out.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='static' model='apparmor'> + <label>foolabel</label> + </seclabel> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml deleted file mode 100644 index ed91e37..0000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml +++ /dev/null @@ -1,25 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219200</memory> - <currentMemory>219200</currentMemory> - <vcpu>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml deleted file mode 100644 index 7990374..0000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml +++ /dev/null @@ -1,54 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219200</memory> - <currentMemory>219200</currentMemory> - <vcpu cpuset='1-4,8-20,525'>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <controller type='virtio-serial' index='0' ports='16' vectors='4'/> - <controller type='virtio-serial' index='1'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> - </controller> - <controller type='virtio-serial' index='2'/> - <channel type='pty'> - <target type='virtio' name='org.linux-kvm.port.0'/> - <address type='virtio-serial' controller='0' bus='0' port='0'/> - </channel> - <channel type='pty'> - <target type='virtio' name='org.linux-kvm.port.foo'/> - <address type='virtio-serial' controller='1' bus='0' port='0'/> - </channel> - <channel type='pty'> - <target type='virtio' name='org.linux-kvm.port.bar'/> - <address type='virtio-serial' controller='1' bus='0' port='3'/> - </channel> - <channel type='pty'> - <target type='virtio' name='org.linux-kvm.port.wizz'/> - <address type='virtio-serial' controller='0' bus='0' port='1'/> - </channel> - <channel type='pty'> - <target type='virtio' name='org.linux-kvm.port.ooh'/> - <address type='virtio-serial' controller='1' bus='0' port='4'/> - </channel> - <channel type='pty'> - <target type='virtio' name='org.linux-kvm.port.lla'/> - <address type='virtio-serial' controller='2' bus='0' port='0'/> - </channel> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml deleted file mode 100644 index 9591c87..0000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml +++ /dev/null @@ -1,31 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219200</memory> - <currentMemory>219200</currentMemory> - <vcpu>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <serial type='pty'> - <target port='0'/> - </serial> - <console type='pty'> - <target type='serial' port='0'/> - </console> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml deleted file mode 100644 index 431dd34..0000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml +++ /dev/null @@ -1,29 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219200</memory> - <currentMemory>219200</currentMemory> - <vcpu cpuset='1-4,8-20,525'>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <controller type='virtio-serial' index='0'/> - <console type='pty'> - <target type='virtio' port='0'/> - </console> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml deleted file mode 100644 index a250940..0000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml +++ /dev/null @@ -1,31 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219200</memory> - <currentMemory>219200</currentMemory> - <vcpu>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <disk type='file' device='disk'> - <source file='/tmp/scsidisk.img'/> - <target dev='sda' bus='scsi'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <controller type='scsi' index='0'/> - <memballoon model='virtio'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 326a1f1..2af7494 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -15,9 +15,14 @@ # include "qemu/qemu_conf.h" # include "testutilsqemu.h" +# define XML2ARGV_FMT "%s/qemuxml2argvdata/qemuxml2argv-%s.xml" +# define XML2XMLIN_FMT "%s/qemuxml2xmldata/qemuxml2xml-%s-in.xml" +# define XML2XMLOUT_FMT "%s/qemuxml2xmldata/qemuxml2xml-%s-out.xml" + static char *progname; static char *abs_srcdir; static struct qemud_driver driver; +static char *input_folder_fmt; # define MAX_FILE 4096 @@ -58,7 +63,7 @@ static int testCompareXMLToXMLFiles(const char *inxml, const char *outxml) { struct testInfo { const char *name; - int different; + bool different; }; static int testCompareXMLToXMLHelper(const void *data) { @@ -67,10 +72,8 @@ static int testCompareXMLToXMLHelper(const void *data) { char xml_out[PATH_MAX]; int ret; - snprintf(xml_in, PATH_MAX, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml", - abs_srcdir, info->name); - snprintf(xml_out, PATH_MAX, "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml", - abs_srcdir, info->name); + snprintf(xml_in, PATH_MAX, input_folder_fmt, abs_srcdir, info->name); + snprintf(xml_out, PATH_MAX, XML2XMLOUT_FMT, abs_srcdir, info->name); if (info->different) { ret = testCompareXMLToXMLFiles(xml_in, xml_out); @@ -111,16 +114,18 @@ mymain(int argc, char **argv) } while (0) # define DO_TEST(name) \ - DO_TEST_FULL(name, 0) + DO_TEST_FULL(name, false) # define DO_TEST_DIFFERENT(name) \ - DO_TEST_FULL(name, 1) + DO_TEST_FULL(name, true) /* Unset or set all envvars here that are copied in qemudBuildCommandLine * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected * values for these envvars */ setenv("PATH", "/bin", 1); + input_folder_fmt = (char *) XML2ARGV_FMT; + DO_TEST("minimal"); DO_TEST("boot-cdrom"); DO_TEST("boot-network"); @@ -190,6 +195,12 @@ mymain(int argc, char **argv) DO_TEST_DIFFERENT("disk-scsi-device-auto"); DO_TEST_DIFFERENT("console-virtio"); + + /* All tests after this point use input and output from qemuxml2xmldata*/ + input_folder_fmt = (char *) XML2XMLIN_FMT; + DO_TEST_DIFFERENT("seclabel-dynamic"); + DO_TEST_DIFFERENT("seclabel-static"); + virCapabilitiesFree(driver.caps); return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); -- 1.7.3.2
On 01/12/2011 10:22 AM, Cole Robinson wrote:
+++ b/tests/domainschematest @@ -4,7 +4,7 @@ . $srcdir/test-lib.sh . $abs_srcdir/schematestutils.sh
-DIRS="domainschemadata qemuxml2argvdata sexpr2xmldata xmconfigdata xml2sexprdata qemuxml2xmloutdata" +DIRS="domainschemadata qemuxml2argvdata sexpr2xmldata xmconfigdata xml2sexprdata qemuxml2xmldata"
At first, I though this should be listing _both_ qemuxml2xml{out,}data, rather than just one or the other, but now I see that you've moved the only five files that were in that directory so that all qemu*.xml files are in a single location, and adjusted the test to match. Nice cleanup, but incomplete - you also need to tweak tests/Makefile.am to drop reference to the deleted qemuxml2xmloutdata directory, so that 'make dist' won't complain. This would have been a little more obvious to me if you had used git diff rename compression (git config diff.renames true), rather than showing the moved files as deletions and creations. In fact, separating into two patches (file movement consolidation, vs. new tests), might be nice, if you're up to the task, but I won't insist.
@@ -67,10 +72,8 @@ static int testCompareXMLToXMLHelper(const void *data) { char xml_out[PATH_MAX]; int ret;
- snprintf(xml_in, PATH_MAX, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml", - abs_srcdir, info->name); - snprintf(xml_out, PATH_MAX, "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml", - abs_srcdir, info->name); + snprintf(xml_in, PATH_MAX, input_folder_fmt, abs_srcdir, info->name); + snprintf(xml_out, PATH_MAX, XML2XMLOUT_FMT, abs_srcdir, info->name);
Hmm, this silently ignores overflow if run in a deeply nested hierarchy. Someday, we should switch these to virAsprintf (and as a bonus, it avoids PATH_MAX stack allocation). But that's independent of this patch. Conditional ACK, once you fix the tests/Makefile.am EXTRA_DIST nit. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 12, 2011 at 12:22:57PM -0500, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/schemas/domain.rng | 13 ++++- tests/domainschematest | 2 +- .../qemuxml2xml-balloon-device-auto-out.xml | 25 +++++++++ .../qemuxml2xml-channel-virtio-auto-out.xml | 54 ++++++++++++++++++++ .../qemuxml2xml-console-compat-auto-out.xml | 31 +++++++++++ .../qemuxml2xml-console-virtio-out.xml | 29 +++++++++++ .../qemuxml2xml-disk-scsi-device-auto-out.xml | 31 +++++++++++ .../qemuxml2xml-seclabel-dynamic-in.xml | 24 +++++++++ .../qemuxml2xml-seclabel-dynamic-out.xml | 20 +++++++ .../qemuxml2xml-seclabel-static-in.xml | 24 +++++++++ .../qemuxml2xml-seclabel-static-out.xml | 23 ++++++++ .../qemuxml2xmlout-balloon-device-auto.xml | 25 --------- .../qemuxml2xmlout-channel-virtio-auto.xml | 54 -------------------- .../qemuxml2xmlout-console-compat-auto.xml | 31 ----------- .../qemuxml2xmlout-console-virtio.xml | 29 ----------- .../qemuxml2xmlout-disk-scsi-device-auto.xml | 31 ----------- tests/qemuxml2xmltest.c | 25 +++++++--- 17 files changed, 290 insertions(+), 181 deletions(-) create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-balloon-device-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-channel-virtio-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-console-compat-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-console-virtio-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-disk-scsi-device-auto-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-static-out.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml
ACK with the caveat Eric mentioned already. Daniel
Renamed from virSecurityLabelDefFree. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- cfg.mk | 1 + src/conf/domain_conf.c | 17 +++++++++-------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 4 +--- src/security/security_apparmor.c | 11 ++--------- src/security/security_selinux.c | 9 ++------- 7 files changed, 17 insertions(+), 28 deletions(-) diff --git a/cfg.mk b/cfg.mk index 03186b3..2c6f595 100644 --- a/cfg.mk +++ b/cfg.mk @@ -98,6 +98,7 @@ useless_free_options = \ --name=virDomainInputDefFree \ --name=virDomainNetDefFree \ --name=virDomainObjFree \ + --name=virDomainSeclabelDefClear \ --name=virDomainSnapshotDefFree \ --name=virDomainSnapshotObjFree \ --name=virDomainSoundDefFree \ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4df38c..8f6ef55 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -754,13 +754,14 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) VIR_FREE(def); } -void virSecurityLabelDefFree(virDomainDefPtr def); - -void virSecurityLabelDefFree(virDomainDefPtr def) +void virDomainSeclabelDefClear(virSecurityLabelDefPtr seclabel) { - VIR_FREE(def->seclabel.model); - VIR_FREE(def->seclabel.label); - VIR_FREE(def->seclabel.imagelabel); + if (!seclabel) + return; + + VIR_FREE(seclabel->model); + VIR_FREE(seclabel->label); + VIR_FREE(seclabel->imagelabel); } static void @@ -855,7 +856,7 @@ void virDomainDefFree(virDomainDefPtr def) virDomainMemballoonDefFree(def->memballoon); - virSecurityLabelDefFree(def); + virDomainSeclabelDefClear(&def->seclabel); virCPUDefFree(def->cpu); @@ -4272,7 +4273,7 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, return 0; error: - virSecurityLabelDefFree(def); + virDomainSeclabelDefClear(&def->seclabel); return -1; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a459a22..b5cf433 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1082,6 +1082,7 @@ void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); void virDomainVideoDefFree(virDomainVideoDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); +void virDomainSeclabelDefClear(virSecurityLabelDefPtr seclabel); int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e9b8cb7..191ae6a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -279,6 +279,7 @@ virDomainRemoveInactive; virDomainSaveConfig; virDomainSaveStatus; virDomainSaveXML; +virDomainSeclabelDefClear; virDomainSnapshotAssignDef; virDomainSnapshotDefFormat; virDomainSnapshotDefFree; @@ -313,7 +314,6 @@ virDomainWatchdogActionTypeToString; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString; - # domain_event.h virDomainEventCallbackListAdd; virDomainEventCallbackListAddID; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9eb9cd5..3745cce 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3043,9 +3043,7 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, /* Clear out dynamically assigned labels */ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - VIR_FREE(vm->def->seclabel.model); - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); + virDomainSeclabelDefClear(&vm->def->seclabel); } virDomainDefClearDeviceAliases(vm->def); diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index d82ba73..42f812c 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -475,9 +475,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto clean; err: - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); - VIR_FREE(vm->def->seclabel.model); + virDomainSeclabelDefClear(&vm->def->seclabel); clean: VIR_FREE(profile_name); @@ -547,12 +545,7 @@ static int AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainObjPtr vm) { - const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - - VIR_FREE(secdef->model); - VIR_FREE(secdef->label); - VIR_FREE(secdef->imagelabel); - + virDomainSeclabelDefClear(&vm->def->seclabel); return 0; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index d06afde..f11e209 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -216,9 +216,7 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, rc = 0; goto done; err: - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); - VIR_FREE(vm->def->seclabel.model); + virDomainSeclabelDefClear(&vm->def->seclabel); done: VIR_FREE(scontext); return rc; @@ -830,10 +828,7 @@ SELinuxReleaseSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, context_free(con); } - VIR_FREE(secdef->model); - VIR_FREE(secdef->label); - VIR_FREE(secdef->imagelabel); - + virDomainSeclabelDefClear(secdef); return 0; } -- 1.7.3.2
On 01/12/2011 10:22 AM, Cole Robinson wrote:
Renamed from virSecurityLabelDefFree.
I like the rename, but...
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- cfg.mk | 1 + src/conf/domain_conf.c | 17 +++++++++-------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 4 +--- src/security/security_apparmor.c | 11 ++--------- src/security/security_selinux.c | 9 ++------- 7 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 03186b3..2c6f595 100644 --- a/cfg.mk +++ b/cfg.mk @@ -98,6 +98,7 @@ useless_free_options = \ --name=virDomainInputDefFree \ --name=virDomainNetDefFree \ --name=virDomainObjFree \ + --name=virDomainSeclabelDefClear \
This is wrong. It is _not_ a free-like function, because it does not free the passed in pointer. You probably want to have two functions: /* clear guts only, must be given valid pointer */ void virDomainSeclabelDefClear(ptr) ATTRIBUTE_NONNULL(1); /* clear entire allocated pointer, free-like */ void virDomainSeclabelDefFree(ptr) { if (!ptr) return; virDomainSeclabelDefClear(ptr); VIR_FREE(ptr); } and list only virDomainSeclabelDefFree in cfg.mk.
-void virSecurityLabelDefFree(virDomainDefPtr def) +void virDomainSeclabelDefClear(virSecurityLabelDefPtr seclabel) { - VIR_FREE(def->seclabel.model); - VIR_FREE(def->seclabel.label); - VIR_FREE(def->seclabel.imagelabel); + if (!seclabel) + return;
which means this early exit should only be part of the Free variant, not the Clear variant.
+ + VIR_FREE(seclabel->model); + VIR_FREE(seclabel->label); + VIR_FREE(seclabel->imagelabel); }
static void @@ -855,7 +856,7 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainMemballoonDefFree(def->memballoon);
- virSecurityLabelDefFree(def); + virDomainSeclabelDefClear(&def->seclabel);
But hunks like this are okay (when a SeclabelDef occurs as an inline struct member, rather than an indirect SeclabelDefPtr pointer).
+++ b/src/libvirt_private.syms @@ -279,6 +279,7 @@ virDomainRemoveInactive; virDomainSaveConfig; virDomainSaveStatus; virDomainSaveXML; +virDomainSeclabelDefClear; virDomainSnapshotAssignDef; virDomainSnapshotDefFormat; virDomainSnapshotDefFree; @@ -313,7 +314,6 @@ virDomainWatchdogActionTypeToString; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString;
- # domain_event.h
Why the whitespace change? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 12, 2011 at 12:22:58PM -0500, Cole Robinson wrote:
Renamed from virSecurityLabelDefFree.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- cfg.mk | 1 + src/conf/domain_conf.c | 17 +++++++++-------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 4 +--- src/security/security_apparmor.c | 11 ++--------- src/security/security_selinux.c | 9 ++------- 7 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 03186b3..2c6f595 100644 --- a/cfg.mk +++ b/cfg.mk @@ -98,6 +98,7 @@ useless_free_options = \ --name=virDomainInputDefFree \ --name=virDomainNetDefFree \ --name=virDomainObjFree \ + --name=virDomainSeclabelDefClear \
This isn't needed
--name=virDomainSnapshotDefFree \ --name=virDomainSnapshotObjFree \ --name=virDomainSoundDefFree \ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4df38c..8f6ef55 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -754,13 +754,14 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) VIR_FREE(def); }
-void virSecurityLabelDefFree(virDomainDefPtr def); - -void virSecurityLabelDefFree(virDomainDefPtr def) +void virDomainSeclabelDefClear(virSecurityLabelDefPtr seclabel) { - VIR_FREE(def->seclabel.model); - VIR_FREE(def->seclabel.label); - VIR_FREE(def->seclabel.imagelabel); + if (!seclabel) + return;
Nor is this.
+ VIR_FREE(seclabel->model); + VIR_FREE(seclabel->label); + VIR_FREE(seclabel->imagelabel); }
static void @@ -855,7 +856,7 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainMemballoonDefFree(def->memballoon);
- virSecurityLabelDefFree(def); + virDomainSeclabelDefClear(&def->seclabel);
virCPUDefFree(def->cpu);
@@ -4272,7 +4273,7 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, return 0;
error: - virSecurityLabelDefFree(def); + virDomainSeclabelDefClear(&def->seclabel); return -1; }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a459a22..b5cf433 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1082,6 +1082,7 @@ void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); void virDomainVideoDefFree(virDomainVideoDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); +void virDomainSeclabelDefClear(virSecurityLabelDefPtr seclabel); int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e9b8cb7..191ae6a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -279,6 +279,7 @@ virDomainRemoveInactive; virDomainSaveConfig; virDomainSaveStatus; virDomainSaveXML; +virDomainSeclabelDefClear; virDomainSnapshotAssignDef; virDomainSnapshotDefFormat; virDomainSnapshotDefFree; @@ -313,7 +314,6 @@ virDomainWatchdogActionTypeToString; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString;
- # domain_event.h virDomainEventCallbackListAdd; virDomainEventCallbackListAddID; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9eb9cd5..3745cce 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3043,9 +3043,7 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver,
/* Clear out dynamically assigned labels */ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - VIR_FREE(vm->def->seclabel.model); - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); + virDomainSeclabelDefClear(&vm->def->seclabel); }
virDomainDefClearDeviceAliases(vm->def); diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index d82ba73..42f812c 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -475,9 +475,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto clean;
err: - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); - VIR_FREE(vm->def->seclabel.model); + virDomainSeclabelDefClear(&vm->def->seclabel);
clean: VIR_FREE(profile_name); @@ -547,12 +545,7 @@ static int AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainObjPtr vm) { - const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - - VIR_FREE(secdef->model); - VIR_FREE(secdef->label); - VIR_FREE(secdef->imagelabel); - + virDomainSeclabelDefClear(&vm->def->seclabel); return 0; }
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index d06afde..f11e209 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -216,9 +216,7 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, rc = 0; goto done; err: - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); - VIR_FREE(vm->def->seclabel.model); + virDomainSeclabelDefClear(&vm->def->seclabel); done: VIR_FREE(scontext); return rc; @@ -830,10 +828,7 @@ SELinuxReleaseSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, context_free(con); }
- VIR_FREE(secdef->model); - VIR_FREE(secdef->label); - VIR_FREE(secdef->imagelabel); - + virDomainSeclabelDefClear(secdef); return 0; }
ACK to the rest of the patch Daniel
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_apparmor.c | 7 +------ src/security/security_driver.c | 19 +++++++++++++++++++ src/security/security_driver.h | 3 +++ src/security/security_selinux.c | 29 +++++------------------------ 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 42f812c..00e5a01 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -581,12 +581,7 @@ AppArmorSetSecurityProcessLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) if ((profile_name = get_profile_name(vm)) == NULL) return rc; - if (STRNEQ(virSecurityManagerGetModel(mgr), secdef->model)) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("security label driver mismatch: " - "\'%s\' model configured for domain, but " - "hypervisor driver is \'%s\'."), - secdef->model, virSecurityManagerGetModel(mgr)); + if (!virSecurityIsSpecifiedDriver(mgr, vm->def)) { if (use_apparmor() > 0) goto clean; } diff --git a/src/security/security_driver.c b/src/security/security_driver.c index fd2c01a..5711aee 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -75,3 +75,22 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name) return drv; } + +bool +virSecurityIsSpecifiedDriver(virSecurityManagerPtr mgr, + virDomainDefPtr def) +{ + bool ret = true; + + if (!STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) { + virSecurityReportError(VIR_ERR_INTERNAL_ERROR, + _("security label driver mismatch: " + "'%s' model configured for domain, but " + "hypervisor driver is '%s'."), + def->seclabel.model, + virSecurityManagerGetModel(mgr)); + ret = false; + } + + return ret; +} diff --git a/src/security/security_driver.h b/src/security/security_driver.h index e5a8d41..a0b15f4 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -117,5 +117,8 @@ struct _virSecurityDriver { }; virSecurityDriverPtr virSecurityDriverLookup(const char *name); +bool virSecurityIsSpecifiedDriver(virSecurityManagerPtr mgr, + virDomainDefPtr def) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif /* __VIR_SECURITY_H__ */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f11e209..f3b76f9 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -866,12 +866,8 @@ SELinuxSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def) { const virSecurityLabelDefPtr secdef = &def->seclabel; - if (!STREQ(virSecurityManagerGetModel(mgr), secdef->model)) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("security label driver mismatch: " - "'%s' model configured for domain, but " - "hypervisor driver is '%s'."), - secdef->model, virSecurityManagerGetModel(mgr)); + + if (!virSecurityIsSpecifiedDriver(mgr, def)) { return -1; } @@ -895,12 +891,7 @@ SELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr, if (vm->def->seclabel.label == NULL) return 0; - if (!STREQ(virSecurityManagerGetModel(mgr), secdef->model)) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("security label driver mismatch: " - "'%s' model configured for domain, but " - "hypervisor driver is '%s'."), - secdef->model, virSecurityManagerGetModel(mgr)); + if (!virSecurityIsSpecifiedDriver(mgr, vm->def)) { if (security_getenforce() == 1) return -1; } @@ -930,12 +921,7 @@ SELinuxSetSecuritySocketLabel(virSecurityManagerPtr mgr, if (vm->def->seclabel.label == NULL) return 0; - if (!STREQ(virSecurityManagerGetModel(mgr), secdef->model)) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("security label driver mismatch: " - "'%s' model configured for domain, but " - "hypervisor driver is '%s'."), - secdef->model, virSecurityManagerGetModel(mgr)); + if (!virSecurityIsSpecifiedDriver(mgr, vm->def)) { goto done; } @@ -997,12 +983,7 @@ SELinuxClearSecuritySocketLabel(virSecurityManagerPtr mgr, if (vm->def->seclabel.label == NULL) return 0; - if (!STREQ(virSecurityManagerGetModel(mgr), secdef->model)) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("security label driver mismatch: " - "'%s' model configured for domain, but " - "hypervisor driver is '%s'."), - secdef->model, virSecurityManagerGetModel(mgr)); + if (!virSecurityIsSpecifiedDriver(mgr, vm->def)) { if (security_getenforce() == 1) return -1; } -- 1.7.3.2
On 01/12/2011 10:22 AM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_apparmor.c | 7 +------ src/security/security_driver.c | 19 +++++++++++++++++++ src/security/security_driver.h | 3 +++ src/security/security_selinux.c | 29 +++++------------------------ 4 files changed, 28 insertions(+), 30 deletions(-)
The summary line of the diffstat doesn't show it very well, but I like this refactorization. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 12, 2011 at 12:22:59PM -0500, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_apparmor.c | 7 +------ src/security/security_driver.c | 19 +++++++++++++++++++ src/security/security_driver.h | 3 +++ src/security/security_selinux.c | 29 +++++------------------------ 4 files changed, 28 insertions(+), 30 deletions(-)
ACK Daniel
If the selinux driver is the default, it will lead to a null dereference. This change should still yeild the intended result, since a null model basically implies 'use the default'. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 5711aee..7d2e0de 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -82,7 +82,8 @@ virSecurityIsSpecifiedDriver(virSecurityManagerPtr mgr, { bool ret = true; - if (!STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) { + if (def->seclabel.model && + !STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("security label driver mismatch: " "'%s' model configured for domain, but " -- 1.7.3.2
On 01/12/2011 10:23 AM, Cole Robinson wrote:
If the selinux driver is the default, it will lead to a null dereference. This change should still yeild the intended result, since a null model basically implies 'use the default'.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 5711aee..7d2e0de 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -82,7 +82,8 @@ virSecurityIsSpecifiedDriver(virSecurityManagerPtr mgr, { bool ret = true;
- if (!STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) { + if (def->seclabel.model && + !STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) {
ACK; and this probably also fixes segfaults for other drivers given your 3/7 cleanup. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 12, 2011 at 12:23:00PM -0500, Cole Robinson wrote:
If the selinux driver is the default, it will lead to a null dereference. This change should still yeild the intended result, since a null model basically implies 'use the default'.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 5711aee..7d2e0de 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -82,7 +82,8 @@ virSecurityIsSpecifiedDriver(virSecurityManagerPtr mgr, { bool ret = true;
- if (!STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) { + if (def->seclabel.model && + !STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("security label driver mismatch: " "'%s' model configured for domain, but "
ACK it doesn't hurt, but it should have also been checked at the higher level in the Verify function Daniel
This allows us to explicitly handle the 'default' seclabel case, as well as provide easier model validation. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++++++-------- src/conf/domain_conf.h | 14 ++++++++++++-- src/security/security_apparmor.c | 9 +++------ src/security/security_driver.c | 15 ++++++++++----- src/security/security_selinux.c | 8 ++------ 5 files changed, 57 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f6ef55..077a396 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -313,6 +313,12 @@ VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST, "dynamic", "static") +VIR_ENUM_IMPL(virDomainSeclabelModel, VIR_DOMAIN_SECLABEL_MODEL_LAST, + "default", + "selinux", + "apparmor", + "none") + VIR_ENUM_IMPL(virDomainNetdevMacvtap, VIR_DOMAIN_NETDEV_MACVTAP_MODE_LAST, "vepa", "private", @@ -759,7 +765,7 @@ void virDomainSeclabelDefClear(virSecurityLabelDefPtr seclabel) if (!seclabel) return; - VIR_FREE(seclabel->model); + seclabel->model = VIR_DOMAIN_SECLABEL_MODEL_DEFAULT; VIR_FREE(seclabel->label); VIR_FREE(seclabel->imagelabel); } @@ -4244,7 +4250,15 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, "%s", _("missing security model")); goto error; } - def->seclabel.model = p; + + def->seclabel.model = virDomainSeclabelModelTypeFromString(p); + if (def->seclabel.model < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown security model '%s'"), p); + VIR_FREE(p); + goto error; + } + VIR_FREE(p); p = virXPathStringLimit("string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -7336,18 +7350,26 @@ char *virDomainDefFormat(virDomainDefPtr def, virBufferAddLit(&buf, " </devices>\n"); - if (def->seclabel.model) { - const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type); + if (def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT) { + const char *sectype, *secmodel; + + sectype = virDomainSeclabelTypeToString(def->seclabel.type); if (!sectype) goto cleanup; + + secmodel = virDomainSeclabelModelTypeToString(def->seclabel.model); + if (!secmodel) + goto cleanup; + + virBufferVSprintf(&buf, " <seclabel type='%s' model='%s'", + sectype, secmodel); + if (!def->seclabel.label || (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && (flags & VIR_DOMAIN_XML_INACTIVE))) { - virBufferVSprintf(&buf, " <seclabel type='%s' model='%s'/>\n", - sectype, def->seclabel.model); + virBufferAddLit(&buf, "/>\n"); } else { - virBufferVSprintf(&buf, " <seclabel type='%s' model='%s'>\n", - sectype, def->seclabel.model); + virBufferAddLit(&buf, ">\n"); virBufferEscapeString(&buf, " <label>%s</label>\n", def->seclabel.label); if (def->seclabel.imagelabel && diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b5cf433..81409f8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -782,14 +782,23 @@ enum virDomainSeclabelType { VIR_DOMAIN_SECLABEL_LAST, }; +enum virDomainSeclabelModel { + VIR_DOMAIN_SECLABEL_MODEL_DEFAULT, + VIR_DOMAIN_SECLABEL_MODEL_SELINUX, + VIR_DOMAIN_SECLABEL_MODEL_APPARMOR, + VIR_DOMAIN_SECLABEL_MODEL_NONE, + + VIR_DOMAIN_SECLABEL_MODEL_LAST, +}; + /* Security configuration for domain */ typedef struct _virSecurityLabelDef virSecurityLabelDef; typedef virSecurityLabelDef *virSecurityLabelDefPtr; struct _virSecurityLabelDef { - char *model; /* name of security model */ char *label; /* security label string */ char *imagelabel; /* security image label string */ - int type; + int type; /* dynamic vs. static */ + int model; /* name of security model */ }; enum virDomainTimerNameType { @@ -1287,6 +1296,7 @@ VIR_ENUM_DECL(virDomainGraphicsSpiceChannelMode) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainSeclabel) +VIR_ENUM_DECL(virDomainSeclabelModel) VIR_ENUM_DECL(virDomainClockOffset) VIR_ENUM_DECL(virDomainNetdevMacvtap) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 00e5a01..7a6fe5c 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -441,7 +441,8 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; if ((vm->def->seclabel.label) || - (vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) { + (vm->def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT) || + (vm->def->seclabel.imagelabel)) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("security label already defined for VM")); @@ -465,11 +466,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto err; } - vm->def->seclabel.model = strdup(SECURITY_APPARMOR_NAME); - if (!vm->def->seclabel.model) { - virReportOOMError(); - goto err; - } + vm->def->seclabel.model = VIR_DOMAIN_SECLABEL_MODEL_APPARMOR; rc = 0; goto clean; diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 7d2e0de..2cd4c0e 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -80,18 +80,23 @@ bool virSecurityIsSpecifiedDriver(virSecurityManagerPtr mgr, virDomainDefPtr def) { - bool ret = true; + bool ret = false; + const char *model = virDomainSeclabelModelTypeToString(def->seclabel.model); + if (!model) + goto err; - if (def->seclabel.model && - !STREQ(virSecurityManagerGetModel(mgr), def->seclabel.model)) { + if (def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT && + !STREQ(virSecurityManagerGetModel(mgr), model)) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, _("security label driver mismatch: " "'%s' model configured for domain, but " "hypervisor driver is '%s'."), - def->seclabel.model, + model, virSecurityManagerGetModel(mgr)); - ret = false; + goto err; } + ret = true; +err: return ret; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f3b76f9..2266c21 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -173,7 +173,7 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; if (vm->def->seclabel.label || - vm->def->seclabel.model || + vm->def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT || vm->def->seclabel.imagelabel) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("security label already defined for VM")); @@ -206,12 +206,8 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, _("cannot generate selinux context for %s"), mcs); goto err; } - vm->def->seclabel.model = strdup(SECURITY_SELINUX_NAME); - if (!vm->def->seclabel.model) { - virReportOOMError(); - goto err; - } + vm->def->seclabel.model = VIR_DOMAIN_SECLABEL_MODEL_SELINUX; rc = 0; goto done; -- 1.7.3.2
On 01/12/2011 10:23 AM, Cole Robinson wrote:
This allows us to explicitly handle the 'default' seclabel case, as well as provide easier model validation.
Signed-off-by: Cole Robinson <crobinso@redhat.com> ---
I'm not so sure about this one. I didn't see any coding bugs in the actual patch, but design-wise:
src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++++++-------- src/conf/domain_conf.h | 14 ++++++++++++-- src/security/security_apparmor.c | 9 +++------ src/security/security_driver.c | 15 ++++++++++----- src/security/security_selinux.c | 8 ++------ 5 files changed, 57 insertions(+), 27 deletions(-)
1. it's already a net gain in lines of code 2. to me, it really seems like it will only be useful if we go with patch 7/7 (which Daniel already NACK'd). 3. ...
+VIR_ENUM_IMPL(virDomainSeclabelModel, VIR_DOMAIN_SECLABEL_MODEL_LAST, + "default", + "selinux", + "apparmor", + "none")
This means the FromString knows how to parse "default",
@@ -4244,7 +4250,15 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, "%s", _("missing security model")); goto error; } - def->seclabel.model = p; + + def->seclabel.model = virDomainSeclabelModelTypeFromString(p); + if (def->seclabel.model < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown security model '%s'"), p); + VIR_FREE(p); + goto error; + }
while this doesn't reject such a parse,
@@ -7336,18 +7350,26 @@ char *virDomainDefFormat(virDomainDefPtr def,
virBufferAddLit(&buf, " </devices>\n");
- if (def->seclabel.model) { - const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type); + if (def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT) {
yet this intentionally avoids outputting model='default', and overall, you didn't modify the domain.rng or docs to mention the new attribute value. I'm assuming you didn't intend to extend the xml to allow model='default', which means you either want:
+enum virDomainSeclabelModel { + VIR_DOMAIN_SECLABEL_MODEL_DEFAULT,
this to be = -1,
+ VIR_DOMAIN_SECLABEL_MODEL_SELINUX,
and this 0, and just omit "default" from the VIR_ENUM_IMPL; or that you have to correct the parseXML function to reject an explicit model='default' parse. Since non-zero defaults have their own set of interesting issues (you'd have to track down every place in code that allocates a virDomainSeclabelDef and ensure that it gets initialized correctly), I'd lean to the latter option, but both options add complexity to this patch. So unless we can come up with another reason why this is worth including, I'm inclined to just omit this one. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 12, 2011 at 01:48:24PM -0700, Eric Blake wrote:
On 01/12/2011 10:23 AM, Cole Robinson wrote:
This allows us to explicitly handle the 'default' seclabel case, as well as provide easier model validation.
Signed-off-by: Cole Robinson <crobinso@redhat.com> ---
I'm not so sure about this one. I didn't see any coding bugs in the actual patch, but design-wise:
src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++++++-------- src/conf/domain_conf.h | 14 ++++++++++++-- src/security/security_apparmor.c | 9 +++------ src/security/security_driver.c | 15 ++++++++++----- src/security/security_selinux.c | 8 ++------ 5 files changed, 57 insertions(+), 27 deletions(-)
1. it's already a net gain in lines of code
2. to me, it really seems like it will only be useful if we go with patch 7/7 (which Daniel already NACK'd).
Even though the code is a bit longer, I think it is good practice to use an ENUM for handling this.
+VIR_ENUM_IMPL(virDomainSeclabelModel, VIR_DOMAIN_SECLABEL_MODEL_LAST, + "default", + "selinux", + "apparmor", + "none")
This means the FromString knows how to parse "default",
@@ -4244,7 +4250,15 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, "%s", _("missing security model")); goto error; } - def->seclabel.model = p; + + def->seclabel.model = virDomainSeclabelModelTypeFromString(p); + if (def->seclabel.model < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown security model '%s'"), p); + VIR_FREE(p); + goto error; + }
while this doesn't reject such a parse,
@@ -7336,18 +7350,26 @@ char *virDomainDefFormat(virDomainDefPtr def,
virBufferAddLit(&buf, " </devices>\n");
- if (def->seclabel.model) { - const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type); + if (def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT) {
yet this intentionally avoids outputting model='default',
Overall, this kind of state of affairs is OK. I'm fairly sure we've other areas where the 'default' is silently accepted in parse, but not output when formatting for sake of brevity.
and overall, you didn't modify the domain.rng or docs to mention the new attribute value. I'm assuming you didn't intend to extend the xml to allow model='default', which means you either want:
+enum virDomainSeclabelModel { + VIR_DOMAIN_SECLABEL_MODEL_DEFAULT,
this to be = -1,
+ VIR_DOMAIN_SECLABEL_MODEL_SELINUX,
and this 0, and just omit "default" from the VIR_ENUM_IMPL; or that you
No, we really do want the default value to be 0. It is important for defaults to be zero, so that when you VIR_ALLOC you get the correct defaults automatically. Regards, Daniel
On Wed, Jan 12, 2011 at 12:23:01PM -0500, Cole Robinson wrote:
This allows us to explicitly handle the 'default' seclabel case, as well as provide easier model validation.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++++++-------- src/conf/domain_conf.h | 14 ++++++++++++-- src/security/security_apparmor.c | 9 +++------ src/security/security_driver.c | 15 ++++++++++----- src/security/security_selinux.c | 8 ++------ 5 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f6ef55..077a396 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -313,6 +313,12 @@ VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST, "dynamic", "static")
+VIR_ENUM_IMPL(virDomainSeclabelModel, VIR_DOMAIN_SECLABEL_MODEL_LAST, + "default", + "selinux", + "apparmor", + "none")
If we remove 'none' from the enum, this is ok.
+ VIR_ENUM_IMPL(virDomainNetdevMacvtap, VIR_DOMAIN_NETDEV_MACVTAP_MODE_LAST, "vepa", "private", @@ -759,7 +765,7 @@ void virDomainSeclabelDefClear(virSecurityLabelDefPtr seclabel) if (!seclabel) return;
- VIR_FREE(seclabel->model); + seclabel->model = VIR_DOMAIN_SECLABEL_MODEL_DEFAULT; VIR_FREE(seclabel->label); VIR_FREE(seclabel->imagelabel); } @@ -4244,7 +4250,15 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, "%s", _("missing security model")); goto error; } - def->seclabel.model = p; + + def->seclabel.model = virDomainSeclabelModelTypeFromString(p); + if (def->seclabel.model < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown security model '%s'"), p); + VIR_FREE(p); + goto error; + } + VIR_FREE(p);
p = virXPathStringLimit("string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -7336,18 +7350,26 @@ char *virDomainDefFormat(virDomainDefPtr def,
virBufferAddLit(&buf, " </devices>\n");
- if (def->seclabel.model) { - const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type); + if (def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT) { + const char *sectype, *secmodel; + + sectype = virDomainSeclabelTypeToString(def->seclabel.type); if (!sectype) goto cleanup; + + secmodel = virDomainSeclabelModelTypeToString(def->seclabel.model); + if (!secmodel) + goto cleanup; + + virBufferVSprintf(&buf, " <seclabel type='%s' model='%s'", + sectype, secmodel); + if (!def->seclabel.label || (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && (flags & VIR_DOMAIN_XML_INACTIVE))) { - virBufferVSprintf(&buf, " <seclabel type='%s' model='%s'/>\n", - sectype, def->seclabel.model); + virBufferAddLit(&buf, "/>\n"); } else { - virBufferVSprintf(&buf, " <seclabel type='%s' model='%s'>\n", - sectype, def->seclabel.model); + virBufferAddLit(&buf, ">\n"); virBufferEscapeString(&buf, " <label>%s</label>\n", def->seclabel.label); if (def->seclabel.imagelabel && diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b5cf433..81409f8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -782,14 +782,23 @@ enum virDomainSeclabelType { VIR_DOMAIN_SECLABEL_LAST, };
+enum virDomainSeclabelModel { + VIR_DOMAIN_SECLABEL_MODEL_DEFAULT, + VIR_DOMAIN_SECLABEL_MODEL_SELINUX, + VIR_DOMAIN_SECLABEL_MODEL_APPARMOR, + VIR_DOMAIN_SECLABEL_MODEL_NONE, + + VIR_DOMAIN_SECLABEL_MODEL_LAST, +};
Remove NONE here too. ACK, if the 'none' / NONE bits are removed. Daniel
This will help facilitate disabling seclabel for an individual VM. One functional change is that the user can now hardcode type='dynamic', but there was no good reason to deny it anyways. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 34 ++++++++++---------- src/security/security_apparmor.c | 6 ++-- src/security/security_selinux.c | 6 ++-- .../qemuxml2xml-seclabel-dynamic-out.xml | 1 + 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 077a396..e5b89a2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4238,28 +4238,28 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, goto error; } + p = virXPathStringLimit("string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + + def->seclabel.model = virDomainSeclabelModelTypeFromString(p); + if (def->seclabel.model < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown security model '%s'"), p); + VIR_FREE(p); + goto error; + } + VIR_FREE(p); + /* Only parse details, if using static labels, or * if the 'live' VM XML is requested */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags & VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit("string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("missing security model")); - goto error; - } - - def->seclabel.model = virDomainSeclabelModelTypeFromString(p); - if (def->seclabel.model < 0) { - virDomainReportError(VIR_ERR_XML_ERROR, - _("unknown security model '%s'"), p); - VIR_FREE(p); - goto error; - } - VIR_FREE(p); - p = virXPathStringLimit("string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p == NULL) { diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 7a6fe5c..9a49e29 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -431,7 +431,7 @@ AppArmorSecurityManagerGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) * called on shutdown. */ static int -AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +AppArmorGenSecurityLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { int rc = -1; @@ -440,8 +440,8 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC) return 0; - if ((vm->def->seclabel.label) || - (vm->def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT) || + if ((!virSecurityIsSpecifiedDriver(mgr, vm->def)) || + (vm->def->seclabel.label) || (vm->def->seclabel.imagelabel)) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2266c21..a03e7d2 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -160,7 +160,7 @@ SELinuxInitialize(void) } static int -SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +SELinuxGenSecurityLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { int rc = -1; @@ -172,8 +172,8 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC) return 0; - if (vm->def->seclabel.label || - vm->def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT || + if (!virSecurityIsSpecifiedDriver(mgr, vm->def) || + vm->def->seclabel.label || vm->def->seclabel.imagelabel) { virSecurityReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("security label already defined for VM")); diff --git a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml index 8b344d7..8d3f837 100644 --- a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml +++ b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-dynamic-out.xml @@ -17,4 +17,5 @@ <controller type='ide' index='0'/> <memballoon model='virtio'/> </devices> + <seclabel type='dynamic' model='selinux'/> </domain> -- 1.7.3.2
On 01/12/2011 10:23 AM, Cole Robinson wrote:
This will help facilitate disabling seclabel for an individual VM. One functional change is that the user can now hardcode type='dynamic', but there was no good reason to deny it anyways.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 34 ++++++++++---------- src/security/security_apparmor.c | 6 ++-- src/security/security_selinux.c | 6 ++-- .../qemuxml2xml-seclabel-dynamic-out.xml | 1 + 4 files changed, 24 insertions(+), 23 deletions(-)
Hmm, the domain.rng states that attribute model is <text/> rather than limiting it to a <choice> between selinux/apparmor (as currently supported) or even <choice> selinux/apparmor/none (per your enum in patch 5/7, as used in patch 7/7). That might be an independently useful thing to clean up, to tighten the .rng to match the possible valid values. And maybe 5/7 has a use after all (but with cleanups to avoid issues with model='default' and to omit model='none'). Given your commit message, I see what you are getting at - the current xml parsing does not reject <seclabel type='dynamic' model='bogus'> for a defined but inactive domain. At which point this code motion makes sense, to always validate model to match the list of allowed enum values. But it could use domain.rng tightening, and docs/formatdomain.html.in doesn't even mention seclabel, so we'd probably want that in first. And then there's the question of whether to go with patch 5/7 as a prereq to this, or whether you should rewrite the code motion in terms of the state things were in before 5/7. I guess this means it is worth a v2, if you think it is still worth keeping this patch in the series. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 12, 2011 at 12:23:02PM -0500, Cole Robinson wrote:
This will help facilitate disabling seclabel for an individual VM. One functional change is that the user can now hardcode type='dynamic', but there was no good reason to deny it anyways.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 34 ++++++++++---------- src/security/security_apparmor.c | 6 ++-- src/security/security_selinux.c | 6 ++-- .../qemuxml2xml-seclabel-dynamic-out.xml | 1 + 4 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 077a396..e5b89a2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4238,28 +4238,28 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, goto error; }
+ p = virXPathStringLimit("string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + + def->seclabel.model = virDomainSeclabelModelTypeFromString(p); + if (def->seclabel.model < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown security model '%s'"), p); + VIR_FREE(p); + goto error; + } + VIR_FREE(p); + /* Only parse details, if using static labels, or * if the 'live' VM XML is requested */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags & VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit("string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("missing security model")); - goto error; - } - - def->seclabel.model = virDomainSeclabelModelTypeFromString(p); - if (def->seclabel.model < 0) { - virDomainReportError(VIR_ERR_XML_ERROR, - _("unknown security model '%s'"), p); - VIR_FREE(p); - goto error; - } - VIR_FREE(p); - p = virXPathStringLimit("string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p == NULL) {
This changes semantics. If the seclabel type is dynamic, then we want to ignore any kind of 'model' at all, because the model should automatically become whatever is current active driver. This ensures that if you change security drivers, then all dynamic VMs will automatically use the new driver and not be stuck with the model of the old driver. Since, we're not supporting per-VM disabled models, I don't think we need this patch anyway. Regards, Daniel
Make the SecurityManager explicitly handle the case when seclabel model='none'. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_manager.c | 90 +++++++++++++------- .../qemuxml2xml-seclabel-model-none-in.xml | 21 +++++ .../qemuxml2xml-seclabel-model-none-out.xml | 21 +++++ tests/qemuxml2xmltest.c | 1 + 4 files changed, 101 insertions(+), 32 deletions(-) create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-out.xml diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 66cffb5..9f98886 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -122,6 +122,16 @@ void virSecurityManagerFree(virSecurityManagerPtr mgr) VIR_FREE(mgr); } +static virSecurityDriverPtr +virSecurityManagerGetDriver(virSecurityManagerPtr mgr, + virDomainDefPtr def) +{ + if (def->seclabel.model == VIR_DOMAIN_SECLABEL_MODEL_NONE) + return virSecurityDriverLookup("none"); + + return mgr->drv; +} + const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) { @@ -151,8 +161,9 @@ int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - if (mgr->drv->domainRestoreSecurityImageLabel) - return mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, disk); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainRestoreSecurityImageLabel) + return drv->domainRestoreSecurityImageLabel(mgr, vm, disk); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -161,8 +172,9 @@ int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { - if (mgr->drv->domainSetSecuritySocketLabel) - return mgr->drv->domainSetSecuritySocketLabel(mgr, vm); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainSetSecuritySocketLabel) + return drv->domainSetSecuritySocketLabel(mgr, vm); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -171,8 +183,9 @@ int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, int virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { - if (mgr->drv->domainClearSecuritySocketLabel) - return mgr->drv->domainClearSecuritySocketLabel(mgr, vm); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainClearSecuritySocketLabel) + return drv->domainClearSecuritySocketLabel(mgr, vm); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -182,8 +195,9 @@ int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - if (mgr->drv->domainSetSecurityImageLabel) - return mgr->drv->domainSetSecurityImageLabel(mgr, vm, disk); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainSetSecurityImageLabel) + return drv->domainSetSecurityImageLabel(mgr, vm, disk); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -193,8 +207,9 @@ int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, virDomainHostdevDefPtr dev) { - if (mgr->drv->domainRestoreSecurityHostdevLabel) - return mgr->drv->domainRestoreSecurityHostdevLabel(mgr, vm, dev); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainRestoreSecurityHostdevLabel) + return drv->domainRestoreSecurityHostdevLabel(mgr, vm, dev); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -204,8 +219,9 @@ int virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, virDomainHostdevDefPtr dev) { - if (mgr->drv->domainSetSecurityHostdevLabel) - return mgr->drv->domainSetSecurityHostdevLabel(mgr, vm, dev); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainSetSecurityHostdevLabel) + return drv->domainSetSecurityHostdevLabel(mgr, vm, dev); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -215,8 +231,9 @@ int virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, const char *savefile) { - if (mgr->drv->domainSetSavedStateLabel) - return mgr->drv->domainSetSavedStateLabel(mgr, vm, savefile); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainSetSavedStateLabel) + return drv->domainSetSavedStateLabel(mgr, vm, savefile); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -226,8 +243,9 @@ int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, const char *savefile) { - if (mgr->drv->domainRestoreSavedStateLabel) - return mgr->drv->domainRestoreSavedStateLabel(mgr, vm, savefile); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainRestoreSavedStateLabel) + return drv->domainRestoreSavedStateLabel(mgr, vm, savefile); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -236,8 +254,9 @@ int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { - if (mgr->drv->domainGenSecurityLabel) - return mgr->drv->domainGenSecurityLabel(mgr, vm); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainGenSecurityLabel) + return drv->domainGenSecurityLabel(mgr, vm); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -246,8 +265,9 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { - if (mgr->drv->domainReserveSecurityLabel) - return mgr->drv->domainReserveSecurityLabel(mgr, vm); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainReserveSecurityLabel) + return drv->domainReserveSecurityLabel(mgr, vm); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -256,8 +276,9 @@ int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, int virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { - if (mgr->drv->domainReleaseSecurityLabel) - return mgr->drv->domainReleaseSecurityLabel(mgr, vm); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainReleaseSecurityLabel) + return drv->domainReleaseSecurityLabel(mgr, vm); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -267,8 +288,9 @@ int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, const char *stdin_path) { - if (mgr->drv->domainSetSecurityAllLabel) - return mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainSetSecurityAllLabel) + return drv->domainSetSecurityAllLabel(mgr, vm, stdin_path); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -278,8 +300,9 @@ int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, int migrated) { - if (mgr->drv->domainRestoreSecurityAllLabel) - return mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainRestoreSecurityAllLabel) + return drv->domainRestoreSecurityAllLabel(mgr, vm, migrated); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -289,8 +312,9 @@ int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, virSecurityLabelPtr sec) { - if (mgr->drv->domainGetSecurityProcessLabel) - return mgr->drv->domainGetSecurityProcessLabel(mgr, vm, sec); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainGetSecurityProcessLabel) + return drv->domainGetSecurityProcessLabel(mgr, vm, sec); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -299,8 +323,9 @@ int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm) { - if (mgr->drv->domainSetSecurityProcessLabel) - return mgr->drv->domainSetSecurityProcessLabel(mgr, vm); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, vm->def); + if (drv->domainSetSecurityProcessLabel) + return drv->domainSetSecurityProcessLabel(mgr, vm); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; @@ -309,8 +334,9 @@ int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, int virSecurityManagerVerify(virSecurityManagerPtr mgr, virDomainDefPtr def) { - if (mgr->drv->domainSecurityVerify) - return mgr->drv->domainSecurityVerify(mgr, def); + virSecurityDriverPtr drv = virSecurityManagerGetDriver(mgr, def); + if (drv->domainSecurityVerify) + return drv->domainSecurityVerify(mgr, def); virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; diff --git a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-in.xml b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-in.xml new file mode 100644 index 0000000..2b3d40b --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-in.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='dynamic' model='none'/> +</domain> diff --git a/tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-out.xml b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-out.xml new file mode 100644 index 0000000..2b3d40b --- /dev/null +++ b/tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-out.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='dynamic' model='none'/> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2af7494..8c08ee6 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -200,6 +200,7 @@ mymain(int argc, char **argv) input_folder_fmt = (char *) XML2XMLIN_FMT; DO_TEST_DIFFERENT("seclabel-dynamic"); DO_TEST_DIFFERENT("seclabel-static"); + DO_TEST_DIFFERENT("seclabel-model-none"); virCapabilitiesFree(driver.caps); -- 1.7.3.2
On 01/12/2011 10:23 AM, Cole Robinson wrote:
Make the SecurityManager explicitly handle the case when seclabel model='none'.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_manager.c | 90 +++++++++++++------- .../qemuxml2xml-seclabel-model-none-in.xml | 21 +++++ .../qemuxml2xml-seclabel-model-none-out.xml | 21 +++++ tests/qemuxml2xmltest.c | 1 + 4 files changed, 101 insertions(+), 32 deletions(-) create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-out.xml
I agree with Daniel's NACK to this patch - when security is enabled globally, allowing just one rogue domain can invalidate all others. And when security is not enabled, <seclabel> is an illusion not aided by an XML marking. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jan 12, 2011 at 12:23:03PM -0500, Cole Robinson wrote:
Make the SecurityManager explicitly handle the case when seclabel model='none'.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_manager.c | 90 +++++++++++++------- .../qemuxml2xml-seclabel-model-none-in.xml | 21 +++++ .../qemuxml2xml-seclabel-model-none-out.xml | 21 +++++ tests/qemuxml2xmltest.c | 1 + 4 files changed, 101 insertions(+), 32 deletions(-) create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-in.xml create mode 100644 tests/qemuxml2xmldata/qemuxml2xml-seclabel-model-none-out.xml
As previously discussed, we should't take this patch Daniel
On Wed, Jan 12, 2011 at 12:22:56PM -0500, Cole Robinson wrote:
Enabling a security driver in qemu.conf is currently all or nothing. The option to disable security on a per VM basis can be a useful debugging tool or work around for frustrated users.
Patches 1-3 and 5-6 are prep and cleanup work. Patch 4 fixes an easily triggerable segfault when defining a domain in qemu. Patch 7 is the actual feature.
Hmm, I can understand the motivation for wanting to allow users to disable security per VM. From the POV of a security person it is a bad idea to allow this capability to be used by default since running one single unconfined VM compromises the entire security model. As a host admin, we need to be able to enforce that every single VM launched is always running with security model active, and not allow libvirt admins to override that decision at all. Thus a default libvirt install would have to forbid any attempt to run a VM with a secmodel=none, and reject with an error. It would have to require a host level configuration change to allow running VMs without a secmodel. Unfortunately once you require this you might as well just be changing the existing config param in qemu.conf for libvirtd as a whole. If a user is having trouble, and needs to debug then I think it is best to just 'setenforce 0' and do the debugging. NB, some of your patches in this series are useful regardless, but I don't think we should allow a tunable to turn off security per VM. Regards, Daniel
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake