[libvirt] [PATCH 01/12] Make security drivers responsible for checking dynamic vs static labelling
Daniel P. Berrange
berrange at redhat.com
Wed Jan 20 15:14:58 UTC 2010
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
More information about the libvir-list
mailing list