9:14 a.m.
This patch series does some work on te security drivers, and the QEMU code
for managing DAC permissions on files.
The core goal is to turn the QEMU driver DAC file management code into a
security driver. Instead of QEMU calling into the SELinux/AppArmour drivers
directly, a stacked driver module is introduced. This delegates all operations
to first the QEMU DAC driver, and then the main SELinux/AppArmour driver.
The end result is that all the permissions management code is removed from
the QEMU driver, and we're left with just simple security driver calls.
In the process of this a number of flaws in the current hotplug code were
found, and code was generally tidied up with a view to making it easier to
manage.
Finally, we add the ability to turn off the QEMU DAC file managment code,
and also deal gracefully with failures to change ownership (eg on NFS with
root squash, or readonly FS).
9:14 a.m.
New subject: [libvirt] [PATCH 01/12] Make security drivers responsible for checking dynamic vs static labelling
The QEMU driver is doing 90% of the calls to check for static vs
dynamic labelling. Except it is forgetting todo so in many places,
in particular hotplug is mistakenly assigning disk labels. Move
all this logic into the security drivers themselves, so the HV
drivers don't have to think about it.
* src/security/security_driver.h: Add virDomainObjPtr parameter
to virSecurityDomainRestoreHostdevLabel and to
virSecurityDomainRestoreSavedStateLabel
* src/security/security_selinux.c, src/security/security_apparmor.c:
Add explicit checks for VIR_DOMAIN_SECLABEL_STATIC and skip all
chcon() code in those cases
* src/qemu/qemu_driver.c: Remove all checks for VIR_DOMAIN_SECLABEL_STATIC
or VIR_DOMAIN_SECLABEL_DYNAMIC. Add missing checks for possibly NULL
driver entry points.
---
src/qemu/qemu_driver.c | 40 ++++++++++++-------
src/security/security_apparmor.c | 46 +++++++++++++++-------
src/security/security_driver.h | 2 +
src/security/security_selinux.c | 78 ++++++++++++++++++++++++++++----------
4 files changed, 117 insertions(+), 49 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2d80774..446f6ff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -855,8 +855,7 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED,
void *opaq
goto error;
}
- if (obj->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
- driver->securityDriver &&
+ if (driver->securityDriver &&
driver->securityDriver->domainReserveSecurityLabel &&
driver->securityDriver->domainReserveSecurityLabel(NULL, obj) < 0)
goto error;
@@ -2441,11 +2440,14 @@ cleanup:
static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver,
virDomainObjPtr vm)
{
- if (vm->def->seclabel.label != NULL)
- if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityLabel)
- return driver->securityDriver->domainSetSecurityLabel(conn,
driver->securityDriver,
- vm);
- return 0;
+ int rc = 0;
+
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityLabel &&
+ driver->securityDriver->domainSetSecurityLabel(conn,
driver->securityDriver, vm) < 0)
+ rc = -1;
+
+ return rc;
}
@@ -2758,8 +2760,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
/* If you are using a SecurityDriver with dynamic labelling,
then generate a security label for isolation */
- if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
- driver->securityDriver &&
+ if (driver->securityDriver &&
driver->securityDriver->domainGenSecurityLabel &&
driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
return -1;
@@ -3054,7 +3055,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
virKillProcess(vm->pid, SIGKILL);
/* Reset Security Labels */
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityLabel)
driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
/* Clear out dynamically assigned labels */
@@ -4166,7 +4168,7 @@ static int qemudDomainSave(virDomainPtr dom,
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
- driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path) ==
-1)
+ driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
goto endjob;
ret = 0;
@@ -4296,7 +4298,7 @@ static int qemudDomainCoreDump(virDomainPtr dom,
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
- driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path) ==
-1)
+ driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
goto endjob;
endjob:
@@ -5871,6 +5873,7 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0)
return -1;
if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
return -1;
@@ -5947,7 +5950,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
switch (dev->data.disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -5957,7 +5961,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -6347,6 +6352,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,
}
if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
VIR_WARN0("Failed to restore device labelling");
@@ -6392,7 +6398,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev);
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel)
driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
VIR_WARN0("Fail to restore disk device ownership");
@@ -6409,6 +6416,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
}
} else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityHostdevLabel)
+ driver->securityDriver->domainRestoreSecurityHostdevLabel(dom->conn,
vm, dev->data.hostdev);
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("This type of device cannot be hot
unplugged"));
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 5844768..0f9ff95 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -327,6 +327,9 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
int rc = -1;
char *profile_name = NULL;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if ((vm->def->seclabel.label) ||
(vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
@@ -425,7 +428,7 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int rc = 0;
- if (secdef->imagelabel) {
+ if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
if ((rc = remove_profile(secdef->label)) != 0) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("could not remove profile for
\'%s\'"),
@@ -486,21 +489,23 @@ AppArmorRestoreSecurityImageLabel(virConnectPtr conn,
int rc = -1;
char *profile_name = NULL;
- if (secdef->imagelabel) {
- if ((profile_name = get_profile_name(conn, vm)) == NULL)
- return rc;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
- /* Update the profile only if it is loaded */
- if (profile_loaded(secdef->imagelabel) >= 0) {
- if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
- virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _("cannot update AppArmor profile "
- "\'%s\'"),
- secdef->imagelabel);
- goto clean;
- }
+ if ((profile_name = get_profile_name(conn, vm)) == NULL)
+ return rc;
+
+ /* Update the profile only if it is loaded */
+ if (profile_loaded(secdef->imagelabel) >= 0) {
+ if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
+ virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("cannot update AppArmor profile "
+ "\'%s\'"),
+ secdef->imagelabel);
+ goto clean;
}
}
+
rc = 0;
clean:
VIR_FREE(profile_name);
@@ -517,6 +522,9 @@ AppArmorSetSecurityImageLabel(virConnectPtr conn,
int rc = -1;
char *profile_name;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (!disk->src)
return 0;
@@ -576,19 +584,29 @@ AppArmorReserveSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
static int
AppArmorSetSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
- virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* TODO: call load_profile with an update vm->def */
return 0;
}
static int
AppArmorRestoreSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* TODO: call load_profile (needs virDomainObjPtr vm) */
return 0;
}
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 5514962..97c9002 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -38,6 +38,7 @@ typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
virDomainObjPtr vm,
virDomainDiskDefPtr disk);
typedef int (*virSecurityDomainRestoreHostdevLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev);
typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn,
virDomainObjPtr vm,
@@ -46,6 +47,7 @@ typedef int (*virSecurityDomainSetSavedStateLabel) (virConnectPtr conn,
virDomainObjPtr vm,
const char *savefile);
typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
const char *savefile);
typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
virDomainObjPtr sec);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index cb585ed..c48f4c8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -165,6 +165,9 @@ SELinuxGenSecurityLabel(virConnectPtr conn,
int c1 = 0;
int c2 = 0;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (vm->def->seclabel.label ||
vm->def->seclabel.model ||
vm->def->seclabel.imagelabel) {
@@ -225,6 +228,9 @@ SELinuxReserveSecurityLabel(virConnectPtr conn,
context_t ctx = NULL;
const char *mcs;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (getpidcon(vm->pid, &pctx) == -1) {
virReportSystemError(conn, errno,
_("unable to get PID %d security context"),
vm->pid);
@@ -376,9 +382,14 @@ err:
static int
SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
- virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* Don't restore labels on readoly/shared disks, because
* other VMs may still be accessing these
* Alternatively we could iterate over all running
@@ -405,6 +416,9 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
const char *path;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (!disk->src)
return 0;
@@ -474,8 +488,12 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
virDomainHostdevDefPtr dev)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int ret = -1;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
return 0;
@@ -541,11 +559,16 @@ SELinuxRestoreSecurityUSBLabel(virConnectPtr conn,
static int
SELinuxRestoreSecurityHostdevLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int ret = -1;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
return 0;
@@ -601,25 +624,28 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
VIR_DEBUG("Restoring security label on %s", vm->def->name);
- if (secdef->imagelabel) {
- for (i = 0 ; i < vm->def->nhostdevs ; i++) {
- if (SELinuxRestoreSecurityHostdevLabel(conn, vm->def->hostdevs[i]) <
0)
- rc = -1;
- }
- for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (SELinuxRestoreSecurityImageLabel(conn, vm,
- vm->def->disks[i]) < 0)
- rc = -1;
- }
- VIR_FREE(secdef->model);
- VIR_FREE(secdef->label);
- context_t con = context_new(secdef->imagelabel);
- if (con) {
- mcsRemove(context_range_get(con));
- context_free(con);
- }
- VIR_FREE(secdef->imagelabel);
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
+ for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+ if (SELinuxRestoreSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) <
0)
+ rc = -1;
}
+ for (i = 0 ; i < vm->def->ndisks ; i++) {
+ if (SELinuxRestoreSecurityImageLabel(conn, vm,
+ vm->def->disks[i]) < 0)
+ rc = -1;
+ }
+ context_t con = context_new(secdef->label);
+ if (con) {
+ mcsRemove(context_range_get(con));
+ context_free(con);
+ }
+
+ VIR_FREE(secdef->model);
+ VIR_FREE(secdef->label);
+ VIR_FREE(secdef->imagelabel);
+
return rc;
}
@@ -631,14 +657,23 @@ SELinuxSetSavedStateLabel(virConnectPtr conn,
{
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
return SELinuxSetFilecon(conn, savefile, secdef->imagelabel);
}
static int
SELinuxRestoreSavedStateLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
const char *savefile)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
return SELinuxRestoreSecurityFileLabel(conn, savefile);
}
@@ -666,6 +701,9 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int i;
+ if (vm->def->seclabel.label == NULL)
+ return 0;
+
if (!STREQ(drv->name, secdef->model)) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("security label driver mismatch: "
@@ -684,7 +722,7 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
return -1;
}
- if (secdef->imagelabel) {
+ if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
for (i = 0 ; i < vm->def->ndisks ; i++) {
/* XXX fixme - we need to recursively label the entriy tree :-( */
if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
--
1.6.5.2
9:43 a.m.
New subject: [libvirt] [PATCH 01/12] Make security drivers responsible for checking dynamic vs static labelling
On Wed, Jan 20, 2010 at 03:14:58PM +0000, Daniel P. Berrange wrote:
The QEMU driver is doing 90% of the calls to check for static vs
dynamic labelling. Except it is forgetting todo so in many places,
in particular hotplug is mistakenly assigning disk labels. Move
all this logic into the security drivers themselves, so the HV
drivers don't have to think about it.
* src/security/security_driver.h: Add virDomainObjPtr parameter
to virSecurityDomainRestoreHostdevLabel and to
virSecurityDomainRestoreSavedStateLabel
* src/security/security_selinux.c, src/security/security_apparmor.c:
Add explicit checks for VIR_DOMAIN_SECLABEL_STATIC and skip all
chcon() code in those cases
* src/qemu/qemu_driver.c: Remove all checks for VIR_DOMAIN_SECLABEL_STATIC
or VIR_DOMAIN_SECLABEL_DYNAMIC. Add missing checks for possibly NULL
driver entry points.
---
src/qemu/qemu_driver.c | 40 ++++++++++++-------
src/security/security_apparmor.c | 46 +++++++++++++++-------
src/security/security_driver.h | 2 +
src/security/security_selinux.c | 78 ++++++++++++++++++++++++++++----------
4 files changed, 117 insertions(+), 49 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2d80774..446f6ff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -855,8 +855,7 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED,
void *opaq
goto error;
}
- if (obj->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
- driver->securityDriver &&
+ if (driver->securityDriver &&
driver->securityDriver->domainReserveSecurityLabel &&
driver->securityDriver->domainReserveSecurityLabel(NULL, obj) < 0)
goto error;
@@ -2441,11 +2440,14 @@ cleanup:
static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver,
virDomainObjPtr vm)
{
- if (vm->def->seclabel.label != NULL)
- if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityLabel)
- return driver->securityDriver->domainSetSecurityLabel(conn,
driver->securityDriver,
- vm);
- return 0;
+ int rc = 0;
+
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityLabel &&
+ driver->securityDriver->domainSetSecurityLabel(conn,
driver->securityDriver, vm) < 0)
+ rc = -1;
+
+ return rc;
}
@@ -2758,8 +2760,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
/* If you are using a SecurityDriver with dynamic labelling,
then generate a security label for isolation */
- if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
- driver->securityDriver &&
+ if (driver->securityDriver &&
driver->securityDriver->domainGenSecurityLabel &&
driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
return -1;
@@ -3054,7 +3055,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
virKillProcess(vm->pid, SIGKILL);
/* Reset Security Labels */
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityLabel)
driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
/* Clear out dynamically assigned labels */
@@ -4166,7 +4168,7 @@ static int qemudDomainSave(virDomainPtr dom,
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
- driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path)
== -1)
+ driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
goto endjob;
ret = 0;
@@ -4296,7 +4298,7 @@ static int qemudDomainCoreDump(virDomainPtr dom,
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
- driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path)
== -1)
+ driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
goto endjob;
endjob:
@@ -5871,6 +5873,7 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0)
return -1;
if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
return -1;
@@ -5947,7 +5950,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
switch (dev->data.disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -5957,7 +5961,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -6347,6 +6352,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,
}
if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
VIR_WARN0("Failed to restore device labelling");
@@ -6392,7 +6398,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev);
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel)
driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
VIR_WARN0("Fail to restore disk device ownership");
@@ -6409,6 +6416,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
}
} else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityHostdevLabel)
+
driver->securityDriver->domainRestoreSecurityHostdevLabel(dom->conn, vm,
dev->data.hostdev);
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("This type of device cannot be hot
unplugged"));
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 5844768..0f9ff95 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -327,6 +327,9 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
int rc = -1;
char *profile_name = NULL;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if ((vm->def->seclabel.label) ||
(vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
@@ -425,7 +428,7 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int rc = 0;
- if (secdef->imagelabel) {
+ if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
if ((rc = remove_profile(secdef->label)) != 0) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("could not remove profile for
\'%s\'"),
@@ -486,21 +489,23 @@ AppArmorRestoreSecurityImageLabel(virConnectPtr conn,
int rc = -1;
char *profile_name = NULL;
- if (secdef->imagelabel) {
- if ((profile_name = get_profile_name(conn, vm)) == NULL)
- return rc;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
- /* Update the profile only if it is loaded */
- if (profile_loaded(secdef->imagelabel) >= 0) {
- if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
- virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _("cannot update AppArmor profile "
- "\'%s\'"),
- secdef->imagelabel);
- goto clean;
- }
+ if ((profile_name = get_profile_name(conn, vm)) == NULL)
+ return rc;
+
+ /* Update the profile only if it is loaded */
+ if (profile_loaded(secdef->imagelabel) >= 0) {
+ if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
+ virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("cannot update AppArmor profile "
+ "\'%s\'"),
+ secdef->imagelabel);
+ goto clean;
}
}
+
rc = 0;
clean:
VIR_FREE(profile_name);
@@ -517,6 +522,9 @@ AppArmorSetSecurityImageLabel(virConnectPtr conn,
int rc = -1;
char *profile_name;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (!disk->src)
return 0;
@@ -576,19 +584,29 @@ AppArmorReserveSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
static int
AppArmorSetSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
- virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* TODO: call load_profile with an update vm->def */
return 0;
}
static int
AppArmorRestoreSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* TODO: call load_profile (needs virDomainObjPtr vm) */
return 0;
}
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 5514962..97c9002 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -38,6 +38,7 @@ typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
virDomainObjPtr vm,
virDomainDiskDefPtr disk);
typedef int (*virSecurityDomainRestoreHostdevLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev);
typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn,
virDomainObjPtr vm,
@@ -46,6 +47,7 @@ typedef int (*virSecurityDomainSetSavedStateLabel) (virConnectPtr
conn,
virDomainObjPtr vm,
const char *savefile);
typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
const char *savefile);
typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
virDomainObjPtr sec);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index cb585ed..c48f4c8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -165,6 +165,9 @@ SELinuxGenSecurityLabel(virConnectPtr conn,
int c1 = 0;
int c2 = 0;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (vm->def->seclabel.label ||
vm->def->seclabel.model ||
vm->def->seclabel.imagelabel) {
@@ -225,6 +228,9 @@ SELinuxReserveSecurityLabel(virConnectPtr conn,
context_t ctx = NULL;
const char *mcs;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (getpidcon(vm->pid, &pctx) == -1) {
virReportSystemError(conn, errno,
_("unable to get PID %d security context"),
vm->pid);
@@ -376,9 +382,14 @@ err:
static int
SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
- virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* Don't restore labels on readoly/shared disks, because
* other VMs may still be accessing these
* Alternatively we could iterate over all running
@@ -405,6 +416,9 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
const char *path;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (!disk->src)
return 0;
@@ -474,8 +488,12 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
virDomainHostdevDefPtr dev)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int ret = -1;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
return 0;
@@ -541,11 +559,16 @@ SELinuxRestoreSecurityUSBLabel(virConnectPtr conn,
static int
SELinuxRestoreSecurityHostdevLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int ret = -1;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
return 0;
@@ -601,25 +624,28 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
VIR_DEBUG("Restoring security label on %s", vm->def->name);
- if (secdef->imagelabel) {
- for (i = 0 ; i < vm->def->nhostdevs ; i++) {
- if (SELinuxRestoreSecurityHostdevLabel(conn, vm->def->hostdevs[i])
< 0)
- rc = -1;
- }
- for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (SELinuxRestoreSecurityImageLabel(conn, vm,
- vm->def->disks[i]) < 0)
- rc = -1;
- }
- VIR_FREE(secdef->model);
- VIR_FREE(secdef->label);
- context_t con = context_new(secdef->imagelabel);
- if (con) {
- mcsRemove(context_range_get(con));
- context_free(con);
- }
- VIR_FREE(secdef->imagelabel);
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
+ for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+ if (SELinuxRestoreSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i])
< 0)
+ rc = -1;
}
+ for (i = 0 ; i < vm->def->ndisks ; i++) {
+ if (SELinuxRestoreSecurityImageLabel(conn, vm,
+ vm->def->disks[i]) < 0)
+ rc = -1;
+ }
+ context_t con = context_new(secdef->label);
+ if (con) {
+ mcsRemove(context_range_get(con));
+ context_free(con);
+ }
+
+ VIR_FREE(secdef->model);
+ VIR_FREE(secdef->label);
+ VIR_FREE(secdef->imagelabel);
+
return rc;
}
@@ -631,14 +657,23 @@ SELinuxSetSavedStateLabel(virConnectPtr conn,
{
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
return SELinuxSetFilecon(conn, savefile, secdef->imagelabel);
}
static int
SELinuxRestoreSavedStateLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
const char *savefile)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
return SELinuxRestoreSecurityFileLabel(conn, savefile);
}
@@ -666,6 +701,9 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int i;
+ if (vm->def->seclabel.label == NULL)
+ return 0;
+
if (!STREQ(drv->name, secdef->model)) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("security label driver mismatch: "
@@ -684,7 +722,7 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
return -1;
}
- if (secdef->imagelabel) {
+ if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
for (i = 0 ; i < vm->def->ndisks ; i++) {
/* XXX fixme - we need to recursively label the entriy tree :-( */
if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
ACK, also adds some missing check for some driver->securityDriver
entry point. My only concern is dereferencing vm->def->seclabel without
any check for NULL, I would feel better if that extra safety belt was
added,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:59 a.m.
New subject: [libvirt] [PATCH 01/12] Make security drivers responsible for checking dynamic vs static labelling
On Wed, Jan 20, 2010 at 03:14:58PM +0000, Daniel P. Berrange wrote:
The QEMU driver is doing 90% of the calls to check for static vs
dynamic labelling. Except it is forgetting todo so in many places,
in particular hotplug is mistakenly assigning disk labels. Move
all this logic into the security drivers themselves, so the HV
drivers don't have to think about it.
* src/security/security_driver.h: Add virDomainObjPtr parameter
to virSecurityDomainRestoreHostdevLabel and to
virSecurityDomainRestoreSavedStateLabel
* src/security/security_selinux.c, src/security/security_apparmor.c:
Add explicit checks for VIR_DOMAIN_SECLABEL_STATIC and skip all
chcon() code in those cases
* src/qemu/qemu_driver.c: Remove all checks for VIR_DOMAIN_SECLABEL_STATIC
or VIR_DOMAIN_SECLABEL_DYNAMIC. Add missing checks for possibly NULL
driver entry points.
---
src/qemu/qemu_driver.c | 40 ++++++++++++-------
src/security/security_apparmor.c | 46 +++++++++++++++-------
src/security/security_driver.h | 2 +
src/security/security_selinux.c | 78 ++++++++++++++++++++++++++++----------
4 files changed, 117 insertions(+), 49 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2d80774..446f6ff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -855,8 +855,7 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED,
void *opaq
goto error;
}
- if (obj->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
- driver->securityDriver &&
+ if (driver->securityDriver &&
driver->securityDriver->domainReserveSecurityLabel &&
driver->securityDriver->domainReserveSecurityLabel(NULL, obj) < 0)
goto error;
@@ -2441,11 +2440,14 @@ cleanup:
static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver,
virDomainObjPtr vm)
{
- if (vm->def->seclabel.label != NULL)
- if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityLabel)
- return driver->securityDriver->domainSetSecurityLabel(conn,
driver->securityDriver,
- vm);
- return 0;
+ int rc = 0;
+
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityLabel &&
+ driver->securityDriver->domainSetSecurityLabel(conn,
driver->securityDriver, vm) < 0)
+ rc = -1;
+
+ return rc;
}
@@ -2758,8 +2760,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
/* If you are using a SecurityDriver with dynamic labelling,
then generate a security label for isolation */
- if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
- driver->securityDriver &&
+ if (driver->securityDriver &&
driver->securityDriver->domainGenSecurityLabel &&
driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
return -1;
@@ -3054,7 +3055,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
virKillProcess(vm->pid, SIGKILL);
/* Reset Security Labels */
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityLabel)
driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
/* Clear out dynamically assigned labels */
@@ -4166,7 +4168,7 @@ static int qemudDomainSave(virDomainPtr dom,
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
- driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path)
== -1)
+ driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
goto endjob;
ret = 0;
@@ -4296,7 +4298,7 @@ static int qemudDomainCoreDump(virDomainPtr dom,
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
- driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path)
== -1)
+ driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
goto endjob;
endjob:
@@ -5871,6 +5873,7 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0)
return -1;
if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
return -1;
@@ -5947,7 +5950,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
switch (dev->data.disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -5957,7 +5961,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -6347,6 +6352,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,
}
if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
VIR_WARN0("Failed to restore device labelling");
@@ -6392,7 +6398,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev);
- if (driver->securityDriver)
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel)
driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn,
vm, dev->data.disk);
if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
VIR_WARN0("Fail to restore disk device ownership");
@@ -6409,6 +6416,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
}
} else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityHostdevLabel)
+
driver->securityDriver->domainRestoreSecurityHostdevLabel(dom->conn, vm,
dev->data.hostdev);
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("This type of device cannot be hot
unplugged"));
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 5844768..0f9ff95 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -327,6 +327,9 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
int rc = -1;
char *profile_name = NULL;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if ((vm->def->seclabel.label) ||
(vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
@@ -425,7 +428,7 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int rc = 0;
- if (secdef->imagelabel) {
+ if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
if ((rc = remove_profile(secdef->label)) != 0) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("could not remove profile for
\'%s\'"),
@@ -486,21 +489,23 @@ AppArmorRestoreSecurityImageLabel(virConnectPtr conn,
int rc = -1;
char *profile_name = NULL;
- if (secdef->imagelabel) {
- if ((profile_name = get_profile_name(conn, vm)) == NULL)
- return rc;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
- /* Update the profile only if it is loaded */
- if (profile_loaded(secdef->imagelabel) >= 0) {
- if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
- virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _("cannot update AppArmor profile "
- "\'%s\'"),
- secdef->imagelabel);
- goto clean;
- }
+ if ((profile_name = get_profile_name(conn, vm)) == NULL)
+ return rc;
+
+ /* Update the profile only if it is loaded */
+ if (profile_loaded(secdef->imagelabel) >= 0) {
+ if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
+ virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("cannot update AppArmor profile "
+ "\'%s\'"),
+ secdef->imagelabel);
+ goto clean;
}
}
+
rc = 0;
clean:
VIR_FREE(profile_name);
@@ -517,6 +522,9 @@ AppArmorSetSecurityImageLabel(virConnectPtr conn,
int rc = -1;
char *profile_name;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (!disk->src)
return 0;
@@ -576,19 +584,29 @@ AppArmorReserveSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
static int
AppArmorSetSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
- virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* TODO: call load_profile with an update vm->def */
return 0;
}
static int
AppArmorRestoreSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* TODO: call load_profile (needs virDomainObjPtr vm) */
return 0;
}
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 5514962..97c9002 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -38,6 +38,7 @@ typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
virDomainObjPtr vm,
virDomainDiskDefPtr disk);
typedef int (*virSecurityDomainRestoreHostdevLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev);
typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn,
virDomainObjPtr vm,
@@ -46,6 +47,7 @@ typedef int (*virSecurityDomainSetSavedStateLabel) (virConnectPtr
conn,
virDomainObjPtr vm,
const char *savefile);
typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
const char *savefile);
typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
virDomainObjPtr sec);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index cb585ed..c48f4c8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -165,6 +165,9 @@ SELinuxGenSecurityLabel(virConnectPtr conn,
int c1 = 0;
int c2 = 0;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (vm->def->seclabel.label ||
vm->def->seclabel.model ||
vm->def->seclabel.imagelabel) {
@@ -225,6 +228,9 @@ SELinuxReserveSecurityLabel(virConnectPtr conn,
context_t ctx = NULL;
const char *mcs;
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (getpidcon(vm->pid, &pctx) == -1) {
virReportSystemError(conn, errno,
_("unable to get PID %d security context"),
vm->pid);
@@ -376,9 +382,14 @@ err:
static int
SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
- virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
/* Don't restore labels on readoly/shared disks, because
* other VMs may still be accessing these
* Alternatively we could iterate over all running
@@ -405,6 +416,9 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
const char *path;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (!disk->src)
return 0;
@@ -474,8 +488,12 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
virDomainHostdevDefPtr dev)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int ret = -1;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
return 0;
@@ -541,11 +559,16 @@ SELinuxRestoreSecurityUSBLabel(virConnectPtr conn,
static int
SELinuxRestoreSecurityHostdevLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int ret = -1;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
return 0;
@@ -601,25 +624,28 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
VIR_DEBUG("Restoring security label on %s", vm->def->name);
- if (secdef->imagelabel) {
- for (i = 0 ; i < vm->def->nhostdevs ; i++) {
- if (SELinuxRestoreSecurityHostdevLabel(conn, vm->def->hostdevs[i])
< 0)
- rc = -1;
- }
- for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (SELinuxRestoreSecurityImageLabel(conn, vm,
- vm->def->disks[i]) < 0)
- rc = -1;
- }
- VIR_FREE(secdef->model);
- VIR_FREE(secdef->label);
- context_t con = context_new(secdef->imagelabel);
- if (con) {
- mcsRemove(context_range_get(con));
- context_free(con);
- }
- VIR_FREE(secdef->imagelabel);
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
+ for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+ if (SELinuxRestoreSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i])
< 0)
+ rc = -1;
}
+ for (i = 0 ; i < vm->def->ndisks ; i++) {
+ if (SELinuxRestoreSecurityImageLabel(conn, vm,
+ vm->def->disks[i]) < 0)
+ rc = -1;
+ }
+ context_t con = context_new(secdef->label);
+ if (con) {
+ mcsRemove(context_range_get(con));
+ context_free(con);
+ }
+
+ VIR_FREE(secdef->model);
+ VIR_FREE(secdef->label);
+ VIR_FREE(secdef->imagelabel);
+
return rc;
}
@@ -631,14 +657,23 @@ SELinuxSetSavedStateLabel(virConnectPtr conn,
{
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
return SELinuxSetFilecon(conn, savefile, secdef->imagelabel);
}
static int
SELinuxRestoreSavedStateLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
const char *savefile)
{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
return SELinuxRestoreSecurityFileLabel(conn, savefile);
}
@@ -666,6 +701,9 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int i;
+ if (vm->def->seclabel.label == NULL)
+ return 0;
+
if (!STREQ(drv->name, secdef->model)) {
virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
_("security label driver mismatch: "
@@ -684,7 +722,7 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
return -1;
}
- if (secdef->imagelabel) {
+ if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
for (i = 0 ; i < vm->def->ndisks ; i++) {
/* XXX fixme - we need to recursively label the entriy tree :-( */
if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
ACK, also adds some missing check for some driver->securityDriver
entry point. My only concern is dereferencing vm->def->seclabel without
any check for NULL, I would feel better if that extra safety belt was
added,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:14 a.m.
New subject: [libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver
The current security driver architecture has the following
split of logic
* domainGenSecurityLabel
Allocate the unique label for the domain about to be started
* domainGetSecurityLabel
Retrieve the current live security label for a process
* domainSetSecurityLabel
Apply the previously allocated label to the current process
Setup all disk image / device labelling
* domainRestoreSecurityLabel
Restore the original disk image / device labelling.
Release the unique label for the domain
The 'domainSetSecurityLabel' method is special because it runs
in the context of the child process between the fork + exec.
This is require in order to set the process label. It is not
required in order to label disks/devices though. Having the
disk labelling code run in the child process limits what it
can do.
In particularly libvirtd would like to remember the current
disk image label, and only change shared image labels for the
first VM to start. This requires use & update of global state
in the libvirtd daemon, and thus cannot run in the child
process context.
The solution is to split domainSetSecurityLabel into two parts,
one applies process label, and the other handles disk image
labelling. At the same time domainRestoreSecurityLabel is
similarly split, just so that it matches the style. Thus the
previous 4 methods are replaced by the following 6 new methods
* domainGenSecurityLabel
Allocate the unique label for the domain about to be started
No actual change here.
* domainReleaseSecurityLabel
Release the unique label for the domain
* domainGetSecurityProcessLabel
Retrieve the current live security label for a process
Merely renamed for clarity.
* domainSetSecurityProcessLabel
Apply the previously allocated label to the current process
* domainRestoreSecurityAllLabel
Restore the original disk image / device labelling.
* domainSetSecurityAllLabel
Setup all disk image / device labelling
The SELinux and AppArmour drivers are then updated to comply with
this new spec. Notice that the AppArmour driver was actually a
little different. It was creating its profile for the disk image
and device labels in the 'domainGenSecurityLabel' method, where as
the SELinux driver did it in 'domainSetSecurityLabel'. With the
new method split, we can have consistency, with both drivers doing
that in the domainSetSecurityAllLabel method.
NB, the AppArmour changes here haven't been compiled so may not
build.
---
src/qemu/qemu_driver.c | 21 ++++++++---
src/security/security_apparmor.c | 66 ++++++++++++++++++++++-----------
src/security/security_driver.h | 30 +++++++++------
src/security/security_selinux.c | 75 +++++++++++++++++++++++++-------------
4 files changed, 127 insertions(+), 65 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 446f6ff..b67abf1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2443,8 +2443,14 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct
qemud_driver *
int rc = 0;
if (driver->securityDriver &&
- driver->securityDriver->domainSetSecurityLabel &&
- driver->securityDriver->domainSetSecurityLabel(conn,
driver->securityDriver, vm) < 0)
+ driver->securityDriver->domainSetSecurityAllLabel &&
+ driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0)
+ rc = -1;
+
+ if (rc == 0 &&
+ driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityProcessLabel &&
+ driver->securityDriver->domainSetSecurityProcessLabel(conn,
driver->securityDriver, vm) < 0)
rc = -1;
return rc;
@@ -3056,8 +3062,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
/* Reset Security Labels */
if (driver->securityDriver &&
- driver->securityDriver->domainRestoreSecurityLabel)
- driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
+ driver->securityDriver->domainRestoreSecurityAllLabel)
+ driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainReleaseSecurityLabel)
+ driver->securityDriver->domainReleaseSecurityLabel(conn, vm);
/* Clear out dynamically assigned labels */
if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
@@ -4633,8 +4642,8 @@ static int qemudDomainGetSecurityLabel(virDomainPtr dom,
virSecurityLabelPtr sec
* QEMU monitor hasn't seen SIGHUP/ERR on poll().
*/
if (virDomainObjIsActive(vm)) {
- if (driver->securityDriver &&
driver->securityDriver->domainGetSecurityLabel) {
- if (driver->securityDriver->domainGetSecurityLabel(dom->conn, vm,
seclabel) == -1) {
+ if (driver->securityDriver &&
driver->securityDriver->domainGetSecurityProcessLabel) {
+ if (driver->securityDriver->domainGetSecurityProcessLabel(dom->conn,
vm, seclabel) == -1) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("Failed to get security
label"));
goto cleanup;
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 0f9ff95..f288645 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -341,16 +341,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
if ((profile_name = get_profile_name(conn, vm)) == NULL)
return rc;
- /* if the profile is not already loaded, then load one */
- if (profile_loaded(profile_name) < 0) {
- if (load_profile(conn, profile_name, vm, NULL) < 0) {
- virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _("cannot generate AppArmor profile "
- "\'%s\'"), profile_name);
- goto clean;
- }
- }
-
vm->def->seclabel.label = strndup(profile_name, strlen(profile_name));
if (!vm->def->seclabel.label) {
virReportOOMError(NULL);
@@ -375,7 +365,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
goto clean;
err:
- remove_profile(profile_name);
VIR_FREE(vm->def->seclabel.label);
VIR_FREE(vm->def->seclabel.imagelabel);
VIR_FREE(vm->def->seclabel.model);
@@ -386,12 +375,33 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
return rc;
}
+static int
+AppArmorSetSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm)
+{
+ int rc = -1;
+
+ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
+ /* if the profile is not already loaded, then load one */
+ if (profile_loaded(vm->def->seclabel.label) < 0) {
+ if (load_profile(conn, vm->def->seclabel.label, vm, NULL) < 0) {
+ virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("cannot generate AppArmor profile "
+ "\'%s\'"),
vm->def->seclabel.label);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
/* Seen with 'virsh dominfo <vm>'. This function only called if the VM is
* running.
*/
static int
-AppArmorGetSecurityLabel(virConnectPtr conn,
- virDomainObjPtr vm, virSecurityLabelPtr sec)
+AppArmorGetSecurityProcessLabel(virConnectPtr conn,
+ virDomainObjPtr vm, virSecurityLabelPtr sec)
{
int rc = -1;
char *profile_name = NULL;
@@ -423,7 +433,20 @@ AppArmorGetSecurityLabel(virConnectPtr conn,
* more details. Currently called via qemudShutdownVMDaemon.
*/
static int
-AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
+AppArmorReleaseSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
+{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ VIR_FREE(secdef->model);
+ VIR_FREE(secdef->label);
+ VIR_FREE(secdef->imagelabel);
+
+ return 0;
+}
+
+
+static int
+AppArmorRestoreSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm)
{
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int rc = 0;
@@ -434,9 +457,6 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
_("could not remove profile for
\'%s\'"),
secdef->label);
}
- VIR_FREE(secdef->model);
- VIR_FREE(secdef->label);
- VIR_FREE(secdef->imagelabel);
}
return rc;
}
@@ -445,8 +465,8 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
* LOCAL_STATE_DIR/log/libvirt/qemu/<vm name>.log
*/
static int
-AppArmorSetSecurityLabel(virConnectPtr conn,
- virSecurityDriverPtr drv, virDomainObjPtr vm)
+AppArmorSetSecurityProcessLabel(virConnectPtr conn,
+ virSecurityDriverPtr drv, virDomainObjPtr vm)
{
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int rc = -1;
@@ -620,9 +640,11 @@ virSecurityDriver virAppArmorSecurityDriver = {
.domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel,
.domainGenSecurityLabel = AppArmorGenSecurityLabel,
.domainReserveSecurityLabel = AppArmorReserveSecurityLabel,
- .domainGetSecurityLabel = AppArmorGetSecurityLabel,
- .domainRestoreSecurityLabel = AppArmorRestoreSecurityLabel,
- .domainSetSecurityLabel = AppArmorSetSecurityLabel,
+ .domainReleaseSecurityLabel = AppArmorReleaseSecurityLabel,
+ .domainGetSecurityProcessLabel = AppArmorGetSecurityProcessLabel,
+ .domainSetSecurityProcessLabel = AppArmorSetSecurityProcessLabel,
+ .domainRestoreSecurityAllLabel = AppArmorRestoreSecurityAllLabel,
+ .domainSetSecurityAllLabel = AppArmorSetSecurityAllLabel,
.domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel,
.domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel,
};
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 97c9002..5d2446d 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -52,15 +52,19 @@ typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr
conn,
typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
virDomainObjPtr sec);
typedef int (*virSecurityDomainReserveLabel) (virConnectPtr conn,
- virDomainObjPtr sec);
-typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn,
- virDomainObjPtr vm,
- virSecurityLabelPtr sec);
-typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn,
- virDomainObjPtr vm);
-typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn,
- virSecurityDriverPtr drv,
- virDomainObjPtr vm);
+ virDomainObjPtr sec);
+typedef int (*virSecurityDomainReleaseLabel) (virConnectPtr conn,
+ virDomainObjPtr sec);
+typedef int (*virSecurityDomainSetAllLabel) (virConnectPtr conn,
+ virDomainObjPtr sec);
+typedef int (*virSecurityDomainRestoreAllLabel) (virConnectPtr conn,
+ virDomainObjPtr vm);
+typedef int (*virSecurityDomainGetProcessLabel) (virConnectPtr conn,
+ virDomainObjPtr vm,
+ virSecurityLabelPtr sec);
+typedef int (*virSecurityDomainSetProcessLabel) (virConnectPtr conn,
+ virSecurityDriverPtr drv,
+ virDomainObjPtr vm);
typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn,
virDomainDefPtr def);
@@ -73,9 +77,11 @@ struct _virSecurityDriver {
virSecurityDomainSetImageLabel domainSetSecurityImageLabel;
virSecurityDomainGenLabel domainGenSecurityLabel;
virSecurityDomainReserveLabel domainReserveSecurityLabel;
- virSecurityDomainGetLabel domainGetSecurityLabel;
- virSecurityDomainSetLabel domainSetSecurityLabel;
- virSecurityDomainRestoreLabel domainRestoreSecurityLabel;
+ virSecurityDomainReleaseLabel domainReleaseSecurityLabel;
+ virSecurityDomainGetProcessLabel domainGetSecurityProcessLabel;
+ virSecurityDomainSetProcessLabel domainSetSecurityProcessLabel;
+ virSecurityDomainSetAllLabel domainSetSecurityAllLabel;
+ virSecurityDomainRestoreAllLabel domainRestoreSecurityAllLabel;
virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel;
virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel;
virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c48f4c8..847a522 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -277,9 +277,9 @@ SELinuxSecurityDriverOpen(virConnectPtr conn, virSecurityDriverPtr
drv)
}
static int
-SELinuxGetSecurityLabel(virConnectPtr conn,
- virDomainObjPtr vm,
- virSecurityLabelPtr sec)
+SELinuxGetSecurityProcessLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virSecurityLabelPtr sec)
{
security_context_t ctx;
@@ -615,8 +615,8 @@ done:
}
static int
-SELinuxRestoreSecurityLabel(virConnectPtr conn,
- virDomainObjPtr vm)
+SELinuxRestoreSecurityAllLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
{
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
int i;
@@ -636,6 +636,19 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
vm->def->disks[i]) < 0)
rc = -1;
}
+
+ return rc;
+}
+
+static int
+SELinuxReleaseSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm)
+{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
context_t con = context_new(secdef->label);
if (con) {
mcsRemove(context_range_get(con));
@@ -646,7 +659,7 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
VIR_FREE(secdef->label);
VIR_FREE(secdef->imagelabel);
- return rc;
+ return 0;
}
@@ -693,13 +706,12 @@ SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def)
}
static int
-SELinuxSetSecurityLabel(virConnectPtr conn,
- virSecurityDriverPtr drv,
- virDomainObjPtr vm)
+SELinuxSetSecurityProcessLabel(virConnectPtr conn,
+ virSecurityDriverPtr drv,
+ virDomainObjPtr vm)
{
/* TODO: verify DOI */
const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
- int i;
if (vm->def->seclabel.label == NULL)
return 0;
@@ -722,18 +734,29 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
return -1;
}
- if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
- for (i = 0 ; i < vm->def->ndisks ; i++) {
- /* XXX fixme - we need to recursively label the entriy tree :-( */
- if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
- continue;
- if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0)
- return -1;
- }
- for (i = 0 ; i < vm->def->nhostdevs ; i++) {
- if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) <
0)
- return -1;
- }
+ return 0;
+}
+
+static int
+SELinuxSetSecurityAllLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ int i;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
+ for (i = 0 ; i < vm->def->ndisks ; i++) {
+ /* XXX fixme - we need to recursively label the entriy tree :-( */
+ if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
+ continue;
+ if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0)
+ return -1;
+ }
+ for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+ if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0)
+ return -1;
}
return 0;
@@ -748,9 +771,11 @@ virSecurityDriver virSELinuxSecurityDriver = {
.domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel,
.domainGenSecurityLabel = SELinuxGenSecurityLabel,
.domainReserveSecurityLabel = SELinuxReserveSecurityLabel,
- .domainGetSecurityLabel = SELinuxGetSecurityLabel,
- .domainRestoreSecurityLabel = SELinuxRestoreSecurityLabel,
- .domainSetSecurityLabel = SELinuxSetSecurityLabel,
+ .domainReleaseSecurityLabel = SELinuxReleaseSecurityLabel,
+ .domainGetSecurityProcessLabel = SELinuxGetSecurityProcessLabel,
+ .domainSetSecurityProcessLabel = SELinuxSetSecurityProcessLabel,
+ .domainRestoreSecurityAllLabel = SELinuxRestoreSecurityAllLabel,
+ .domainSetSecurityAllLabel = SELinuxSetSecurityAllLabel,
.domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel,
.domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel,
.domainSetSavedStateLabel = SELinuxSetSavedStateLabel,
--
1.6.5.2
10:19 a.m.
New subject: [libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver
On Wed, Jan 20, 2010 at 03:14:59PM +0000, Daniel P. Berrange wrote:
The current security driver architecture has the following
split of logic
* domainGenSecurityLabel
Allocate the unique label for the domain about to be started
* domainGetSecurityLabel
Retrieve the current live security label for a process
* domainSetSecurityLabel
Apply the previously allocated label to the current process
Setup all disk image / device labelling
* domainRestoreSecurityLabel
Restore the original disk image / device labelling.
Release the unique label for the domain
The 'domainSetSecurityLabel' method is special because it runs
in the context of the child process between the fork + exec.
This is require in order to set the process label. It is not
required in order to label disks/devices though. Having the
disk labelling code run in the child process limits what it
can do.
In particularly libvirtd would like to remember the current
disk image label, and only change shared image labels for the
first VM to start. This requires use & update of global state
in the libvirtd daemon, and thus cannot run in the child
process context.
The solution is to split domainSetSecurityLabel into two parts,
one applies process label, and the other handles disk image
labelling. At the same time domainRestoreSecurityLabel is
similarly split, just so that it matches the style. Thus the
previous 4 methods are replaced by the following 6 new methods
* domainGenSecurityLabel
Allocate the unique label for the domain about to be started
No actual change here.
* domainReleaseSecurityLabel
Release the unique label for the domain
* domainGetSecurityProcessLabel
Retrieve the current live security label for a process
Merely renamed for clarity.
* domainSetSecurityProcessLabel
Apply the previously allocated label to the current process
* domainRestoreSecurityAllLabel
Restore the original disk image / device labelling.
* domainSetSecurityAllLabel
Setup all disk image / device labelling
The SELinux and AppArmour drivers are then updated to comply with
this new spec. Notice that the AppArmour driver was actually a
little different. It was creating its profile for the disk image
and device labels in the 'domainGenSecurityLabel' method, where as
the SELinux driver did it in 'domainSetSecurityLabel'. With the
new method split, we can have consistency, with both drivers doing
that in the domainSetSecurityAllLabel method.
A bit complex, thanks for the explanations !
NB, the AppArmour changes here haven't been compiled so may not
build.
---
src/qemu/qemu_driver.c | 21 ++++++++---
src/security/security_apparmor.c | 66 ++++++++++++++++++++++-----------
src/security/security_driver.h | 30 +++++++++------
src/security/security_selinux.c | 75 +++++++++++++++++++++++++-------------
4 files changed, 127 insertions(+), 65 deletions(-)
[...]
+static int
+SELinuxSetSecurityAllLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+ int i;
+
+ if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+ return 0;
+
+ for (i = 0 ; i < vm->def->ndisks ; i++) {
+ /* XXX fixme - we need to recursively label the entriy tree :-( */
while we're are there let's fix the entriy/entire typo
+ if (vm->def->disks[i]->type ==
VIR_DOMAIN_DISK_TYPE_DIR)
+ continue;
+ if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0)
+ return -1;
+ }
+ for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+ if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) <
0)
+ return -1;
}
Okay, ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
11:41 a.m.
New subject: [libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver
On Wed, 2010-01-20 at 17:19 +0100, Daniel Veillard wrote:
On Wed, Jan 20, 2010 at 03:14:59PM +0000, Daniel P. Berrange wrote:
> The current security driver architecture has the following
> split of logic
>
> * domainGenSecurityLabel
>
> Allocate the unique label for the domain about to be started
>
> * domainGetSecurityLabel
>
> Retrieve the current live security label for a process
>
> * domainSetSecurityLabel
>
> Apply the previously allocated label to the current process
> Setup all disk image / device labelling
>
> * domainRestoreSecurityLabel
>
> Restore the original disk image / device labelling.
> Release the unique label for the domain
>
> The 'domainSetSecurityLabel' method is special because it runs
> in the context of the child process between the fork + exec.
>
> This is require in order to set the process label. It is not
> required in order to label disks/devices though. Having the
> disk labelling code run in the child process limits what it
> can do.
>
> In particularly libvirtd would like to remember the current
> disk image label, and only change shared image labels for the
> first VM to start. This requires use & update of global state
> in the libvirtd daemon, and thus cannot run in the child
> process context.
>
> The solution is to split domainSetSecurityLabel into two parts,
> one applies process label, and the other handles disk image
> labelling. At the same time domainRestoreSecurityLabel is
> similarly split, just so that it matches the style. Thus the
> previous 4 methods are replaced by the following 6 new methods
>
> * domainGenSecurityLabel
>
> Allocate the unique label for the domain about to be started
> No actual change here.
>
> * domainReleaseSecurityLabel
>
> Release the unique label for the domain
>
> * domainGetSecurityProcessLabel
>
> Retrieve the current live security label for a process
> Merely renamed for clarity.
>
> * domainSetSecurityProcessLabel
>
> Apply the previously allocated label to the current process
>
> * domainRestoreSecurityAllLabel
>
> Restore the original disk image / device labelling.
>
> * domainSetSecurityAllLabel
>
> Setup all disk image / device labelling
>
> The SELinux and AppArmour drivers are then updated to comply with
> this new spec. Notice that the AppArmour driver was actually a
> little different. It was creating its profile for the disk image
> and device labels in the 'domainGenSecurityLabel' method, where as
> the SELinux driver did it in 'domainSetSecurityLabel'. With the
> new method split, we can have consistency, with both drivers doing
> that in the domainSetSecurityAllLabel method.
A bit complex, thanks for the explanations !
> NB, the AppArmour changes here haven't been compiled so may not
> build.
> ---
> src/qemu/qemu_driver.c | 21 ++++++++---
> src/security/security_apparmor.c | 66 ++++++++++++++++++++++-----------
> src/security/security_driver.h | 30 +++++++++------
> src/security/security_selinux.c | 75 +++++++++++++++++++++++++-------------
> 4 files changed, 127 insertions(+), 65 deletions(-)
[...]
> +static int
> +SELinuxSetSecurityAllLabel(virConnectPtr conn,
> + virDomainObjPtr vm)
> +{
> + const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
> + int i;
> +
> + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> + return 0;
> +
> + for (i = 0 ; i < vm->def->ndisks ; i++) {
> + /* XXX fixme - we need to recursively label the entriy tree :-( */
while we're are there let's fix the entriy/entire typo
Not sure what this is supposed to mean, but you could exec chcon -R on
the directory tree if you want to recursively relabel it.
--
Stephen Smalley
National Security Agency
11:42 a.m.
New subject: [libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver
On Wed, Jan 20, 2010 at 12:41:29PM -0500, Stephen Smalley wrote:
On Wed, 2010-01-20 at 17:19 +0100, Daniel Veillard wrote:
> On Wed, Jan 20, 2010 at 03:14:59PM +0000, Daniel P. Berrange wrote:
> > The current security driver architecture has the following
> > split of logic
> >
> > * domainGenSecurityLabel
> >
> > Allocate the unique label for the domain about to be started
> >
> > * domainGetSecurityLabel
> >
> > Retrieve the current live security label for a process
> >
> > * domainSetSecurityLabel
> >
> > Apply the previously allocated label to the current process
> > Setup all disk image / device labelling
> >
> > * domainRestoreSecurityLabel
> >
> > Restore the original disk image / device labelling.
> > Release the unique label for the domain
> >
> > The 'domainSetSecurityLabel' method is special because it runs
> > in the context of the child process between the fork + exec.
> >
> > This is require in order to set the process label. It is not
> > required in order to label disks/devices though. Having the
> > disk labelling code run in the child process limits what it
> > can do.
> >
> > In particularly libvirtd would like to remember the current
> > disk image label, and only change shared image labels for the
> > first VM to start. This requires use & update of global state
> > in the libvirtd daemon, and thus cannot run in the child
> > process context.
> >
> > The solution is to split domainSetSecurityLabel into two parts,
> > one applies process label, and the other handles disk image
> > labelling. At the same time domainRestoreSecurityLabel is
> > similarly split, just so that it matches the style. Thus the
> > previous 4 methods are replaced by the following 6 new methods
> >
> > * domainGenSecurityLabel
> >
> > Allocate the unique label for the domain about to be started
> > No actual change here.
> >
> > * domainReleaseSecurityLabel
> >
> > Release the unique label for the domain
> >
> > * domainGetSecurityProcessLabel
> >
> > Retrieve the current live security label for a process
> > Merely renamed for clarity.
> >
> > * domainSetSecurityProcessLabel
> >
> > Apply the previously allocated label to the current process
> >
> > * domainRestoreSecurityAllLabel
> >
> > Restore the original disk image / device labelling.
> >
> > * domainSetSecurityAllLabel
> >
> > Setup all disk image / device labelling
> >
> > The SELinux and AppArmour drivers are then updated to comply with
> > this new spec. Notice that the AppArmour driver was actually a
> > little different. It was creating its profile for the disk image
> > and device labels in the 'domainGenSecurityLabel' method, where as
> > the SELinux driver did it in 'domainSetSecurityLabel'. With the
> > new method split, we can have consistency, with both drivers doing
> > that in the domainSetSecurityAllLabel method.
>
> A bit complex, thanks for the explanations !
>
> > NB, the AppArmour changes here haven't been compiled so may not
> > build.
> > ---
> > src/qemu/qemu_driver.c | 21 ++++++++---
> > src/security/security_apparmor.c | 66 ++++++++++++++++++++++-----------
> > src/security/security_driver.h | 30 +++++++++------
> > src/security/security_selinux.c | 75
+++++++++++++++++++++++++-------------
> > 4 files changed, 127 insertions(+), 65 deletions(-)
> [...]
> > +static int
> > +SELinuxSetSecurityAllLabel(virConnectPtr conn,
> > + virDomainObjPtr vm)
> > +{
> > + const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
> > + int i;
> > +
> > + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> > + return 0;
> > +
> > + for (i = 0 ; i < vm->def->ndisks ; i++) {
> > + /* XXX fixme - we need to recursively label the entriy tree :-( */
>
> while we're are there let's fix the entriy/entire typo
Not sure what this is supposed to mean, but you could exec chcon -R on
the directory tree if you want to recursively relabel it.
It is tricky, because part of the this refactoring work is to allow me to
maintain a DB of the original contexts, so that the restore step at VM
shutdown can put back the original context, rather than resetting to the
defaults using matchpathcon(). So we need to handle the recursive
chcon directly in libvirt, rather than spawning an external command.
Fortunately, I don't expect anyone to ever use the VVFAT driver that
this comment in referring to, so I'm not particularly worrying about
fixing this any time soon.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:15 a.m.
New subject: [libvirt] [PATCH 03/12] Fix leak of allocated security label
If a VM fails to start, we can't simply free the security label
strings, we must call the domainReleaseSecurityLabel() method
otherwise the reserved 'mcs' level will be leaked in SElinux
* src/qemu/qemu_driver.c: Invoke domainReleaseSecurityLabel()
when domain fails to start
---
src/qemu/qemu_driver.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b67abf1..6c66b22 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2978,11 +2978,9 @@ cleanup:
/* We jump here if we failed to start the VM for any reason
* XXX investigate if we can kill this block and safely call
* qemudShutdownVMDaemon even though no PID is running */
- 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);
- }
+ if (driver->securityDriver &&
+ driver->securityDriver->domainReleaseSecurityLabel)
+ driver->securityDriver->domainReleaseSecurityLabel(conn, vm);
qemuRemoveCgroup(conn, driver, vm, 0);
if ((vm->def->ngraphics == 1) &&
vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
--
1.6.5.2
10:20 a.m.
New subject: [libvirt] [PATCH 03/12] Fix leak of allocated security label
On Wed, Jan 20, 2010 at 03:15:00PM +0000, Daniel P. Berrange wrote:
If a VM fails to start, we can't simply free the security label
strings, we must call the domainReleaseSecurityLabel() method
otherwise the reserved 'mcs' level will be leaked in SElinux
* src/qemu/qemu_driver.c: Invoke domainReleaseSecurityLabel()
when domain fails to start
---
src/qemu/qemu_driver.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b67abf1..6c66b22 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2978,11 +2978,9 @@ cleanup:
/* We jump here if we failed to start the VM for any reason
* XXX investigate if we can kill this block and safely call
* qemudShutdownVMDaemon even though no PID is running */
- 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);
- }
+ if (driver->securityDriver &&
+ driver->securityDriver->domainReleaseSecurityLabel)
+ driver->securityDriver->domainReleaseSecurityLabel(conn, vm);
qemuRemoveCgroup(conn, driver, vm, 0);
if ((vm->def->ngraphics == 1) &&
vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 04/12] Pull initial disk labelling out into libvirtd instead of exec hook
Pulling the disk labelling code out of the exec hook, and into
libvirtd will allow it to access shared state in the daemon. It
will also make debugging & error reporting easier / more reliable.
* qemu/qemu_driver.c: Move initial disk labelling calls up into
libvirtd. Add cleanup of disk labels upon failure
---
src/qemu/qemu_driver.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6c66b22..8195b74 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2443,12 +2443,6 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct
qemud_driver *
int rc = 0;
if (driver->securityDriver &&
- driver->securityDriver->domainSetSecurityAllLabel &&
- driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0)
- rc = -1;
-
- if (rc == 0 &&
- driver->securityDriver &&
driver->securityDriver->domainSetSecurityProcessLabel &&
driver->securityDriver->domainSetSecurityProcessLabel(conn,
driver->securityDriver, vm) < 0)
rc = -1;
@@ -2771,6 +2765,11 @@ static int qemudStartVMDaemon(virConnectPtr conn,
driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
return -1;
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityAllLabel &&
+ driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0)
+ goto cleanup;
+
/* Ensure no historical cgroup for this VM is lieing around bogus settings */
qemuRemoveCgroup(conn, driver, vm, 1);
@@ -2979,6 +2978,9 @@ cleanup:
* XXX investigate if we can kill this block and safely call
* qemudShutdownVMDaemon even though no PID is running */
if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityAllLabel)
+ driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);
+ if (driver->securityDriver &&
driver->securityDriver->domainReleaseSecurityLabel)
driver->securityDriver->domainReleaseSecurityLabel(conn, vm);
qemuRemoveCgroup(conn, driver, vm, 0);
--
1.6.5.2
10:24 a.m.
New subject: [libvirt] [PATCH 04/12] Pull initial disk labelling out into libvirtd instead of exec hook
On Wed, Jan 20, 2010 at 03:15:01PM +0000, Daniel P. Berrange wrote:
Pulling the disk labelling code out of the exec hook, and into
libvirtd will allow it to access shared state in the daemon. It
will also make debugging & error reporting easier / more reliable.
* qemu/qemu_driver.c: Move initial disk labelling calls up into
libvirtd. Add cleanup of disk labels upon failure
---
src/qemu/qemu_driver.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6c66b22..8195b74 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2443,12 +2443,6 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct
qemud_driver *
int rc = 0;
if (driver->securityDriver &&
- driver->securityDriver->domainSetSecurityAllLabel &&
- driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0)
- rc = -1;
-
- if (rc == 0 &&
- driver->securityDriver &&
driver->securityDriver->domainSetSecurityProcessLabel &&
driver->securityDriver->domainSetSecurityProcessLabel(conn,
driver->securityDriver, vm) < 0)
rc = -1;
@@ -2771,6 +2765,11 @@ static int qemudStartVMDaemon(virConnectPtr conn,
driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
return -1;
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityAllLabel &&
+ driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0)
+ goto cleanup;
+
/* Ensure no historical cgroup for this VM is lieing around bogus settings */
qemuRemoveCgroup(conn, driver, vm, 1);
@@ -2979,6 +2978,9 @@ cleanup:
* XXX investigate if we can kill this block and safely call
* qemudShutdownVMDaemon even though no PID is running */
if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityAllLabel)
+ driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);
+ if (driver->securityDriver &&
driver->securityDriver->domainReleaseSecurityLabel)
driver->securityDriver->domainReleaseSecurityLabel(conn, vm);
qemuRemoveCgroup(conn, driver, vm, 0);
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 05/12] Introduce a stacked security driver impl for QEMU
* qemu/qemu_conf.h: Add securityPrimaryDriver and
securitySecondaryDriver fields to 'struct qemud_driver'
* Makefile.am: Add new files
* qemu/qemu_security_stacked.c, qemu/qemu_security_stacked.h: A
simple stacked security driver
---
src/Makefile.am | 4 +-
src/qemu/qemu_conf.h | 2 +
src/qemu/qemu_security_stacked.c | 353 ++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_security_stacked.h | 22 +++
4 files changed, 380 insertions(+), 1 deletions(-)
create mode 100644 src/qemu/qemu_security_stacked.c
create mode 100644 src/qemu/qemu_security_stacked.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 713cbda..0fb6dba 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -198,7 +198,9 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_monitor_json.h \
qemu/qemu_driver.c qemu/qemu_driver.h \
qemu/qemu_bridge_filter.c \
- qemu/qemu_bridge_filter.h
+ qemu/qemu_bridge_filter.h \
+ qemu/qemu_security_stacked.h \
+ qemu/qemu_security_stacked.c
UML_DRIVER_SOURCES = \
uml/uml_conf.c uml/uml_conf.h \
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index ed2d32b..678bc6f 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -134,6 +134,8 @@ struct qemud_driver {
char *securityDriverName;
virSecurityDriverPtr securityDriver;
+ virSecurityDriverPtr securityPrimaryDriver;
+ virSecurityDriverPtr securitySecondaryDriver;
char *saveImageFormat;
diff --git a/src/qemu/qemu_security_stacked.c b/src/qemu/qemu_security_stacked.c
new file mode 100644
index 0000000..60acb4c
--- /dev/null
+++ b/src/qemu/qemu_security_stacked.c
@@ -0,0 +1,353 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * QEMU stacked security driver
+ */
+
+#include <config.h>
+#include <selinux/selinux.h>
+#include <selinux/context.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "qemu_security_stacked.h"
+
+#include "qemu_conf.h"
+#include "datatypes.h"
+#include "virterror_internal.h"
+#include "util.h"
+#include "memory.h"
+#include "logging.h"
+#include "pci.h"
+#include "hostusb.h"
+#include "storage_file.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+
+static struct qemud_driver *driver;
+
+void qemuSecurityStackedSetDriver(struct qemud_driver *newdriver)
+{
+ driver = newdriver;
+}
+
+
+static int
+qemuSecurityStackedVerify(virConnectPtr conn,
+ virDomainDefPtr def)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainSecurityVerify &&
+ driver->securitySecondaryDriver->domainSecurityVerify(conn, def) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainSecurityVerify &&
+ driver->securityPrimaryDriver->domainSecurityVerify(conn, def) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedGenLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainGenSecurityLabel &&
+ driver->securitySecondaryDriver->domainGenSecurityLabel(conn, vm) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainGenSecurityLabel &&
+ driver->securityPrimaryDriver->domainGenSecurityLabel(conn, vm) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedReleaseLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainReleaseSecurityLabel &&
+ driver->securitySecondaryDriver->domainReleaseSecurityLabel(conn, vm) <
0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainReleaseSecurityLabel &&
+ driver->securityPrimaryDriver->domainReleaseSecurityLabel(conn, vm) <
0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedReserveLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainReserveSecurityLabel &&
+ driver->securitySecondaryDriver->domainReserveSecurityLabel(conn, vm) <
0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainReserveSecurityLabel &&
+ driver->securityPrimaryDriver->domainReserveSecurityLabel(conn, vm) <
0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedSetSecurityImageLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainSetSecurityImageLabel &&
+ driver->securitySecondaryDriver->domainSetSecurityImageLabel(conn, vm,
disk) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainSetSecurityImageLabel &&
+ driver->securityPrimaryDriver->domainSetSecurityImageLabel(conn, vm, disk)
< 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedRestoreSecurityImageLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainRestoreSecurityImageLabel
&&
+ driver->securitySecondaryDriver->domainRestoreSecurityImageLabel(conn, vm,
disk) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainRestoreSecurityImageLabel &&
+ driver->securityPrimaryDriver->domainRestoreSecurityImageLabel(conn, vm,
disk) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedSetSecurityHostdevLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virDomainHostdevDefPtr dev)
+
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainSetSecurityHostdevLabel &&
+ driver->securitySecondaryDriver->domainSetSecurityHostdevLabel(conn, vm,
dev) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainSetSecurityHostdevLabel &&
+ driver->securityPrimaryDriver->domainSetSecurityHostdevLabel(conn, vm, dev)
< 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedRestoreSecurityHostdevLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virDomainHostdevDefPtr dev)
+
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainRestoreSecurityHostdevLabel
&&
+ driver->securitySecondaryDriver->domainRestoreSecurityHostdevLabel(conn,
vm, dev) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainRestoreSecurityHostdevLabel
&&
+ driver->securityPrimaryDriver->domainRestoreSecurityHostdevLabel(conn, vm,
dev) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedSetSecurityAllLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainSetSecurityAllLabel &&
+ driver->securitySecondaryDriver->domainSetSecurityAllLabel(conn, vm) <
0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainSetSecurityAllLabel &&
+ driver->securityPrimaryDriver->domainSetSecurityAllLabel(conn, vm) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedRestoreSecurityAllLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainRestoreSecurityAllLabel &&
+ driver->securitySecondaryDriver->domainRestoreSecurityAllLabel(conn, vm)
< 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainRestoreSecurityAllLabel &&
+ driver->securityPrimaryDriver->domainRestoreSecurityAllLabel(conn, vm) <
0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedSetSavedStateLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ const char *savefile)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainSetSavedStateLabel &&
+ driver->securitySecondaryDriver->domainSetSavedStateLabel(conn, vm,
savefile) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainSetSavedStateLabel &&
+ driver->securityPrimaryDriver->domainSetSavedStateLabel(conn, vm, savefile)
< 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedRestoreSavedStateLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ const char *savefile)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainRestoreSavedStateLabel &&
+ driver->securitySecondaryDriver->domainRestoreSavedStateLabel(conn, vm,
savefile) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainRestoreSavedStateLabel &&
+ driver->securityPrimaryDriver->domainRestoreSavedStateLabel(conn, vm,
savefile) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+
+static int
+qemuSecurityStackedSetProcessLabel(virConnectPtr conn,
+ virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainSetSecurityProcessLabel &&
+ driver->securitySecondaryDriver->domainSetSecurityProcessLabel(conn,
+
driver->securitySecondaryDriver,
+ vm) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainSetSecurityProcessLabel &&
+ driver->securityPrimaryDriver->domainSetSecurityProcessLabel(conn,
+
driver->securityPrimaryDriver,
+ vm) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+static int
+qemuSecurityStackedGetProcessLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virSecurityLabelPtr seclabel)
+{
+ int rc = 0;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainGetSecurityProcessLabel &&
+ driver->securityPrimaryDriver->domainGetSecurityProcessLabel(conn,
+ vm,
+ seclabel) < 0)
+ rc = -1;
+
+ return rc;
+}
+
+virSecurityDriver qemuStackedSecurityDriver = {
+ .name = "qemuStacked",
+ .domainSecurityVerify = qemuSecurityStackedVerify,
+
+ .domainGenSecurityLabel = qemuSecurityStackedGenLabel,
+ .domainReleaseSecurityLabel = qemuSecurityStackedReleaseLabel,
+ .domainReserveSecurityLabel = qemuSecurityStackedReserveLabel,
+
+ .domainGetSecurityProcessLabel = qemuSecurityStackedGetProcessLabel,
+ .domainSetSecurityProcessLabel = qemuSecurityStackedSetProcessLabel,
+
+ .domainSetSecurityImageLabel = qemuSecurityStackedSetSecurityImageLabel,
+ .domainRestoreSecurityImageLabel = qemuSecurityStackedRestoreSecurityImageLabel,
+
+ .domainSetSecurityAllLabel = qemuSecurityStackedSetSecurityAllLabel,
+ .domainRestoreSecurityAllLabel = qemuSecurityStackedRestoreSecurityAllLabel,
+
+ .domainSetSecurityHostdevLabel = qemuSecurityStackedSetSecurityHostdevLabel,
+ .domainRestoreSecurityHostdevLabel = qemuSecurityStackedRestoreSecurityHostdevLabel,
+
+ .domainSetSavedStateLabel = qemuSecurityStackedSetSavedStateLabel,
+ .domainRestoreSavedStateLabel = qemuSecurityStackedRestoreSavedStateLabel,
+};
diff --git a/src/qemu/qemu_security_stacked.h b/src/qemu/qemu_security_stacked.h
new file mode 100644
index 0000000..d67a5f1
--- /dev/null
+++ b/src/qemu/qemu_security_stacked.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * QEMU stacked security driver
+ */
+
+#include "security/security_driver.h"
+#include "qemu_conf.h"
+
+#ifndef __QEMU_SECURITY_STACKED
+#define __QEMU_SECURITY_STACKED
+
+extern virSecurityDriver qemuStackedSecurityDriver;
+
+void qemuSecurityStackedSetDriver(struct qemud_driver *driver);
+
+#endif /* __QEMU_SECURITY_DAC */
--
1.6.5.2
11 a.m.
New subject: [libvirt] [PATCH 05/12] Introduce a stacked security driver impl for QEMU
On Wed, Jan 20, 2010 at 03:15:02PM +0000, Daniel P. Berrange wrote:
* qemu/qemu_conf.h: Add securityPrimaryDriver and
securitySecondaryDriver fields to 'struct qemud_driver'
* Makefile.am: Add new files
* qemu/qemu_security_stacked.c, qemu/qemu_security_stacked.h: A
simple stacked security driver
---
src/Makefile.am | 4 +-
src/qemu/qemu_conf.h | 2 +
src/qemu/qemu_security_stacked.c | 353 ++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_security_stacked.h | 22 +++
4 files changed, 380 insertions(+), 1 deletions(-)
create mode 100644 src/qemu/qemu_security_stacked.c
create mode 100644 src/qemu/qemu_security_stacked.h
[...]
+++ b/src/qemu/qemu_security_stacked.c
@@ -0,0 +1,353 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * QEMU stacked security driver
+ */
+
+#include <config.h>
+#include <selinux/selinux.h>
+#include <selinux/context.h>
Why do we need to include SELinux headers, the module content seems to
be agnostic on the underlying framework (SELinux/Apparmor)
+static int
+qemuSecurityStackedGenLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int rc = 0;
+
+ if (driver->securitySecondaryDriver &&
+ driver->securitySecondaryDriver->domainGenSecurityLabel &&
+ driver->securitySecondaryDriver->domainGenSecurityLabel(conn, vm) < 0)
+ rc = -1;
+
+ if (driver->securityPrimaryDriver &&
+ driver->securityPrimaryDriver->domainGenSecurityLabel &&
+ driver->securityPrimaryDriver->domainGenSecurityLabel(conn, vm) < 0)
+ rc = -1;
+
+ return rc;
+}
Okay, so the general pattern is that any stacked driver call will fail
if any of the subdriver entry point fail, a missing entry point not
being a failure. I'm still surprized that if both entry point are
missing we return 0 i.e. success...
+virSecurityDriver qemuStackedSecurityDriver = {
+ .name = "qemuStacked",
+ .domainSecurityVerify = qemuSecurityStackedVerify,
+
+ .domainGenSecurityLabel = qemuSecurityStackedGenLabel,
+ .domainReleaseSecurityLabel = qemuSecurityStackedReleaseLabel,
+ .domainReserveSecurityLabel = qemuSecurityStackedReserveLabel,
+
+ .domainGetSecurityProcessLabel = qemuSecurityStackedGetProcessLabel,
+ .domainSetSecurityProcessLabel = qemuSecurityStackedSetProcessLabel,
+
+ .domainSetSecurityImageLabel = qemuSecurityStackedSetSecurityImageLabel,
+ .domainRestoreSecurityImageLabel = qemuSecurityStackedRestoreSecurityImageLabel,
+
+ .domainSetSecurityAllLabel = qemuSecurityStackedSetSecurityAllLabel,
+ .domainRestoreSecurityAllLabel = qemuSecurityStackedRestoreSecurityAllLabel,
+
+ .domainSetSecurityHostdevLabel = qemuSecurityStackedSetSecurityHostdevLabel,
+ .domainRestoreSecurityHostdevLabel =
qemuSecurityStackedRestoreSecurityHostdevLabel,
+
+ .domainSetSavedStateLabel = qemuSecurityStackedSetSavedStateLabel,
+ .domainRestoreSavedStateLabel = qemuSecurityStackedRestoreSavedStateLabel,
+};
I tend to prefer full initialization to see if there is missing entry points.
Just minor remarks except maybe for the return value in case both
subdrivers misses the entry point.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
11:05 a.m.
New subject: [libvirt] [PATCH 05/12] Introduce a stacked security driver impl for QEMU
On Wed, Jan 20, 2010 at 06:00:06PM +0100, Daniel Veillard wrote:
On Wed, Jan 20, 2010 at 03:15:02PM +0000, Daniel P. Berrange wrote:
> * qemu/qemu_conf.h: Add securityPrimaryDriver and
> securitySecondaryDriver fields to 'struct qemud_driver'
> * Makefile.am: Add new files
> * qemu/qemu_security_stacked.c, qemu/qemu_security_stacked.h: A
> simple stacked security driver
> ---
> src/Makefile.am | 4 +-
> src/qemu/qemu_conf.h | 2 +
> src/qemu/qemu_security_stacked.c | 353 ++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_security_stacked.h | 22 +++
> 4 files changed, 380 insertions(+), 1 deletions(-)
> create mode 100644 src/qemu/qemu_security_stacked.c
> create mode 100644 src/qemu/qemu_security_stacked.h
[...]
> +++ b/src/qemu/qemu_security_stacked.c
> @@ -0,0 +1,353 @@
> +/*
> + * Copyright (C) 2010 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * QEMU stacked security driver
> + */
> +
> +#include <config.h>
> +#include <selinux/selinux.h>
> +#include <selinux/context.h>
Why do we need to include SELinux headers, the module content seems to
be agnostic on the underlying framework (SELinux/Apparmor)
Copy & paste error :-)
> +static int
> +qemuSecurityStackedGenLabel(virConnectPtr conn,
> + virDomainObjPtr vm)
> +{
> + int rc = 0;
> +
> + if (driver->securitySecondaryDriver &&
> + driver->securitySecondaryDriver->domainGenSecurityLabel &&
> + driver->securitySecondaryDriver->domainGenSecurityLabel(conn, vm)
< 0)
> + rc = -1;
> +
> + if (driver->securityPrimaryDriver &&
> + driver->securityPrimaryDriver->domainGenSecurityLabel &&
> + driver->securityPrimaryDriver->domainGenSecurityLabel(conn, vm) <
0)
> + rc = -1;
> +
> + return rc;
> +}
Okay, so the general pattern is that any stacked driver call will fail
if any of the subdriver entry point fail, a missing entry point not
being a failure. I'm still surprized that if both entry point are
missing we return 0 i.e. success...
If the operation being performed doesn't make sense for a driver it is
fine to skip it without error. This is actually the same semantics we
had before this refactoring, because the QEMU driver always checks for
NULL entry point & skips it without error.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:15 a.m.
New subject: [libvirt] [PATCH 06/12] Introduce a new DAC security driver for QEMU
This new security driver is responsible for managing UID/GID changes
to the QEMU process, and any files/disks/devices assigned to it.
* qemu/qemu_conf.h: Add flag for disabling automatic file permission
changes
* qemu/qemu_security_dac.h, qemu/qemu_security_dac.c: New DAC driver
for QEMU guests
* Makefile.am: Add new files
---
po/POTFILES.in | 1 +
src/Makefile.am | 4 +-
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_security_dac.c | 458 ++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_security_dac.h | 22 ++
5 files changed, 485 insertions(+), 1 deletions(-)
create mode 100644 src/qemu/qemu_security_dac.c
create mode 100644 src/qemu/qemu_security_dac.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 3b82a74..18f5243 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -36,6 +36,7 @@ src/qemu/qemu_driver.c
src/qemu/qemu_monitor.c
src/qemu/qemu_monitor_json.c
src/qemu/qemu_monitor_text.c
+src/qemu/qemu_security_dac.c
src/remote/remote_driver.c
src/secret/secret_driver.c
src/security/security_driver.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 0fb6dba..3232256 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -200,7 +200,9 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_bridge_filter.c \
qemu/qemu_bridge_filter.h \
qemu/qemu_security_stacked.h \
- qemu/qemu_security_stacked.c
+ qemu/qemu_security_stacked.c \
+ qemu/qemu_security_dac.h \
+ qemu/qemu_security_dac.c
UML_DRIVER_SOURCES = \
uml/uml_conf.c uml/uml_conf.h \
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 678bc6f..e4a10bb 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -90,6 +90,7 @@ struct qemud_driver {
uid_t user;
gid_t group;
+ int dynamicOwnership;
unsigned int qemuVersion;
int nextvmid;
diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c
new file mode 100644
index 0000000..6b7aab5
--- /dev/null
+++ b/src/qemu/qemu_security_dac.c
@@ -0,0 +1,458 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * QEMU POSIX DAC security driver
+ */
+#include <config.h>
+#include <selinux/selinux.h>
+#include <selinux/context.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "qemu_security_dac.h"
+#include "qemu_conf.h"
+#include "datatypes.h"
+#include "virterror_internal.h"
+#include "util.h"
+#include "memory.h"
+#include "logging.h"
+#include "pci.h"
+#include "hostusb.h"
+#include "storage_file.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+static struct qemud_driver *driver;
+
+void qemuSecurityDACSetDriver(struct qemud_driver *newdriver)
+{
+ driver = newdriver;
+}
+
+
+static int
+qemuSecurityDACSetOwnership(virConnectPtr conn, const char *path, int uid, int gid)
+{
+ VIR_INFO("Setting DAC context on '%s' to '%d:%d'", path,
uid, gid);
+
+ if (chown(path, uid, gid) < 0) {
+ struct stat sb;
+ int chown_errno = errno;
+
+ if (stat(path, &sb) >= 0) {
+ if (sb.st_uid == uid &&
+ sb.st_gid == gid) {
+ /* It's alright, there's nothing to change anyway. */
+ return 0;
+ }
+ }
+
+ /* if the error complaint is related to an image hosted on
+ * an nfs mount, or a usbfs/sysfs filesystem not supporting
+ * labelling, then just ignore it & hope for the best.
+ * The user hopefully set one of the necessary qemuSecurityDAC
+ * virt_use_{nfs,usb,pci} boolean tunables to allow it...
+ */
+ if (chown_errno == EOPNOTSUPP) {
+ VIR_INFO("Setting security context '%d:%d' on '%s' not
supported by filesystem",
+ uid, gid, path);
+ } else if (chown_errno == EPERM) {
+ VIR_INFO("Setting security context '%d:%d' on '%s' not
permitted",
+ uid, gid, path);
+ } else if (chown_errno == EROFS) {
+ VIR_INFO("Setting security context '%d:%d' on '%s' not
possible on readonly filesystem",
+ uid, gid, path);
+ } else {
+ virReportSystemError(conn, chown_errno,
+ _("unable to set security context '%d:%d'
on '%s'"),
+ uid, gid, path);
+ return -1;
+ }
+ }
+ return 0;
+}
+
+static int
+qemuSecurityDACRestoreSecurityFileLabel(virConnectPtr conn,
+ const char *path)
+{
+ struct stat buf;
+ security_context_t fcon = NULL;
+ int rc = -1;
+ int err;
+ char *newpath = NULL;
+
+ VIR_INFO("Restoring DAC context on '%s'", path);
+
+ if ((err = virFileResolveLink(path, &newpath)) < 0) {
+ virReportSystemError(conn, err,
+ _("cannot resolve symlink %s"), path);
+ goto err;
+ }
+
+ if (stat(newpath, &buf) != 0)
+ goto err;
+
+ /* XXX record previous ownership */
+ rc = qemuSecurityDACSetOwnership(conn, newpath, 0, 0);
+
+err:
+ VIR_FREE(fcon);
+ VIR_FREE(newpath);
+ return rc;
+}
+
+
+static int
+qemuSecurityDACSetSecurityImageLabel(virConnectPtr conn,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainDiskDefPtr disk)
+
+{
+ const char *path;
+
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ if (!disk->src)
+ return 0;
+
+ path = disk->src;
+ do {
+ virStorageFileMetadata meta;
+ int ret;
+
+ memset(&meta, 0, sizeof(meta));
+
+ ret = virStorageFileGetMetadata(conn, path, &meta);
+
+ if (path != disk->src)
+ VIR_FREE(path);
+ path = NULL;
+
+ if (ret < 0)
+ return -1;
+
+ if (meta.backingStore != NULL &&
+ qemuSecurityDACSetOwnership(conn, meta.backingStore,
+ driver->user, driver->group) < 0) {
+ VIR_FREE(meta.backingStore);
+ return -1;
+ }
+
+ path = meta.backingStore;
+ } while (path != NULL);
+
+ return qemuSecurityDACSetOwnership(conn, disk->src, driver->user,
driver->group);
+}
+
+
+static int
+qemuSecurityDACRestoreSecurityImageLabel(virConnectPtr conn,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainDiskDefPtr disk)
+{
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ /* Don't restore labels on readoly/shared disks, because
+ * other VMs may still be accessing these
+ * Alternatively we could iterate over all running
+ * domains and try to figure out if it is in use, but
+ * this would not work for clustered filesystems, since
+ * we can't see running VMs using the file on other nodes
+ * Safest bet is thus to skip the restore step.
+ */
+ if (disk->readonly || disk->shared)
+ return 0;
+
+ if (!disk->src)
+ return 0;
+
+ return qemuSecurityDACRestoreSecurityFileLabel(conn, disk->src);
+}
+
+
+static int
+qemuSecurityDACSetSecurityPCILabel(virConnectPtr conn,
+ pciDevice *dev ATTRIBUTE_UNUSED,
+ const char *file,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+ return qemuSecurityDACSetOwnership(conn, file, driver->user, driver->group);
+}
+
+
+static int
+qemuSecurityDACSetSecurityUSBLabel(virConnectPtr conn,
+ usbDevice *dev ATTRIBUTE_UNUSED,
+ const char *file,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+ return qemuSecurityDACSetOwnership(conn, file, driver->user, driver->group);
+}
+
+
+static int
+qemuSecurityDACSetSecurityHostdevLabel(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virDomainHostdevDefPtr dev)
+
+{
+ int ret = -1;
+
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
+ return 0;
+
+ switch (dev->source.subsys.type) {
+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
+ usbDevice *usb = usbGetDevice(conn,
+ dev->source.subsys.u.usb.bus,
+ dev->source.subsys.u.usb.device,
+ dev->source.subsys.u.usb.vendor,
+ dev->source.subsys.u.usb.product);
+
+ if (!usb)
+ goto done;
+
+ ret = usbDeviceFileIterate(conn, usb, qemuSecurityDACSetSecurityUSBLabel, vm);
+ usbFreeDevice(conn, usb);
+ break;
+ }
+
+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
+ pciDevice *pci = pciGetDevice(conn,
+ dev->source.subsys.u.pci.domain,
+ dev->source.subsys.u.pci.bus,
+ dev->source.subsys.u.pci.slot,
+ dev->source.subsys.u.pci.function);
+
+ if (!pci)
+ goto done;
+
+ ret = pciDeviceFileIterate(conn, pci, qemuSecurityDACSetSecurityPCILabel, vm);
+ pciFreeDevice(conn, pci);
+
+ break;
+ }
+
+ default:
+ ret = 0;
+ break;
+ }
+
+done:
+ return ret;
+}
+
+
+static int
+qemuSecurityDACRestoreSecurityPCILabel(virConnectPtr conn,
+ pciDevice *dev ATTRIBUTE_UNUSED,
+ const char *file,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+ return qemuSecurityDACRestoreSecurityFileLabel(conn, file);
+}
+
+
+static int
+qemuSecurityDACRestoreSecurityUSBLabel(virConnectPtr conn,
+ usbDevice *dev ATTRIBUTE_UNUSED,
+ const char *file,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+ return qemuSecurityDACRestoreSecurityFileLabel(conn, file);
+}
+
+
+static int
+qemuSecurityDACRestoreSecurityHostdevLabel(virConnectPtr conn,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainHostdevDefPtr dev)
+
+{
+ int ret = -1;
+
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
+ return 0;
+
+ switch (dev->source.subsys.type) {
+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
+ usbDevice *usb = usbGetDevice(conn,
+ dev->source.subsys.u.usb.bus,
+ dev->source.subsys.u.usb.device,
+ dev->source.subsys.u.usb.vendor,
+ dev->source.subsys.u.usb.product);
+
+ if (!usb)
+ goto done;
+
+ ret = usbDeviceFileIterate(conn, usb, qemuSecurityDACRestoreSecurityUSBLabel,
NULL);
+ usbFreeDevice(conn, usb);
+
+ break;
+ }
+
+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
+ pciDevice *pci = pciGetDevice(conn,
+ dev->source.subsys.u.pci.domain,
+ dev->source.subsys.u.pci.bus,
+ dev->source.subsys.u.pci.slot,
+ dev->source.subsys.u.pci.function);
+
+ if (!pci)
+ goto done;
+
+ ret = pciDeviceFileIterate(conn, pci, qemuSecurityDACRestoreSecurityPCILabel,
NULL);
+ pciFreeDevice(conn, pci);
+
+ break;
+ }
+
+ default:
+ ret = 0;
+ break;
+ }
+
+done:
+ return ret;
+}
+
+
+static int
+qemuSecurityDACRestoreSecurityAllLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int i;
+ int rc = 0;
+
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ VIR_DEBUG("Restoring security label on %s", vm->def->name);
+
+ for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+ if (qemuSecurityDACRestoreSecurityHostdevLabel(conn, vm,
+ vm->def->hostdevs[i]) <
0)
+ rc = -1;
+ }
+ for (i = 0 ; i < vm->def->ndisks ; i++) {
+ if (qemuSecurityDACRestoreSecurityImageLabel(conn, vm,
+ vm->def->disks[i]) < 0)
+ rc = -1;
+ }
+ return rc;
+}
+
+
+static int
+qemuSecurityDACSetSecurityAllLabel(virConnectPtr conn,
+ virDomainObjPtr vm)
+{
+ int i;
+
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ for (i = 0 ; i < vm->def->ndisks ; i++) {
+ /* XXX fixme - we need to recursively label the entriy tree :-( */
+ if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
+ continue;
+ if (qemuSecurityDACSetSecurityImageLabel(conn, vm, vm->def->disks[i]) <
0)
+ return -1;
+ }
+ for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+ if (qemuSecurityDACSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i])
< 0)
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+qemuSecurityDACSetSavedStateLabel(virConnectPtr conn,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ const char *savefile)
+{
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ return qemuSecurityDACSetOwnership(conn, savefile, driver->user,
driver->group);
+}
+
+
+static int
+qemuSecurityDACRestoreSavedStateLabel(virConnectPtr conn,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ const char *savefile)
+{
+ if (!driver->privileged || !driver->dynamicOwnership)
+ return 0;
+
+ return qemuSecurityDACRestoreSecurityFileLabel(conn, savefile);
+}
+
+
+static int
+qemuSecurityDACSetProcessLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED)
+{
+ DEBUG("Dropping privileges of VM to %d:%d", driver->user,
driver->group);
+
+ if (!driver->privileged)
+ return 0;
+
+ if (driver->group) {
+ if (setregid(driver->group, driver->group) < 0) {
+ virReportSystemError(NULL, errno,
+ _("cannot change to '%d' group"),
+ driver->group);
+ return -1;
+ }
+ }
+ if (driver->user) {
+ if (setreuid(driver->user, driver->user) < 0) {
+ virReportSystemError(NULL, errno,
+ _("cannot change to '%d' user"),
+ driver->user);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
+
+virSecurityDriver virqemuSecurityDACSecurityDriver = {
+ .name = "qemuDAC",
+
+ .domainSetSecurityProcessLabel = qemuSecurityDACSetProcessLabel,
+
+ .domainSetSecurityImageLabel = qemuSecurityDACSetSecurityImageLabel,
+ .domainRestoreSecurityImageLabel = qemuSecurityDACRestoreSecurityImageLabel,
+
+ .domainSetSecurityAllLabel = qemuSecurityDACSetSecurityAllLabel,
+ .domainRestoreSecurityAllLabel = qemuSecurityDACRestoreSecurityAllLabel,
+
+ .domainSetSecurityHostdevLabel = qemuSecurityDACSetSecurityHostdevLabel,
+ .domainRestoreSecurityHostdevLabel = qemuSecurityDACRestoreSecurityHostdevLabel,
+
+ .domainSetSavedStateLabel = qemuSecurityDACSetSavedStateLabel,
+ .domainRestoreSavedStateLabel = qemuSecurityDACRestoreSavedStateLabel,
+};
diff --git a/src/qemu/qemu_security_dac.h b/src/qemu/qemu_security_dac.h
new file mode 100644
index 0000000..246ebe2
--- /dev/null
+++ b/src/qemu/qemu_security_dac.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * QEMU POSIX DAC security driver
+ */
+
+#include "security/security_driver.h"
+#include "qemu_conf.h"
+
+#ifndef __QEMU_SECURITY_DAC
+#define __QEMU_SECURITY_DAC
+
+extern virSecurityDriver qemuDACSecurityDriver;
+
+void qemuSecurityDACSetDriver(struct qemud_driver *driver);
+
+#endif /* __QEMU_SECURITY_DAC */
--
1.6.5.2
11:19 a.m.
New subject: [libvirt] [PATCH 06/12] Introduce a new DAC security driver for QEMU
On Wed, Jan 20, 2010 at 03:15:03PM +0000, Daniel P. Berrange wrote:
This new security driver is responsible for managing UID/GID changes
to the QEMU process, and any files/disks/devices assigned to it.
* qemu/qemu_conf.h: Add flag for disabling automatic file permission
changes
* qemu/qemu_security_dac.h, qemu/qemu_security_dac.c: New DAC driver
for QEMU guests
* Makefile.am: Add new files
---
po/POTFILES.in | 1 +
src/Makefile.am | 4 +-
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_security_dac.c | 458 ++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_security_dac.h | 22 ++
5 files changed, 485 insertions(+), 1 deletions(-)
create mode 100644 src/qemu/qemu_security_dac.c
create mode 100644 src/qemu/qemu_security_dac.h
[...]
+ * QEMU POSIX DAC security driver
+ */
+#include <config.h>
+#include <selinux/selinux.h>
+#include <selinux/context.h>
SELinux includes not needed there either I think
Otherwise looks fine, ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 07/12] Switch QEMU driver over to use the DAC security driver
Remove all the QEMU driver calls for setting file ownership and
process uid/gid. Instead wire in the QEMU DAC security driver,
stacking it ontop of the primary SELinux/AppArmour driver.
* qemu/qemu_driver.c: Switch over to new DAC security driver
---
src/qemu/qemu_driver.c | 311 ++----------------------------------------
src/qemu/qemu_security_dac.c | 2 +-
2 files changed, 13 insertions(+), 300 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8195b74..9554852 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -69,7 +69,8 @@
#include "pci.h"
#include "hostusb.h"
#include "processinfo.h"
-#include "security/security_driver.h"
+#include "qemu_security_stacked.h"
+#include "qemu_security_dac.h"
#include "cgroup.h"
#include "libvirt_internal.h"
#include "xml.h"
@@ -936,7 +937,12 @@ qemudSecurityInit(struct qemud_driver *qemud_drv)
return 0;
}
- qemud_drv->securityDriver = security_drv;
+ qemuSecurityStackedSetDriver(qemud_drv);
+ qemuSecurityDACSetDriver(qemud_drv);
+
+ qemud_drv->securityPrimaryDriver = security_drv;
+ qemud_drv->securitySecondaryDriver = &qemuDACSecurityDriver;
+ qemud_drv->securityDriver = &qemuStackedSecurityDriver;
VIR_INFO("Initialized security driver %s", security_drv->name);
@@ -2438,225 +2444,6 @@ cleanup:
}
-static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver,
virDomainObjPtr vm)
-{
- int rc = 0;
-
- if (driver->securityDriver &&
- driver->securityDriver->domainSetSecurityProcessLabel &&
- driver->securityDriver->domainSetSecurityProcessLabel(conn,
driver->securityDriver, vm) < 0)
- rc = -1;
-
- return rc;
-}
-
-
-#ifdef __linux__
-struct qemuFileOwner {
- uid_t uid;
- gid_t gid;
-};
-
-static int qemuDomainSetHostdevUSBOwnershipActor(virConnectPtr conn,
- usbDevice *dev ATTRIBUTE_UNUSED,
- const char *file, void *opaque)
-{
- struct qemuFileOwner *owner = opaque;
-
- VIR_DEBUG("Setting ownership on %s to %d:%d", file, owner->uid,
owner->gid);
-
- if (chown(file, owner->uid, owner->gid) < 0) {
- virReportSystemError(conn, errno, _("cannot set ownership on %s"),
file);
- return -1;
- }
-
- return 0;
-}
-
-static int qemuDomainSetHostdevUSBOwnership(virConnectPtr conn,
- virDomainHostdevDefPtr def,
- uid_t uid, gid_t gid)
-{
- struct qemuFileOwner owner = { uid, gid };
- int ret = -1;
-
- usbDevice *dev = usbGetDevice(conn,
- def->source.subsys.u.usb.bus,
- def->source.subsys.u.usb.device,
- def->source.subsys.u.usb.vendor,
- def->source.subsys.u.usb.product);
-
- if (!dev)
- goto cleanup;
-
- ret = usbDeviceFileIterate(conn, dev,
- qemuDomainSetHostdevUSBOwnershipActor, &owner);
-
- usbFreeDevice(conn, dev);
-cleanup:
- return ret;
-}
-
-static int qemuDomainSetHostdevPCIOwnershipActor(virConnectPtr conn,
- pciDevice *dev ATTRIBUTE_UNUSED,
- const char *file, void *opaque)
-{
- struct qemuFileOwner *owner = opaque;
-
- VIR_DEBUG("Setting ownership on %s to %d:%d", file, owner->uid,
owner->gid);
-
- if (chown(file, owner->uid, owner->gid) < 0) {
- virReportSystemError(conn, errno, _("cannot set ownership on %s"),
file);
- return -1;
- }
-
- return 0;
-}
-
-static int qemuDomainSetHostdevPCIOwnership(virConnectPtr conn,
- virDomainHostdevDefPtr def,
- uid_t uid, gid_t gid)
-{
- struct qemuFileOwner owner = { uid, gid };
- int ret = -1;
-
- pciDevice *dev = pciGetDevice(conn,
- def->source.subsys.u.pci.domain,
- def->source.subsys.u.pci.bus,
- def->source.subsys.u.pci.slot,
- def->source.subsys.u.pci.function);
-
- if (!dev)
- goto cleanup;
-
- ret = pciDeviceFileIterate(conn, dev,
- qemuDomainSetHostdevPCIOwnershipActor, &owner);
-
- pciFreeDevice(conn, dev);
-cleanup:
- return ret;
-}
-#endif
-
-
-static int qemuDomainSetHostdevOwnership(virConnectPtr conn,
- virDomainHostdevDefPtr def,
- uid_t uid, gid_t gid)
-{
- if (def->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
- return 0;
-
-#ifdef __linux__
- switch (def->source.subsys.type) {
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- return qemuDomainSetHostdevUSBOwnership(conn, def, uid, gid);
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- return qemuDomainSetHostdevPCIOwnership(conn, def, uid, gid);
-
- }
- return 0;
-#else
- qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, "%s",
- _("unable to set host device ownership on this
platform"));
- return -1;
-#endif
-
-}
-
-static int qemuDomainSetFileOwnership(virConnectPtr conn,
- const char *path,
- uid_t uid, gid_t gid)
-{
-
- if (!path)
- return 0;
-
- VIR_DEBUG("Setting ownership on %s to %d:%d", path, uid, gid);
- if (chown(path, uid, gid) < 0) {
- virReportSystemError(conn, errno, _("cannot set ownership on %s"),
- path);
- return -1;
- }
- return 0;
-}
-
-static int qemuDomainSetDeviceOwnership(virConnectPtr conn,
- struct qemud_driver *driver,
- virDomainDeviceDefPtr def,
- int restore)
-{
- uid_t uid;
- gid_t gid;
-
- if (!driver->privileged)
- return 0;
-
- /* short circuit case of root:root */
- if (!driver->user && !driver->group)
- return 0;
-
- uid = restore ? 0 : driver->user;
- gid = restore ? 0 : driver->group;
-
- switch (def->type) {
- case VIR_DOMAIN_DEVICE_DISK:
- if (restore &&
- (def->data.disk->readonly || def->data.disk->shared))
- return 0;
-
- return qemuDomainSetFileOwnership(conn, def->data.disk->src, uid, gid);
-
- case VIR_DOMAIN_DEVICE_HOSTDEV:
- return qemuDomainSetHostdevOwnership(conn, def->data.hostdev, uid, gid);
- }
-
- return 0;
-}
-
-static int qemuDomainSetAllDeviceOwnership(virConnectPtr conn,
- struct qemud_driver *driver,
- virDomainDefPtr def,
- int restore)
-{
- int i;
- uid_t uid;
- gid_t gid;
-
- if (!driver->privileged)
- return 0;
-
- /* short circuit case of root:root */
- if (!driver->user && !driver->group)
- return 0;
-
- uid = restore ? 0 : driver->user;
- gid = restore ? 0 : driver->group;
-
- if (qemuDomainSetFileOwnership(conn, def->os.kernel, uid, gid) < 0 ||
- qemuDomainSetFileOwnership(conn, def->os.initrd, uid, gid) < 0)
- return -1;
-
- for (i = 0 ; i < def->ndisks ; i++) {
- if (restore &&
- (def->disks[i]->readonly || def->disks[i]->shared))
- continue;
-
- if (qemuDomainSetFileOwnership(conn, def->disks[i]->src, uid, gid) < 0)
- return -1;
- }
-
- for (i = 0 ; i < def->nhostdevs ; i++) {
- if (qemuDomainSetHostdevOwnership(conn, def->hostdevs[i], uid, gid) < 0)
- return -1;
- }
-
- return 0;
-}
-
-static virDomainPtr qemudDomainLookupByName(virConnectPtr conn,
- const char *name);
-
struct qemudHookData {
virConnectPtr conn;
virDomainObjPtr vm;
@@ -2669,33 +2456,11 @@ static int qemudSecurityHook(void *data) {
if (qemuAddToCgroup(h->driver, h->vm->def) < 0)
return -1;
- if (qemudDomainSetSecurityLabel(h->conn, h->driver, h->vm) < 0)
+ if (h->driver->securityDriver &&
+ h->driver->securityDriver->domainSetSecurityProcessLabel &&
+ h->driver->securityDriver->domainSetSecurityProcessLabel(h->conn,
h->driver->securityDriver, h->vm) < 0)
return -1;
- if (h->driver->privileged) {
- if (qemuDomainSetAllDeviceOwnership(h->conn, h->driver, h->vm->def,
0) < 0)
- return -1;
-
- DEBUG("Dropping privileges of VM to %d:%d", h->driver->user,
h->driver->group);
-
- if (h->driver->group) {
- if (setregid(h->driver->group, h->driver->group) < 0) {
- virReportSystemError(NULL, errno,
- _("cannot change to '%d' group"),
- h->driver->group);
- return -1;
- }
- }
- if (h->driver->user) {
- if (setreuid(h->driver->user, h->driver->user) < 0) {
- virReportSystemError(NULL, errno,
- _("cannot change to '%d' user"),
- h->driver->user);
- return -1;
- }
- }
- }
-
return 0;
}
@@ -3078,10 +2843,6 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
virDomainDefClearPCIAddresses(vm->def);
virDomainDefClearDeviceAliases(vm->def);
- if (qemuDomainSetAllDeviceOwnership(conn, driver, vm->def, 1) < 0)
- VIR_WARN("Failed to restore all device ownership for %s",
- vm->def->name);
-
qemuDomainReAttachHostDevices(conn, driver, vm->def);
retry:
@@ -4132,14 +3893,6 @@ static int qemudDomainSave(virDomainPtr dom,
}
fd = -1;
- if (driver->privileged &&
- chown(path, driver->user, driver->group) < 0) {
- virReportSystemError(NULL, errno,
- _("unable to set ownership of '%s' to user
%d:%d"),
- path, driver->user, driver->group);
- goto endjob;
- }
-
if (driver->securityDriver &&
driver->securityDriver->domainSetSavedStateLabel &&
driver->securityDriver->domainSetSavedStateLabel(dom->conn, vm, path) ==
-1)
@@ -4167,14 +3920,6 @@ static int qemudDomainSave(virDomainPtr dom,
if (rc < 0)
goto endjob;
- if (driver->privileged &&
- chown(path, 0, 0) < 0) {
- virReportSystemError(NULL, errno,
- _("unable to set ownership of '%s' to user
%d:%d"),
- path, 0, 0);
- goto endjob;
- }
-
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
@@ -4262,14 +4007,6 @@ static int qemudDomainCoreDump(virDomainPtr dom,
goto endjob;
}
- if (driver->privileged &&
- chown(path, driver->user, driver->group) < 0) {
- virReportSystemError(NULL, errno,
- _("unable to set ownership of '%s' to user
%d:%d"),
- path, driver->user, driver->group);
- goto endjob;
- }
-
if (driver->securityDriver &&
driver->securityDriver->domainSetSavedStateLabel &&
driver->securityDriver->domainSetSavedStateLabel(dom->conn, vm, path) ==
-1)
@@ -4297,14 +4034,6 @@ static int qemudDomainCoreDump(virDomainPtr dom,
qemuDomainObjExitMonitor(vm);
paused |= (ret == 0);
- if (driver->privileged &&
- chown(path, 0, 0) < 0) {
- virReportSystemError(NULL, errno,
- _("unable to set ownership of '%s' to user
%d:%d"),
- path, 0, 0);
- goto endjob;
- }
-
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSavedStateLabel &&
driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm,
path) == -1)
@@ -5879,8 +5608,6 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
return -1;
}
- if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0)
- return -1;
if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
@@ -5963,9 +5690,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
- if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
- goto endjob;
-
ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm, dev);
break;
@@ -5974,9 +5698,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
driver->securityDriver->domainSetSecurityImageLabel)
driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
- if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
- goto endjob;
-
if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm,
dev);
} else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
@@ -6032,11 +5753,8 @@ cleanup:
if (cgroup)
virCgroupFree(&cgroup);
- if (ret < 0 && dev != NULL) {
- if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
- VIR_WARN0("Fail to restore disk device ownership");
+ if (ret < 0 && dev != NULL)
virDomainDeviceDefFree(dev);
- }
if (vm)
virDomainObjUnlock(vm);
qemuDriverUnlock(driver);
@@ -6365,9 +6083,6 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,
driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
VIR_WARN0("Failed to restore device labelling");
- if (qemuDomainSetDeviceOwnership(conn, driver, dev, 1) < 0)
- VIR_WARN0("Failed to restore device ownership");
-
return ret;
}
@@ -6410,8 +6125,6 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityImageLabel)
driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn,
vm, dev->data.disk);
- if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
- VIR_WARN0("Fail to restore disk device ownership");
} else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
ret = qemudDomainDetachNetDevice(dom->conn, driver, vm, dev);
} else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c
index 6b7aab5..623aa55 100644
--- a/src/qemu/qemu_security_dac.c
+++ b/src/qemu/qemu_security_dac.c
@@ -439,7 +439,7 @@ qemuSecurityDACSetProcessLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
-virSecurityDriver virqemuSecurityDACSecurityDriver = {
+virSecurityDriver qemuDACSecurityDriver = {
.name = "qemuDAC",
.domainSetSecurityProcessLabel = qemuSecurityDACSetProcessLabel,
--
1.6.5.2
11:21 a.m.
New subject: [libvirt] [PATCH 07/12] Switch QEMU driver over to use the DAC security driver
On Wed, Jan 20, 2010 at 03:15:04PM +0000, Daniel P. Berrange wrote:
Remove all the QEMU driver calls for setting file ownership and
process uid/gid. Instead wire in the QEMU DAC security driver,
stacking it ontop of the primary SELinux/AppArmour driver.
* qemu/qemu_driver.c: Switch over to new DAC security driver
---
src/qemu/qemu_driver.c | 311 ++----------------------------------------
src/qemu/qemu_security_dac.c | 2 +-
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 08/12] Add missing call to re-attach host devices if VM startup fails
If there is a problem with VM startup, PCI devices may be left
assigned to pci-stub / pci-back. Adding a call to reattach
host devices in the cleanup path is required.
* qemu/qemu_driver.c: qemuDomainReAttachHostDevices() when
VM startup fails
---
src/qemu/qemu_driver.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9554852..22b6adc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2742,6 +2742,8 @@ cleanup:
/* We jump here if we failed to start the VM for any reason
* XXX investigate if we can kill this block and safely call
* qemudShutdownVMDaemon even though no PID is running */
+ qemuDomainReAttachHostDevices(conn, driver, vm->def);
+
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityAllLabel)
driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);
--
1.6.5.2
11:21 a.m.
New subject: [libvirt] [PATCH 08/12] Add missing call to re-attach host devices if VM startup fails
On Wed, Jan 20, 2010 at 03:15:05PM +0000, Daniel P. Berrange wrote:
If there is a problem with VM startup, PCI devices may be left
assigned to pci-stub / pci-back. Adding a call to reattach
host devices in the cleanup path is required.
* qemu/qemu_driver.c: qemuDomainReAttachHostDevices() when
VM startup fails
---
src/qemu/qemu_driver.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9554852..22b6adc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2742,6 +2742,8 @@ cleanup:
/* We jump here if we failed to start the VM for any reason
* XXX investigate if we can kill this block and safely call
* qemudShutdownVMDaemon even though no PID is running */
+ qemuDomainReAttachHostDevices(conn, driver, vm->def);
+
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityAllLabel)
driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 09/12] Fix security driver calls in hotplug cleanup paths
The hotplug code was not correctly invoking the security driver
in error paths. If a hotplug attempt failed, the device would
be left with VM permissions applied, rather than restored to the
original permissions. Also, a CDROM media that is ejected was
not restored to original permissions. Finally there was a bogus
call to set hostdev permissions in the hostdev unplug code
* qemu/qemu_driver.c: Fix security driver usage in hotplug/unplug
---
src/qemu/qemu_driver.c | 177 +++++++++++++++++++++++++++++++++---------------
1 files changed, 123 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 22b6adc..5054bcf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5126,6 +5126,11 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
return -1;
}
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel &&
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm, newdisk) <
0)
+ return -1;
+
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
if (newdisk->src) {
@@ -5144,14 +5149,27 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (ret == 0) {
- VIR_FREE(origdisk->src);
- origdisk->src = newdisk->src;
- newdisk->src = NULL;
- origdisk->type = newdisk->type;
- }
+ if (ret < 0)
+ goto error;
+
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, origdisk)
< 0)
+ VIR_WARN("Unable to restore security label on ejected image %s",
origdisk->src);
+
+ VIR_FREE(origdisk->src);
+ origdisk->src = newdisk->src;
+ newdisk->src = NULL;
+ origdisk->type = newdisk->type;
return ret;
+
+error:
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, newdisk)
< 0)
+ VIR_WARN("Unable to restore security label on new media %s",
newdisk->src);
+ return -1;
}
@@ -5173,9 +5191,14 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn,
}
}
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel &&
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
+ return -1;
+
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
virReportOOMError(conn);
- return -1;
+ goto error;
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
@@ -5185,13 +5208,22 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn,
&guestAddr);
qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (ret == 0) {
- dev->data.disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
- memcpy(&dev->data.disk->info.addr.pci, &guestAddr,
sizeof(guestAddr));
- virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
- }
+ if (ret < 0)
+ goto error;
- return ret;
+ dev->data.disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+ memcpy(&dev->data.disk->info.addr.pci, &guestAddr, sizeof(guestAddr));
+ virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
+
+ return 0;
+
+error:
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
+ VIR_WARN("Unable to restore security label on %s",
dev->data.disk->src);
+
+ return -1;
}
static int qemudDomainAttachPciControllerDevice(virConnectPtr conn,
@@ -5285,15 +5317,21 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
if (STREQ(vm->def->disks[i]->dst, dev->dst)) {
qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
_("target %s already exists"), dev->dst);
- goto cleanup;
+ return -1;
}
}
+
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel &&
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev) < 0)
+ return -1;
+
/* This func allocates the bus/unit IDs so must be before
* we search for controller
*/
if (!(drivestr = qemuBuildDriveStr(dev, 0, qemuCmdFlags)))
- goto cleanup;
+ goto error;
/* We should have an adddress now, so make sure */
@@ -5301,24 +5339,24 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_("unexpected disk address type %s"),
virDomainDeviceAddressTypeToString(dev->info.type));
- goto cleanup;
+ goto error;
}
for (i = 0 ; i < dev->info.addr.drive.controller ; i++) {
cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i);
if (!cont)
- goto cleanup;
+ goto error;
}
if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_("SCSI controller %d was missing its PCI address"),
cont->idx);
- goto cleanup;
+ goto error;
}
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
virReportOOMError(conn);
- goto cleanup;
+ goto error;
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
@@ -5326,20 +5364,28 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
drivestr,
&cont->info.addr.pci,
&driveAddr);
-
qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (ret == 0) {
- /* XXX we should probably validate that the addr matches
- * our existing defined addr instead of overwriting */
- dev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
- memcpy(&dev->info.addr.drive, &driveAddr, sizeof(driveAddr));
- virDomainDiskInsertPreAlloced(vm->def, dev);
- }
+ if (ret < 0)
+ goto error;
-cleanup:
+ /* XXX we should probably validate that the addr matches
+ * our existing defined addr instead of overwriting */
+ dev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+ memcpy(&dev->info.addr.drive, &driveAddr, sizeof(driveAddr));
+ virDomainDiskInsertPreAlloced(vm->def, dev);
VIR_FREE(drivestr);
- return ret;
+
+ return 0;
+
+error:
+ VIR_FREE(drivestr);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev) <
0)
+ VIR_WARN("Unable to restore security label on %s", dev->src);
+
+ return -1;
}
@@ -5359,27 +5405,43 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr
conn,
}
}
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSecurityImageLabel &&
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
+ return -1;
+
if (!dev->data.disk->src) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("disk source path is missing"));
- return -1;
+ goto error;
}
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
virReportOOMError(conn);
- return -1;
+ goto error;
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
ret = qemuMonitorAddUSBDisk(priv->mon, dev->data.disk->src);
qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (ret == 0)
- virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
+ if (ret < 0)
+ goto error;
- return ret;
+ virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
+
+ return 0;
+
+error:
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
+ VIR_WARN("Unable to restore security label on %s",
dev->data.disk->src);
+
+ return -1;
}
+
static int qemudDomainAttachNetDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
@@ -5617,15 +5679,31 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
switch (hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- return qemudDomainAttachHostPciDevice(conn, driver, vm, dev);
+ if (qemudDomainAttachHostPciDevice(conn, driver, vm, dev) < 0)
+ goto error;
+ break;
+
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- return qemudDomainAttachHostUsbDevice(conn, driver, vm, dev);
+ if (qemudDomainAttachHostUsbDevice(conn, driver, vm, dev) < 0)
+ goto error;
+ break;
+
default:
qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT,
_("hostdev subsys type '%s' not supported"),
virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
- return -1;
+ goto error;
}
+
+ return 0;
+
+error:
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityHostdevLabel &&
+ driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
+ VIR_WARN0("Unable to restore host device labelling on hotplug fail");
+
+ return -1;
}
static int qemudDomainAttachDevice(virDomainPtr dom,
@@ -5688,18 +5766,10 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
switch (dev->data.disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- if (driver->securityDriver &&
- driver->securityDriver->domainSetSecurityImageLabel)
- driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
-
ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm, dev);
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
- if (driver->securityDriver &&
- driver->securityDriver->domainSetSecurityImageLabel)
- driver->securityDriver->domainSetSecurityImageLabel(dom->conn,
vm, dev->data.disk);
-
if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm,
dev);
} else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
@@ -5815,6 +5885,11 @@ static int qemudDomainDetachPciDiskDevice(virConnectPtr conn,
}
virDomainDiskDefFree(detach);
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSecurityImageLabel &&
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
+ VIR_WARN("Unable to restore security label on %s",
dev->data.disk->src);
+
ret = 0;
cleanup:
@@ -6081,9 +6156,9 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,
}
if (driver->securityDriver &&
- driver->securityDriver->domainSetSecurityHostdevLabel &&
- driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
- VIR_WARN0("Failed to restore device labelling");
+ driver->securityDriver->domainRestoreSecurityHostdevLabel &&
+ driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
+ VIR_WARN0("Failed to restore host device labelling");
return ret;
}
@@ -6124,9 +6199,6 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev);
- if (driver->securityDriver &&
- driver->securityDriver->domainRestoreSecurityImageLabel)
- driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn,
vm, dev->data.disk);
} else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
ret = qemudDomainDetachNetDevice(dom->conn, driver, vm, dev);
} else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
@@ -6140,9 +6212,6 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
}
} else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev);
- if (driver->securityDriver &&
- driver->securityDriver->domainRestoreSecurityHostdevLabel)
- driver->securityDriver->domainRestoreSecurityHostdevLabel(dom->conn,
vm, dev->data.hostdev);
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("This type of device cannot be hot
unplugged"));
--
1.6.5.2
11:37 a.m.
New subject: [libvirt] [PATCH 09/12] Fix security driver calls in hotplug cleanup paths
On Wed, Jan 20, 2010 at 03:15:06PM +0000, Daniel P. Berrange wrote:
The hotplug code was not correctly invoking the security driver
in error paths. If a hotplug attempt failed, the device would
be left with VM permissions applied, rather than restored to the
original permissions. Also, a CDROM media that is ejected was
not restored to original permissions. Finally there was a bogus
call to set hostdev permissions in the hostdev unplug code
* qemu/qemu_driver.c: Fix security driver usage in hotplug/unplug
---
src/qemu/qemu_driver.c | 177 +++++++++++++++++++++++++++++++++---------------
1 files changed, 123 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 22b6adc..5054bcf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5126,6 +5126,11 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
Okay the refactoring for error handling of the functions is not trivial
to follow but looks fine,
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 10/12] Add configuration option to turn off dynamic permissions management
Add the ability to turn off dynamic management of file permissions
for libvirt guests.
* qemu/libvirtd_qemu.aug: Support 'dynamic_ownership' flag
* qemu/qemu.conf: Document 'dynamic_ownership' flag.
* qemu/qemu_conf.c: Load 'dynamic_ownership' flag
* qemu/test_libvirtd_qemu.aug: Test 'dynamic_ownership' flag
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 5 +++++
src/qemu/qemu_conf.c | 13 ++++++++++---
src/qemu/test_libvirtd_qemu.aug | 4 ++++
4 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index f0b2a5e..b71e4cf 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -32,6 +32,7 @@ module Libvirtd_qemu =
| str_entry "security_driver"
| str_entry "user"
| str_entry "group"
+ | bool_entry "dynamic_ownership"
| str_array_entry "cgroup_controllers"
| str_array_entry "cgroup_device_acl"
| str_entry "save_image_format"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 2129bae..c662893 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -96,6 +96,11 @@
# The group ID for QEMU processes run by the system instance
#group = "root"
+# Whether libvirt should dynamically change file ownership
+# to match the configured user/group above. Defaults to 1.
+# Set to 0 to disable file ownership changes.
+#dynamic_ownership = 1
+
# What cgroup controllers to make use of with QEMU guests
#
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2cefa6a..34c6fdb 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -102,7 +102,9 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
char *group;
int i;
- /* Setup 2 critical defaults */
+ /* Setup critical defaults */
+ driver->dynamicOwnership = 1;
+
if (!(driver->vncListen = strdup("127.0.0.1"))) {
virReportOOMError(NULL);
return -1;
@@ -224,6 +226,7 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
}
VIR_FREE(user);
+
p = virConfGetValue (conf, "group");
CHECK_TYPE ("group", VIR_CONF_STRING);
if (!(group = strdup(p && p->str ? p->str : QEMU_GROUP))) {
@@ -231,8 +234,6 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
virConfFree(conf);
return -1;
}
-
-
if (virGetGroupID(NULL, group, &driver->group) < 0) {
VIR_FREE(group);
virConfFree(conf);
@@ -240,6 +241,12 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
}
VIR_FREE(group);
+
+ p = virConfGetValue (conf, "dynamic_ownership");
+ CHECK_TYPE ("dynamic_ownership", VIR_CONF_LONG);
+ if (p) driver->dynamicOwnership = p->l;
+
+
p = virConfGetValue (conf, "cgroup_controllers");
CHECK_TYPE ("cgroup_controllers", VIR_CONF_LIST);
if (p) {
diff --git a/src/qemu/test_libvirtd_qemu.aug b/src/qemu/test_libvirtd_qemu.aug
index ac89438..3ce47f3 100644
--- a/src/qemu/test_libvirtd_qemu.aug
+++ b/src/qemu/test_libvirtd_qemu.aug
@@ -86,6 +86,8 @@ user = \"root\"
group = \"root\"
+dynamic_ownership = 1
+
cgroup_controllers = [ \"cpu\", \"devices\" ]
cgroup_device_acl = [ \"/dev/null\", \"/dev/full\",
\"/dev/zero\" ]
@@ -182,6 +184,8 @@ hugetlbfs_mount = \"/dev/hugepages\"
{ "#empty" }
{ "group" = "root" }
{ "#empty" }
+{ "dynamic_ownership" = "1" }
+{ "#empty" }
{ "cgroup_controllers"
{ "1" = "cpu" }
{ "2" = "devices" }
--
1.6.5.2
11:42 a.m.
New subject: [libvirt] [PATCH 10/12] Add configuration option to turn off dynamic permissions management
On Wed, Jan 20, 2010 at 03:15:07PM +0000, Daniel P. Berrange wrote:
Add the ability to turn off dynamic management of file permissions
for libvirt guests.
* qemu/libvirtd_qemu.aug: Support 'dynamic_ownership' flag
* qemu/qemu.conf: Document 'dynamic_ownership' flag.
* qemu/qemu_conf.c: Load 'dynamic_ownership' flag
* qemu/test_libvirtd_qemu.aug: Test 'dynamic_ownership' flag
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 5 +++++
src/qemu/qemu_conf.c | 13 ++++++++++---
src/qemu/test_libvirtd_qemu.aug | 4 ++++
4 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index f0b2a5e..b71e4cf 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -32,6 +32,7 @@ module Libvirtd_qemu =
| str_entry "security_driver"
| str_entry "user"
| str_entry "group"
+ | bool_entry "dynamic_ownership"
| str_array_entry "cgroup_controllers"
| str_array_entry "cgroup_device_acl"
| str_entry "save_image_format"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 2129bae..c662893 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -96,6 +96,11 @@
# The group ID for QEMU processes run by the system instance
#group = "root"
+# Whether libvirt should dynamically change file ownership
+# to match the configured user/group above. Defaults to 1.
+# Set to 0 to disable file ownership changes.
+#dynamic_ownership = 1
+
# What cgroup controllers to make use of with QEMU guests
#
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2cefa6a..34c6fdb 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -102,7 +102,9 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
char *group;
int i;
- /* Setup 2 critical defaults */
+ /* Setup critical defaults */
+ driver->dynamicOwnership = 1;
+
if (!(driver->vncListen = strdup("127.0.0.1"))) {
virReportOOMError(NULL);
return -1;
@@ -224,6 +226,7 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
}
VIR_FREE(user);
+
p = virConfGetValue (conf, "group");
CHECK_TYPE ("group", VIR_CONF_STRING);
if (!(group = strdup(p && p->str ? p->str : QEMU_GROUP))) {
@@ -231,8 +234,6 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
virConfFree(conf);
return -1;
}
-
-
if (virGetGroupID(NULL, group, &driver->group) < 0) {
VIR_FREE(group);
virConfFree(conf);
@@ -240,6 +241,12 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
}
VIR_FREE(group);
+
+ p = virConfGetValue (conf, "dynamic_ownership");
+ CHECK_TYPE ("dynamic_ownership", VIR_CONF_LONG);
+ if (p) driver->dynamicOwnership = p->l;
+
Hum, I was wondering about the value space for dynamicOwnership, it's
defined as an int but so far we just use the !driver->dynamicOwnership
test. So that looks fine, but I wonder if in the future we may not
extend this to take more values for example depending on the type of
devices.
ACK
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 11/12] Fix leak in hotplug code in QEMU driver
The hotplug code in QEMU was leaking memory because although the
inner device object was being moved into the main virDomainDefPtr
config object, the outer container virDomainDeviceDefPtr was not.
* src/qemu/qemu_driver.c: Clarify code to show that the inner
device object is owned by the main domain config upon
successfull attach.
---
src/qemu/qemu_driver.c | 190 ++++++++++++++++++++++++++----------------------
1 files changed, 103 insertions(+), 87 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5054bcf..cb6fe86 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5088,17 +5088,16 @@ cleanup:
static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev)
+ virDomainDiskDefPtr disk)
{
- virDomainDiskDefPtr origdisk = NULL, newdisk;
+ virDomainDiskDefPtr origdisk = NULL;
int i;
int ret;
origdisk = NULL;
- newdisk = dev->data.disk;
for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (vm->def->disks[i]->bus == newdisk->bus &&
- STREQ(vm->def->disks[i]->dst, newdisk->dst)) {
+ if (vm->def->disks[i]->bus == disk->bus &&
+ STREQ(vm->def->disks[i]->dst, disk->dst)) {
origdisk = vm->def->disks[i];
break;
}
@@ -5107,8 +5106,8 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
if (!origdisk) {
qemudReportError(conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
_("No device with bus '%s' and target
'%s'"),
- virDomainDiskBusTypeToString(newdisk->bus),
- newdisk->dst);
+ virDomainDiskBusTypeToString(disk->bus),
+ disk->dst);
return -1;
}
@@ -5122,28 +5121,28 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
origdisk->device != VIR_DOMAIN_DISK_DEVICE_CDROM) {
qemudReportError(conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
_("Removable media not supported for %s device"),
- virDomainDiskDeviceTypeToString(newdisk->device));
+ virDomainDiskDeviceTypeToString(disk->device));
return -1;
}
if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityImageLabel &&
- driver->securityDriver->domainSetSecurityImageLabel(conn, vm, newdisk) <
0)
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) <
0)
return -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- if (newdisk->src) {
+ if (disk->src) {
const char *format = NULL;
- if (newdisk->type != VIR_DOMAIN_DISK_TYPE_DIR) {
- if (newdisk->driverType)
- format = newdisk->driverType;
+ if (disk->type != VIR_DOMAIN_DISK_TYPE_DIR) {
+ if (disk->driverType)
+ format = disk->driverType;
else if (origdisk->driverType)
format = origdisk->driverType;
}
ret = qemuMonitorChangeMedia(priv->mon,
origdisk->info.alias,
- newdisk->src, format);
+ disk->src, format);
} else {
ret = qemuMonitorEjectMedia(priv->mon, origdisk->info.alias);
}
@@ -5158,17 +5157,19 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
VIR_WARN("Unable to restore security label on ejected image %s",
origdisk->src);
VIR_FREE(origdisk->src);
- origdisk->src = newdisk->src;
- newdisk->src = NULL;
- origdisk->type = newdisk->type;
+ origdisk->src = disk->src;
+ disk->src = NULL;
+ origdisk->type = disk->type;
+
+ virDomainDiskDefFree(disk);
return ret;
error:
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityImageLabel &&
- driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, newdisk)
< 0)
- VIR_WARN("Unable to restore security label on new media %s",
newdisk->src);
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk)
< 0)
+ VIR_WARN("Unable to restore security label on new media %s",
disk->src);
return -1;
}
@@ -5176,24 +5177,24 @@ error:
static int qemudDomainAttachPciDiskDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev)
+ virDomainDiskDefPtr disk)
{
int i, ret;
- const char* type = virDomainDiskBusTypeToString(dev->data.disk->bus);
+ const char* type = virDomainDiskBusTypeToString(disk->bus);
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDevicePCIAddress guestAddr;
for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) {
+ if (STREQ(vm->def->disks[i]->dst, disk->dst)) {
qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
- _("target %s already exists"),
dev->data.disk->dst);
+ _("target %s already exists"), disk->dst);
return -1;
}
}
if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityImageLabel &&
- driver->securityDriver->domainSetSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) <
0)
return -1;
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
@@ -5203,7 +5204,7 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn,
qemuDomainObjEnterMonitorWithDriver(driver, vm);
ret = qemuMonitorAddPCIDisk(priv->mon,
- dev->data.disk->src,
+ disk->src,
type,
&guestAddr);
qemuDomainObjExitMonitorWithDriver(driver, vm);
@@ -5211,36 +5212,37 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn,
if (ret < 0)
goto error;
- dev->data.disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
- memcpy(&dev->data.disk->info.addr.pci, &guestAddr, sizeof(guestAddr));
- virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
+ disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+ memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr));
+ virDomainDiskInsertPreAlloced(vm->def, disk);
return 0;
error:
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityImageLabel &&
- driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
- VIR_WARN("Unable to restore security label on %s",
dev->data.disk->src);
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk)
< 0)
+ VIR_WARN("Unable to restore security label on %s", disk->src);
return -1;
}
+
static int qemudDomainAttachPciControllerDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainControllerDefPtr def)
+ virDomainControllerDefPtr controller)
{
int i, ret;
- const char* type = virDomainControllerTypeToString(def->type);
+ const char* type = virDomainControllerTypeToString(controller->type);
qemuDomainObjPrivatePtr priv = vm->privateData;
for (i = 0 ; i < vm->def->ncontrollers ; i++) {
- if ((vm->def->controllers[i]->type == def->type) &&
- (vm->def->controllers[i]->idx == def->idx)) {
+ if ((vm->def->controllers[i]->type == controller->type) &&
+ (vm->def->controllers[i]->idx == controller->idx)) {
qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
_("target %s:%d already exists"),
- type, def->idx);
+ type, controller->idx);
return -1;
}
}
@@ -5253,17 +5255,18 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr
conn,
qemuDomainObjEnterMonitorWithDriver(driver, vm);
ret = qemuMonitorAttachPCIDiskController(priv->mon,
type,
- &def->info.addr.pci);
+ &controller->info.addr.pci);
qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret == 0) {
- def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
- virDomainControllerInsertPreAlloced(vm->def, def);
+ controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+ virDomainControllerInsertPreAlloced(vm->def, controller);
}
return ret;
}
+
static virDomainControllerDefPtr
qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn,
struct qemud_driver *driver,
@@ -5300,10 +5303,11 @@ qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn,
return cont;
}
+
static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDiskDefPtr dev,
+ virDomainDiskDefPtr disk,
int qemuCmdFlags)
{
int i;
@@ -5314,9 +5318,9 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
int ret = -1;
for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (STREQ(vm->def->disks[i]->dst, dev->dst)) {
+ if (STREQ(vm->def->disks[i]->dst, disk->dst)) {
qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
- _("target %s already exists"), dev->dst);
+ _("target %s already exists"), disk->dst);
return -1;
}
}
@@ -5324,25 +5328,25 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityImageLabel &&
- driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev) < 0)
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) <
0)
return -1;
/* This func allocates the bus/unit IDs so must be before
* we search for controller
*/
- if (!(drivestr = qemuBuildDriveStr(dev, 0, qemuCmdFlags)))
+ if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags)))
goto error;
/* We should have an adddress now, so make sure */
- if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
+ if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_("unexpected disk address type %s"),
- virDomainDeviceAddressTypeToString(dev->info.type));
+ virDomainDeviceAddressTypeToString(disk->info.type));
goto error;
}
- for (i = 0 ; i < dev->info.addr.drive.controller ; i++) {
+ for (i = 0 ; i < disk->info.addr.drive.controller ; i++) {
cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i);
if (!cont)
goto error;
@@ -5371,9 +5375,9 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
/* XXX we should probably validate that the addr matches
* our existing defined addr instead of overwriting */
- dev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
- memcpy(&dev->info.addr.drive, &driveAddr, sizeof(driveAddr));
- virDomainDiskInsertPreAlloced(vm->def, dev);
+ disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+ memcpy(&disk->info.addr.drive, &driveAddr, sizeof(driveAddr));
+ virDomainDiskInsertPreAlloced(vm->def, disk);
VIR_FREE(drivestr);
return 0;
@@ -5382,8 +5386,8 @@ error:
VIR_FREE(drivestr);
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityImageLabel &&
- driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev) <
0)
- VIR_WARN("Unable to restore security label on %s", dev->src);
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk)
< 0)
+ VIR_WARN("Unable to restore security label on %s", disk->src);
return -1;
}
@@ -5392,25 +5396,25 @@ error:
static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev)
+ virDomainDiskDefPtr disk)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
int i, ret;
for (i = 0 ; i < vm->def->ndisks ; i++) {
- if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) {
+ if (STREQ(vm->def->disks[i]->dst, disk->dst)) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
- _("target %s already exists"),
dev->data.disk->dst);
+ _("target %s already exists"), disk->dst);
return -1;
}
}
if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityImageLabel &&
- driver->securityDriver->domainSetSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
+ driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) <
0)
return -1;
- if (!dev->data.disk->src) {
+ if (!disk->src) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("disk source path is missing"));
goto error;
@@ -5422,21 +5426,21 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr
conn,
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- ret = qemuMonitorAddUSBDisk(priv->mon, dev->data.disk->src);
+ ret = qemuMonitorAddUSBDisk(priv->mon, disk->src);
qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret < 0)
goto error;
- virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
+ virDomainDiskInsertPreAlloced(vm->def, disk);
return 0;
error:
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityImageLabel &&
- driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm,
dev->data.disk) < 0)
- VIR_WARN("Unable to restore security label on %s",
dev->data.disk->src);
+ driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk)
< 0)
+ VIR_WARN("Unable to restore security label on %s", disk->src);
return -1;
}
@@ -5445,10 +5449,9 @@ error:
static int qemudDomainAttachNetDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
+ virDomainNetDefPtr net,
unsigned int qemuCmdFlags)
{
- virDomainNetDefPtr net = dev->data.net;
qemuDomainObjPrivatePtr priv = vm->privateData;
char *tapfd_name = NULL;
int i, tapfd = -1;
@@ -5572,13 +5575,13 @@ no_memory:
goto cleanup;
}
+
static int qemudDomainAttachHostPciDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev)
+ virDomainHostdevDefPtr hostdev)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- virDomainHostdevDefPtr hostdev = dev->data.hostdev;
pciDevice *pci;
int ret;
virDomainDevicePCIAddress guestAddr;
@@ -5627,10 +5630,11 @@ error:
return -1;
}
+
static int qemudDomainAttachHostUsbDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev)
+ virDomainHostdevDefPtr hostdev)
{
int ret;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -5641,30 +5645,29 @@ static int qemudDomainAttachHostUsbDevice(virConnectPtr conn,
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- if (dev->data.hostdev->source.subsys.u.usb.vendor) {
+ if (hostdev->source.subsys.u.usb.vendor) {
ret = qemuMonitorAddUSBDeviceMatch(priv->mon,
-
dev->data.hostdev->source.subsys.u.usb.vendor,
-
dev->data.hostdev->source.subsys.u.usb.product);
+ hostdev->source.subsys.u.usb.vendor,
+ hostdev->source.subsys.u.usb.product);
} else {
ret = qemuMonitorAddUSBDeviceExact(priv->mon,
-
dev->data.hostdev->source.subsys.u.usb.bus,
-
dev->data.hostdev->source.subsys.u.usb.device);
+ hostdev->source.subsys.u.usb.bus,
+ hostdev->source.subsys.u.usb.device);
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (ret != -1)
- vm->def->hostdevs[vm->def->nhostdevs++] = dev->data.hostdev;
+ if (ret == 0)
+ vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
return ret;
}
+
static int qemudDomainAttachHostDevice(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainDeviceDefPtr dev)
+ virDomainHostdevDefPtr hostdev)
{
- virDomainHostdevDefPtr hostdev = dev->data.hostdev;
-
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT,
_("hostdev mode '%s' not supported"),
@@ -5674,17 +5677,17 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityHostdevLabel &&
- driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
+ driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, hostdev)
< 0)
return -1;
switch (hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- if (qemudDomainAttachHostPciDevice(conn, driver, vm, dev) < 0)
+ if (qemudDomainAttachHostPciDevice(conn, driver, vm, hostdev) < 0)
goto error;
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- if (qemudDomainAttachHostUsbDevice(conn, driver, vm, dev) < 0)
+ if (qemudDomainAttachHostUsbDevice(conn, driver, vm, hostdev) < 0)
goto error;
break;
@@ -5700,14 +5703,16 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
error:
if (driver->securityDriver &&
driver->securityDriver->domainRestoreSecurityHostdevLabel &&
- driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm,
dev->data.hostdev) < 0)
+ driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm,
hostdev) < 0)
VIR_WARN0("Unable to restore host device labelling on hotplug fail");
return -1;
}
+
static int qemudDomainAttachDevice(virDomainPtr dom,
- const char *xml) {
+ const char *xml)
+{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
virDomainDeviceDefPtr dev = NULL;
@@ -5766,16 +5771,24 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
switch (dev->data.disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm, dev);
+ ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm,
dev->data.disk);
+ if (ret == 0)
+ dev->data.disk = NULL;
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
- ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm,
dev);
+ ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm,
dev->data.disk);
+ if (ret == 0)
+ dev->data.disk = NULL;
} else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
- ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm, dev);
+ ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm,
dev->data.disk);
+ if (ret == 0)
+ dev->data.disk = NULL;
} else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
ret = qemudDomainAttachSCSIDisk(dom->conn, driver, vm,
dev->data.disk, qemuCmdFlags);
+ if (ret == 0)
+ dev->data.disk = NULL;
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
_("disk bus '%s' cannot be
hotplugged."),
@@ -5804,9 +5817,13 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
/* fallthrough */
}
} else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
- ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, dev, qemuCmdFlags);
+ ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, dev->data.net,
qemuCmdFlags);
+ if (ret == 0)
+ dev->data.net = NULL;
} else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
- ret = qemudDomainAttachHostDevice(dom->conn, driver, vm, dev);
+ ret = qemudDomainAttachHostDevice(dom->conn, driver, vm,
dev->data.hostdev);
+ if (ret == 0)
+ dev->data.hostdev = NULL;
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
_("device type '%s' cannot be attached"),
@@ -5825,8 +5842,7 @@ cleanup:
if (cgroup)
virCgroupFree(&cgroup);
- if (ret < 0 && dev != NULL)
- virDomainDeviceDefFree(dev);
+ virDomainDeviceDefFree(dev);
if (vm)
virDomainObjUnlock(vm);
qemuDriverUnlock(driver);
--
1.6.5.2
11:49 a.m.
New subject: [libvirt] [PATCH 11/12] Fix leak in hotplug code in QEMU driver
On Wed, Jan 20, 2010 at 03:15:08PM +0000, Daniel P. Berrange wrote:
The hotplug code in QEMU was leaking memory because although the
inner device object was being moved into the main virDomainDefPtr
config object, the outer container virDomainDeviceDefPtr was not.
* src/qemu/qemu_driver.c: Clarify code to show that the inner
device object is owned by the main domain config upon
successfull attach.
---
src/qemu/qemu_driver.c | 190 ++++++++++++++++++++++++++----------------------
1 files changed, 103 insertions(+), 87 deletions(-)
Okay, that results in a fairly large refactoring
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:15 a.m.
New subject: [libvirt] [PATCH 12/12] Fix off-by-1 in SCSI drive hotplug
The loop looking for the controller associated with a SCI drive had
an off by one, causing it to miss the last controller.
* src/qemu/qemu_driver.c: Fix off-by-1 in searching for SCSI
drive hotplug
---
src/qemu/qemu_driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cb6fe86..d546975 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5346,7 +5346,7 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
goto error;
}
- for (i = 0 ; i < disk->info.addr.drive.controller ; i++) {
+ for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) {
cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i);
if (!cont)
goto error;
--
1.6.5.2
11:55 a.m.
New subject: [libvirt] [PATCH 12/12] Fix off-by-1 in SCSI drive hotplug
On Wed, Jan 20, 2010 at 03:15:09PM +0000, Daniel P. Berrange wrote:
The loop looking for the controller associated with a SCI drive had
an off by one, causing it to miss the last controller.
* src/qemu/qemu_driver.c: Fix off-by-1 in searching for SCSI
drive hotplug
---
src/qemu/qemu_driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cb6fe86..d546975 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5346,7 +5346,7 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
goto error;
}
- for (i = 0 ; i < disk->info.addr.drive.controller ; i++) {
+ for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) {
cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i);
if (!cont)
goto error;
Ah, yes, it's initialized by
def->info.addr.drive.controller = idx / 7;
so 0 need to be checked,
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:57 a.m.
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
This patch series does some work on te security drivers, and the QEMU
code
for managing DAC permissions on files.
The core goal is to turn the QEMU driver DAC file management code into a
security driver. Instead of QEMU calling into the SELinux/AppArmour drivers
directly, a stacked driver module is introduced. This delegates all operations
to first the QEMU DAC driver, and then the main SELinux/AppArmour driver.
The end result is that all the permissions management code is removed from
the QEMU driver, and we're left with just simple security driver calls.
In the process of this a number of flaws in the current hotplug code were
found, and code was generally tidied up with a view to making it easier to
manage.
Finally, we add the ability to turn off the QEMU DAC file managment code,
and also deal gracefully with failures to change ownership (eg on NFS with
root squash, or readonly FS).
Thanks for this series. However, it seems that we still have a problem
when trying to save domain to a root-squashing nfs export.
When using qemu directly, as a user with write permissions to that
export, there is no problem. When using libvirt, libvirt tries to write
its own state to the target file. I would not want to pre-create the
target file as world redable.
How about performing open(path, O_CREAT|O_TRUNC|O_WRONLY,
S_IRUSR|S_IWUSR)) with the euid of the qemu process?
Dan.
10:40 a.m.
On 01/24/2010 11:57 AM, Dan Kenigsberg wrote:
Thanks for this series. However, it seems that we still have a
problem
when trying to save domain to a root-squashing nfs export.
When using qemu directly, as a user with write permissions to that
export, there is no problem. When using libvirt, libvirt tries to write
its own state to the target file. I would not want to pre-create the
target file as world redable.
How about performing open(path, O_CREAT|O_TRUNC|O_WRONLY,
S_IRUSR|S_IWUSR)) with the euid of the qemu process?
This sounds like what I did to fix storage volume creation on NFS:
https://www.redhat.com/archives/libvir-list/2010-January/msg00612.html
I talked to Dan Berrange about it this morning, and he agrees that
something similar can/should be done for domain save. I'm starting work
on it now, and should have something you can try out within a day or two.
6:43 p.m.
New subject: [libvirt] root-squash NFS and qemu domain save
On 01/24/2010 11:57 AM, Dan Kenigsberg wrote:
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
> This patch series does some work on te security drivers, and the QEMU code
> for managing DAC permissions on files.
>
> The core goal is to turn the QEMU driver DAC file management code into a
> security driver. Instead of QEMU calling into the SELinux/AppArmour drivers
> directly, a stacked driver module is introduced. This delegates all operations
> to first the QEMU DAC driver, and then the main SELinux/AppArmour driver.
> The end result is that all the permissions management code is removed from
> the QEMU driver, and we're left with just simple security driver calls.
>
> In the process of this a number of flaws in the current hotplug code were
> found, and code was generally tidied up with a view to making it easier to
> manage.
>
> Finally, we add the ability to turn off the QEMU DAC file managment code,
> and also deal gracefully with failures to change ownership (eg on NFS with
> root squash, or readonly FS).
>
Thanks for this series. However, it seems that we still have a problem
when trying to save domain to a root-squashing nfs export.
When using qemu directly, as a user with write permissions to that
export, there is no problem. When using libvirt, libvirt tries to write
its own state to the target file. I would not want to pre-create the
target file as world redable.
How about performing open(path, O_CREAT|O_TRUNC|O_WRONLY,
S_IRUSR|S_IWUSR)) with the euid of the qemu process?
I came up with a patch that creates the domain state file by first
forking, followed by the child process doing setuid(qemu) and all the
write-to-the-file stuff that creates the file (similar to the recent
virFileCreate patch). This works all the way up through qemu's appending
its state onto the file.
The trouble comes when qemudDomainSave calls
securityDriver->domainRestoreSavedStateLabel() (in this case,
qemuSecurityStackedRestoreSavedStateLabel()). That function first calls
the primary security driver's qemuSecurityDACRestoreSavedStateLabel(),
which will fail because it attempts to chown the file to 0,0 (to prevent
other qemu instances from messing with it). That can be disabled by
setting "dynamic_ownership=0" in qemu.conf (needed in several other
places when stuff is stored on root-squashing NFS anyway :-/). Then the
stacked security driver calls the secondary security driver's
SELinuxRestoreSavedStateLabel(), which also fails because of an SELinux
function (matchpathcon) returning failure. We could ignore this failure,
but that would create an unnecessary security hole for all those
installations that *aren't* storing their domains on root-squashing NFS.
So I *think* what I'm looking for a good way to avoid the SELinux stuff
when the destination file is located on a root-squashing NFS share, but
not ignore it otherwise (I believe the consensus is that we *don't* want
to require another configuration knob if at all possible, since that
would not only lead to extra complexity of config, but also to
uninformed users turning it off when not necessary). Does anyone have
any good ideas for doing this (or for a different approach)?
4:06 a.m.
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
This patch series does some work on te security drivers, and the QEMU
code
for managing DAC permissions on files.
The core goal is to turn the QEMU driver DAC file management code into a
security driver. Instead of QEMU calling into the SELinux/AppArmour drivers
directly, a stacked driver module is introduced. This delegates all operations
to first the QEMU DAC driver, and then the main SELinux/AppArmour driver.
The end result is that all the permissions management code is removed from
the QEMU driver, and we're left with just simple security driver calls.
In the process of this a number of flaws in the current hotplug code were
found, and code was generally tidied up with a view to making it easier to
manage.
Finally, we add the ability to turn off the QEMU DAC file managment code,
and also deal gracefully with failures to change ownership (eg on NFS with
root squash, or readonly FS).
hmmm, there's another problem which this patch set does not address:
error : virStorageFileGetMetadata:415 : cannot open file
'/deep/into/my/root/squashing/export': Permission denied
With dynamic_ownership = 0, libvirt down not mess with chown, but it now
assumes that it can read disk images.
Regrards,
Dan.
4:23 a.m.
On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
> This patch series does some work on te security drivers, and the QEMU code
> for managing DAC permissions on files.
>
> The core goal is to turn the QEMU driver DAC file management code into a
> security driver. Instead of QEMU calling into the SELinux/AppArmour drivers
> directly, a stacked driver module is introduced. This delegates all operations
> to first the QEMU DAC driver, and then the main SELinux/AppArmour driver.
> The end result is that all the permissions management code is removed from
> the QEMU driver, and we're left with just simple security driver calls.
>
> In the process of this a number of flaws in the current hotplug code were
> found, and code was generally tidied up with a view to making it easier to
> manage.
>
> Finally, we add the ability to turn off the QEMU DAC file managment code,
> and also deal gracefully with failures to change ownership (eg on NFS with
> root squash, or readonly FS).
hmmm, there's another problem which this patch set does not address:
error : virStorageFileGetMetadata:415 : cannot open file
'/deep/into/my/root/squashing/export': Permission denied
With dynamic_ownership = 0, libvirt down not mess with chown, but it now
assumes that it can read disk images.
That is a not unreasonable assumption. The model we want to support is one
where
- App uses virStoragePool APIs to setup the NFS mount point on the host
- App uses virStorageVol APIs to create the new volume with correct
ownership at time of creation
- libvirt starts the guest, without needing chown() due to step 2
getting ownership correct at time of creation.
If libvirt can't read the directories leading upto the image, then it
would never have been able to create the volume in the first place. The
patchs we have done have aimed at avoiding root squash problems which
prevent chmod/chown of files between root & non-root. We still assume
that we can read files / browse directories.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6:28 a.m.
On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:
> On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
> > This patch series does some work on te security drivers, and the QEMU code
> > for managing DAC permissions on files.
> >
> > The core goal is to turn the QEMU driver DAC file management code into a
> > security driver. Instead of QEMU calling into the SELinux/AppArmour drivers
> > directly, a stacked driver module is introduced. This delegates all operations
> > to first the QEMU DAC driver, and then the main SELinux/AppArmour driver.
> > The end result is that all the permissions management code is removed from
> > the QEMU driver, and we're left with just simple security driver calls.
> >
> > In the process of this a number of flaws in the current hotplug code were
> > found, and code was generally tidied up with a view to making it easier to
> > manage.
> >
> > Finally, we add the ability to turn off the QEMU DAC file managment code,
> > and also deal gracefully with failures to change ownership (eg on NFS with
> > root squash, or readonly FS).
>
> hmmm, there's another problem which this patch set does not address:
> error : virStorageFileGetMetadata:415 : cannot open file
'/deep/into/my/root/squashing/export': Permission denied
>
> With dynamic_ownership = 0, libvirt down not mess with chown, but it now
> assumes that it can read disk images.
That is a not unreasonable assumption. The model we want to support is one
where
- App uses virStoragePool APIs to setup the NFS mount point on the host
- App uses virStorageVol APIs to create the new volume with correct
ownership at time of creation
- libvirt starts the guest, without needing chown() due to step 2
getting ownership correct at time of creation.
If libvirt can't read the directories leading upto the image, then it
would never have been able to create the volume in the first place. The
patchs we have done have aimed at avoiding root squash problems which
prevent chmod/chown of files between root & non-root. We still assume
that we can read files / browse directories.
Understood. But if I am handling storage myself, I have a regression
when I switch to libvirt from the sordid art of playing with qemu
directly. I don't want to make my repository world-readable, and I don't
want (yet) to use libvirt storage API.
Do I have a third option?
Note that before this patchset, I could apply a patch disabling chown's
and I managed to start domains over root squashing nfs.
Regards,
Dan.
6:56 a.m.
On Tue, Jan 26, 2010 at 02:28:45PM +0200, Dan Kenigsberg wrote:
On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:
> On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:
> > On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
> > > This patch series does some work on te security drivers, and the QEMU
code
> > > for managing DAC permissions on files.
> > >
> > > The core goal is to turn the QEMU driver DAC file management code into a
> > > security driver. Instead of QEMU calling into the SELinux/AppArmour
drivers
> > > directly, a stacked driver module is introduced. This delegates all
operations
> > > to first the QEMU DAC driver, and then the main SELinux/AppArmour driver.
> > > The end result is that all the permissions management code is removed from
> > > the QEMU driver, and we're left with just simple security driver
calls.
> > >
> > > In the process of this a number of flaws in the current hotplug code
were
> > > found, and code was generally tidied up with a view to making it easier
to
> > > manage.
> > >
> > > Finally, we add the ability to turn off the QEMU DAC file managment
code,
> > > and also deal gracefully with failures to change ownership (eg on NFS
with
> > > root squash, or readonly FS).
> >
> > hmmm, there's another problem which this patch set does not address:
> > error : virStorageFileGetMetadata:415 : cannot open file
'/deep/into/my/root/squashing/export': Permission denied
> >
> > With dynamic_ownership = 0, libvirt down not mess with chown, but it now
> > assumes that it can read disk images.
>
> That is a not unreasonable assumption. The model we want to support is one
> where
>
> - App uses virStoragePool APIs to setup the NFS mount point on the host
> - App uses virStorageVol APIs to create the new volume with correct
> ownership at time of creation
> - libvirt starts the guest, without needing chown() due to step 2
> getting ownership correct at time of creation.
>
> If libvirt can't read the directories leading upto the image, then it
> would never have been able to create the volume in the first place. The
> patchs we have done have aimed at avoiding root squash problems which
> prevent chmod/chown of files between root & non-root. We still assume
> that we can read files / browse directories.
Understood. But if I am handling storage myself, I have a regression
when I switch to libvirt from the sordid art of playing with qemu
directly. I don't want to make my repository world-readable, and I don't
want (yet) to use libvirt storage API.
Do I have a third option?
virStorageFileGetMetadata() is used to determine if the disk configured
for a guest has a backing file (eg qcow linked images). This is called
from the DAC driver to setup ownership, and from the SELinux driver to
setup labelling. You say you've already disabled the DAC driver ownership
so it shouldn't be hitting it there. Thus I assume it must the SELinux
driver that's causing the error.
Note that before this patchset, I could apply a patch disabling
chown's
and I managed to start domains over root squashing nfs.
I'm surprised if that's true, because I don't believe my patches changed
this behaviour of the SELinux driver - it was always calling into the
virStorageFileGetMetadata() method to determine backing store, so should
always have hit the issue you describe
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9 a.m.
On Tue, Jan 26, 2010 at 12:56:00PM +0000, Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 02:28:45PM +0200, Dan Kenigsberg wrote:
> On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:
> > On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:
> > > On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
> > > > This patch series does some work on te security drivers, and the QEMU
code
> > > > for managing DAC permissions on files.
> > > >
> > > > The core goal is to turn the QEMU driver DAC file management code
into a
> > > > security driver. Instead of QEMU calling into the SELinux/AppArmour
drivers
> > > > directly, a stacked driver module is introduced. This delegates all
operations
> > > > to first the QEMU DAC driver, and then the main SELinux/AppArmour
driver.
> > > > The end result is that all the permissions management code is removed
from
> > > > the QEMU driver, and we're left with just simple security driver
calls.
> > > >
> > > > In the process of this a number of flaws in the current hotplug code
were
> > > > found, and code was generally tidied up with a view to making it
easier to
> > > > manage.
> > > >
> > > > Finally, we add the ability to turn off the QEMU DAC file managment
code,
> > > > and also deal gracefully with failures to change ownership (eg on NFS
with
> > > > root squash, or readonly FS).
> > >
> > > hmmm, there's another problem which this patch set does not address:
> > > error : virStorageFileGetMetadata:415 : cannot open file
'/deep/into/my/root/squashing/export': Permission denied
> > >
> > > With dynamic_ownership = 0, libvirt down not mess with chown, but it now
> > > assumes that it can read disk images.
> >
> > That is a not unreasonable assumption. The model we want to support is one
> > where
> >
> > - App uses virStoragePool APIs to setup the NFS mount point on the host
> > - App uses virStorageVol APIs to create the new volume with correct
> > ownership at time of creation
> > - libvirt starts the guest, without needing chown() due to step 2
> > getting ownership correct at time of creation.
> >
> > If libvirt can't read the directories leading upto the image, then it
> > would never have been able to create the volume in the first place. The
> > patchs we have done have aimed at avoiding root squash problems which
> > prevent chmod/chown of files between root & non-root. We still assume
> > that we can read files / browse directories.
>
> Understood. But if I am handling storage myself, I have a regression
> when I switch to libvirt from the sordid art of playing with qemu
> directly. I don't want to make my repository world-readable, and I don't
> want (yet) to use libvirt storage API.
>
> Do I have a third option?
virStorageFileGetMetadata() is used to determine if the disk configured
for a guest has a backing file (eg qcow linked images). This is called
from the DAC driver to setup ownership, and from the SELinux driver to
setup labelling. You say you've already disabled the DAC driver ownership
so it shouldn't be hitting it there. Thus I assume it must the SELinux
driver that's causing the error.
> Note that before this patchset, I could apply a patch disabling chown's
> and I managed to start domains over root squashing nfs.
I'm surprised if that's true, because I don't believe my patches changed
this behaviour of the SELinux driver - it was always calling into the
virStorageFileGetMetadata() method to determine backing store, so should
always have hit the issue you describe
Ok I was probably too hasty to blame this patchset, after all, I tested
it earlier on a different machine with a different os and a different
storage export.
However my question still stands: what can I do to make this work when
the storage is not readable by root?
9:06 a.m.
On Tue, Jan 26, 2010 at 05:00:53PM +0200, Dan Kenigsberg wrote:
On Tue, Jan 26, 2010 at 12:56:00PM +0000, Daniel P. Berrange wrote:
> On Tue, Jan 26, 2010 at 02:28:45PM +0200, Dan Kenigsberg wrote:
> > On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:
> > > On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:
> > > > On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:
> > > > > This patch series does some work on te security drivers, and the
QEMU code
> > > > > for managing DAC permissions on files.
> > > > >
> > > > > The core goal is to turn the QEMU driver DAC file management
code into a
> > > > > security driver. Instead of QEMU calling into the
SELinux/AppArmour drivers
> > > > > directly, a stacked driver module is introduced. This delegates
all operations
> > > > > to first the QEMU DAC driver, and then the main
SELinux/AppArmour driver.
> > > > > The end result is that all the permissions management code is
removed from
> > > > > the QEMU driver, and we're left with just simple security
driver calls.
> > > > >
> > > > > In the process of this a number of flaws in the current hotplug
code were
> > > > > found, and code was generally tidied up with a view to making it
easier to
> > > > > manage.
> > > > >
> > > > > Finally, we add the ability to turn off the QEMU DAC file
managment code,
> > > > > and also deal gracefully with failures to change ownership (eg
on NFS with
> > > > > root squash, or readonly FS).
> > > >
> > > > hmmm, there's another problem which this patch set does not
address:
> > > > error : virStorageFileGetMetadata:415 : cannot open file
'/deep/into/my/root/squashing/export': Permission denied
> > > >
> > > > With dynamic_ownership = 0, libvirt down not mess with chown, but it
now
> > > > assumes that it can read disk images.
> > >
> > > That is a not unreasonable assumption. The model we want to support is
one
> > > where
> > >
> > > - App uses virStoragePool APIs to setup the NFS mount point on the host
> > > - App uses virStorageVol APIs to create the new volume with correct
> > > ownership at time of creation
> > > - libvirt starts the guest, without needing chown() due to step 2
> > > getting ownership correct at time of creation.
> > >
> > > If libvirt can't read the directories leading upto the image, then it
> > > would never have been able to create the volume in the first place. The
> > > patchs we have done have aimed at avoiding root squash problems which
> > > prevent chmod/chown of files between root & non-root. We still assume
> > > that we can read files / browse directories.
> >
> > Understood. But if I am handling storage myself, I have a regression
> > when I switch to libvirt from the sordid art of playing with qemu
> > directly. I don't want to make my repository world-readable, and I
don't
> > want (yet) to use libvirt storage API.
> >
> > Do I have a third option?
>
> virStorageFileGetMetadata() is used to determine if the disk configured
> for a guest has a backing file (eg qcow linked images). This is called
> from the DAC driver to setup ownership, and from the SELinux driver to
> setup labelling. You say you've already disabled the DAC driver ownership
> so it shouldn't be hitting it there. Thus I assume it must the SELinux
> driver that's causing the error.
>
> > Note that before this patchset, I could apply a patch disabling chown's
> > and I managed to start domains over root squashing nfs.
>
> I'm surprised if that's true, because I don't believe my patches
changed
> this behaviour of the SELinux driver - it was always calling into the
> virStorageFileGetMetadata() method to determine backing store, so should
> always have hit the issue you describe
Ok I was probably too hasty to blame this patchset, after all, I tested
it earlier on a different machine with a different os and a different
storage export.
However my question still stands: what can I do to make this work when
the storage is not readable by root?
Have you tried disabling the SELinux driver ? That ought to at least
stop this code being called. We might also need to make the function
virStorageFileGetMetadata() treat failure to open the file as non-fatal,
though I'm kind of loathe todo that. I fear all the places where we're
putting in hacks to ignore errors on NFS, are going to make it harder
to see / diagnose real important errors on non-NFS filesystems
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5440
days inactive
5456
days old
37 comments
5 participants
participants (5)
-
Dan Kenigsberg -
Daniel P. Berrange -
Daniel Veillard -
Laine Stump -
Stephen Smalley