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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list