[libvirt] [PATCH v1 0/2] Keep original file label
This creates a new file to store the original file labels. Or do we want the whole functionality to reside in virtlockd? Michal Privoznik (2): security_dac: Keep original file label security_dac: Lock label store file src/Makefile.am | 3 +- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 25 +++ src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 3 +- src/qemu/qemu_driver.c | 9 +- src/security/security_dac.c | 412 +++++++++++++++++++++++++++++++++++---- src/security/security_manager.c | 24 ++- src/security/security_manager.h | 7 +- tests/qemuhotplugtest.c | 2 +- tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 3 +- tests/securityselinuxtest.c | 2 +- 13 files changed, 438 insertions(+), 58 deletions(-) -- 1.9.0
In the DAC security driver chown() is executed in order to allow a domain accessing certain files. Later, when the domain is shutting down, we are chown()-ing the file back. But we don't remember the original owner anywhere, so we return the file to root:root and hope for the best. In this approach, a new file is created. It's in XML, so whenever we want to extend it, we can. In the file, there's this structure: <labels> <label path='/home/zippy/work/tmp/gentoo.qcow2' uid='1000' gid='1000' refcount='1'/> ... </labels> The first chown() appends new <label/> element, each subsequent increments the @refcount attribute. The file is parsed and formatted prior to each call of chown(). It may seem like overhead right now, but with locking introduction (not done here though) we may allow other libvirtds to share the status. Then, whenever we want to restore the DAC label, the corresponding refcount is decremented. If the value is greater than zero, no restoring is done (other domains using the file may lose access). Therefore the original label is restored only if refcount hits value of zero. Then it's removed from the label store file too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 335 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 295 insertions(+), 40 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 85e6eec..b119825 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -34,6 +34,7 @@ #include "virstoragefile.h" #include "virstring.h" #include "virutil.h" +#include "configmake.h" #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" @@ -48,8 +49,140 @@ struct _virSecurityDACData { int ngroups; bool dynamicOwnership; char *baselabel; + char *labelStore; }; +typedef struct _virDACLabel virDACLabel; +typedef virDACLabel *virDACLabelPtr; + +struct _virDACLabel { + char *path; + uid_t uid; + gid_t gid; + unsigned int refcount; +}; + +static void +virSecurityDACLabelStoreFree(virDACLabelPtr labels, + size_t labels_size) +{ + size_t i; + + for (i = 0; i < labels_size; i++) { + VIR_FREE(labels->path); + } + VIR_FREE(labels); +} + +static int +virSecurityDACLabelStoreParse(virSecurityDACDataPtr priv, + virDACLabelPtr *labels, + size_t *labels_size) +{ + int n = 0, ret = -1; + size_t i; + xmlDocPtr doc; + xmlXPathContextPtr ctxt = NULL; + xmlNodePtr *nodes = NULL; + + if (!virFileExists(priv->labelStore)) { + *labels = NULL; + *labels_size = 0; + return 0; + } + + if (!(doc = virXMLParseFileCtxt(priv->labelStore, &ctxt))) + goto cleanup; + + n = virXPathNodeSet("./label", ctxt, &nodes); + + *labels = NULL; + if (n && VIR_ALLOC_N(*labels, n) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + virDACLabelPtr label = &(*labels)[i]; + xmlNodePtr node = nodes[i]; + + ctxt->node = node; + if (!(label->path = virXPathString("string(./@path)", ctxt))) + goto cleanup; + if (virXPathUInt("string(./@uid)", ctxt, &label->uid) < 0) + goto cleanup; + if (virXPathUInt("string(./@gid)", ctxt, &label->gid) < 0) + goto cleanup; + if (virXPathUInt("string(./@refcount)", ctxt, &label->refcount) < 0) + goto cleanup; + } + + *labels_size = n; + ret = 0; +cleanup: + if (ret < 0) + virSecurityDACLabelStoreFree(*labels, n); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + return ret; +} + +static virDACLabelPtr +virSecurityDACLabelStoreFind(virDACLabelPtr labels, + size_t labels_size, + const char *path) +{ + size_t i; + + for (i = 0; i < labels_size; i++) { + if (STREQ(labels[i].path, path)) + return &labels[i]; + } + return NULL; +} + +static int +virSecurityDACLabelStoreFormat(virSecurityDACDataPtr priv, + virDACLabelPtr labels, + size_t labels_size) +{ + int ret = -1; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *xml = NULL; + size_t i; + + virBufferAddLit(&buf, "<labels>\n"); + virBufferAdjustIndent(&buf, 2); + for (i = 0; i < labels_size; i++) { + virDACLabelPtr label = &labels[i]; + if (label->refcount == 0) + continue; + virBufferAsprintf(&buf, + "<label path='%s' uid='%u' gid='%u' refcount='%u'/>\n", + label->path, label->uid, label->gid, label->refcount); + } + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</labels>\n"); + + if (virBufferError(&buf)) + goto cleanup; + + xml = virBufferContentAndReset(&buf); + + if (!xml) + goto cleanup; + + if (virFileWriteStr(priv->labelStore, xml, 0600) < 0) { + virReportSystemError(errno, + _("unable to write label store file: %s"), + priv->labelStore); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(xml); + return ret; +} + /* returns -1 on error, 0 on success */ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, @@ -210,8 +343,13 @@ virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) } static int -virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecurityDACOpen(virSecurityManagerPtr mgr) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + if (VIR_STRDUP(priv->labelStore, LOCALSTATEDIR "/run/libvirt/labelStore.xml") < 0) + return -1; + return 0; } @@ -221,6 +359,7 @@ virSecurityDACClose(virSecurityManagerPtr mgr) virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); VIR_FREE(priv->groups); VIR_FREE(priv->baselabel); + VIR_FREE(priv->labelStore); return 0; } @@ -253,7 +392,98 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) } static int -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +virSecurityDACRememberLabel(virSecurityManagerPtr mgr, + const char *path) +{ + int ret = -1; + virDACLabelPtr labels = NULL, label; + size_t labels_size = 0; + struct stat sb; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + /* TODO lock the labelStore file */ + if (virSecurityDACLabelStoreParse(priv, &labels, &labels_size) < 0) + goto cleanup; + + label = virSecurityDACLabelStoreFind(labels, labels_size, path); + + if (!label) { + if (VIR_REALLOC_N(labels, labels_size + 1) < 0) + goto cleanup; + + label = &labels[labels_size++]; + if (VIR_STRDUP(label->path, path) < 0) + goto cleanup; + + if (stat(path, &sb) < 0) { + virReportSystemError(errno, + _("unable to stat: %s"), + path); + goto cleanup; + } + label->uid = sb.st_uid; + label->gid = sb.st_gid; + label->refcount = 0; + } + + label->refcount++; + + if (virSecurityDACLabelStoreFormat(priv, labels, labels_size) < 0) + goto cleanup; + + ret = 0; +cleanup: + /* TODO unlock the labelStore file */ + virSecurityDACLabelStoreFree(labels, labels_size); + return ret; +} + +static int +virSecurityDACRecallLabel(virSecurityManagerPtr mgr, + const char *path, + uid_t *uid, + gid_t *gid, + unsigned int *refcount) +{ + int ret = -1; + virDACLabelPtr labels = NULL, label; + size_t labels_size = 0; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + /* TODO lock the labelStore file */ + if (virSecurityDACLabelStoreParse(priv, &labels, &labels_size) < 0) + goto cleanup; + + label = virSecurityDACLabelStoreFind(labels, labels_size, path); + + if (!label) { + /* It's not an error if @path wasn't found */ + VIR_DEBUG("path %s wasn't found in labelStore %s, using defaults", + path, priv->labelStore); + *uid = *gid = *refcount = 0; + ret = 0; + goto cleanup; + } + + *refcount = --label->refcount; + + if (!label->refcount) { + *uid = label->uid; + *gid = label->gid; + } + + if (virSecurityDACLabelStoreFormat(priv, labels, labels_size) < 0) + goto cleanup; + + ret = 0; +cleanup: + /* TODO unlock the labelStore file */ + virSecurityDACLabelStoreFree(labels, labels_size); + return ret; +} + +static int +virSecurityDACChown(const char *path, uid_t uid, gid_t gid) { VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", path, (long) uid, (long) gid); @@ -294,29 +524,53 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) } static int -virSecurityDACRestoreSecurityFileLabel(const char *path) +virSecurityDACSetOwnership(virSecurityManagerPtr mgr, + const char *path, + uid_t uid, + gid_t gid) { - struct stat buf; - int rc = -1; + int ret = -1; + + if (virSecurityDACRememberLabel(mgr, path) < 0) + goto cleanup; + + ret = virSecurityDACChown(path, uid, gid); +cleanup: + return ret; +} + +static int +virSecurityDACRestoreSecurityFileLabel(virSecurityManagerPtr mgr, + const char *path) +{ + int ret = -1; char *newpath = NULL; - - VIR_INFO("Restoring DAC user and group on '%s'", path); + uid_t uid; + gid_t gid; + unsigned int refcount; if (virFileResolveLink(path, &newpath) < 0) { virReportSystemError(errno, _("cannot resolve symlink %s"), path); - goto err; + goto cleanup; } - if (stat(newpath, &buf) != 0) - goto err; + if (virSecurityDACRecallLabel(mgr, path, &uid, &gid, &refcount) < 0) + goto cleanup; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); + if (refcount > 0) { + VIR_DEBUG("Not restoring label on path %s refcount=%u", path, refcount); + } else { + VIR_INFO("Restoring DAC user and group on '%s'", path); + if (virSecurityDACChown(newpath, uid, gid) < 0) + goto cleanup; + } + + ret = 0; -err: +cleanup: VIR_FREE(newpath); - return rc; + return ret; } @@ -336,7 +590,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, if (virSecurityDACGetImageIds(def, priv, &user, &group)) return -1; - return virSecurityDACSetOwnership(path, user, group); + return virSecurityDACSetOwnership(mgr, path, user, group); } @@ -408,7 +662,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreSecurityFileLabel(disk->src); + return virSecurityDACRestoreSecurityFileLabel(mgr, disk->src); } @@ -435,7 +689,7 @@ virSecurityDACSetSecurityHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) return -1; - return virSecurityDACSetOwnership(file, user, group); + return virSecurityDACSetOwnership(mgr, file, user, group); } @@ -563,27 +817,27 @@ done: static int virSecurityDACRestoreSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + return virSecurityDACRestoreSecurityFileLabel(opaque, file); } static int virSecurityDACRestoreSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + return virSecurityDACRestoreSecurityFileLabel(opaque, file); } static int virSecurityDACRestoreSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + return virSecurityDACRestoreSecurityFileLabel(opaque, file); } @@ -696,7 +950,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(dev->data.file.path, user, group); + ret = virSecurityDACSetOwnership(mgr, dev->data.file.path, user, group); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -704,11 +958,11 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACSetOwnership(in, user, group) < 0) || - (virSecurityDACSetOwnership(out, user, group) < 0)) { + if ((virSecurityDACSetOwnership(mgr, in, user, group) < 0) || + (virSecurityDACSetOwnership(mgr, out, user, group) < 0)) { goto done; } - } else if (virSecurityDACSetOwnership(dev->data.file.path, + } else if (virSecurityDACSetOwnership(mgr, dev->data.file.path, user, group) < 0) { goto done; } @@ -727,7 +981,7 @@ done: } static int -virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainChrSourceDefPtr dev) { char *in = NULL, *out = NULL; @@ -736,7 +990,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path); + ret = virSecurityDACRestoreSecurityFileLabel(mgr, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -744,11 +998,12 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, (virAsprintf(&in, "%s.in", dev->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) || - (virSecurityDACRestoreSecurityFileLabel(in) < 0)) { + if ((virSecurityDACRestoreSecurityFileLabel(mgr, out) < 0) || + (virSecurityDACRestoreSecurityFileLabel(mgr, in) < 0)) { goto done; } - } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path) < 0) { + } else if (virSecurityDACRestoreSecurityFileLabel(mgr, + dev->data.file.path) < 0) { goto done; } ret = 0; @@ -860,15 +1115,15 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, } if (def->os.kernel && - virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) + virSecurityDACRestoreSecurityFileLabel(mgr, def->os.kernel) < 0) rc = -1; if (def->os.initrd && - virSecurityDACRestoreSecurityFileLabel(def->os.initrd) < 0) + virSecurityDACRestoreSecurityFileLabel(mgr, def->os.initrd) < 0) rc = -1; if (def->os.dtb && - virSecurityDACRestoreSecurityFileLabel(def->os.dtb) < 0) + virSecurityDACRestoreSecurityFileLabel(mgr, def->os.dtb) < 0) rc = -1; return rc; @@ -933,15 +1188,15 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.kernel && - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) + virSecurityDACSetOwnership(mgr, def->os.kernel, user, group) < 0) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(def->os.initrd, user, group) < 0) + virSecurityDACSetOwnership(mgr, def->os.initrd, user, group) < 0) return -1; if (def->os.dtb && - virSecurityDACSetOwnership(def->os.dtb, user, group) < 0) + virSecurityDACSetOwnership(mgr, def->os.dtb, user, group) < 0) return -1; return 0; @@ -960,7 +1215,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(def, priv, &user, &group)) return -1; - return virSecurityDACSetOwnership(savefile, user, group); + return virSecurityDACSetOwnership(mgr, savefile, user, group); } @@ -974,7 +1229,7 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - return virSecurityDACRestoreSecurityFileLabel(savefile); + return virSecurityDACRestoreSecurityFileLabel(mgr, savefile); } -- 1.9.0
If there are two or more libvirtds fighting over the label store file a data corruption is likely to occur. Therefore we need to enforce an exclusive access to the file. With a little help from virtlockd we can achieve that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/Makefile.am | 3 +- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 25 ++++++++++++ src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 3 +- src/qemu/qemu_driver.c | 9 +++-- src/security/security_dac.c | 85 ++++++++++++++++++++++++++++++++++++++-- src/security/security_manager.c | 24 +++++++++--- src/security/security_manager.h | 7 +++- tests/qemuhotplugtest.c | 2 +- tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 3 +- tests/securityselinuxtest.c | 2 +- 13 files changed, 147 insertions(+), 22 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index d4d7b2b..eab1dbf 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1555,7 +1555,7 @@ libvirt_security_manager_la_SOURCES = $(SECURITY_DRIVER_SOURCES) noinst_LTLIBRARIES += libvirt_security_manager.la libvirt_la_BUILT_LIBADD += libvirt_security_manager.la libvirt_security_manager_la_CFLAGS = \ - -I$(top_srcdir)/src/conf $(AM_CFLAGS) + -I$(top_srcdir)/src/conf -I$(top_srcdir)/src/locking $(AM_CFLAGS) libvirt_security_manager_la_LDFLAGS = $(AM_LDFLAGS) libvirt_security_manager_la_LIBADD = $(SECDRIVER_LIBS) if WITH_SECDRIVER_SELINUX @@ -2512,6 +2512,7 @@ libvirt_lxc_LDADD = \ libvirt_security_manager.la \ libvirt_conf.la \ libvirt_util.la \ + libvirt_driver.la \ ../gnulib/lib/libgnu.la if WITH_DTRACE_PROBES libvirt_lxc_LDADD += libvirt_probes.lo diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index a7d9782..6bcb3cc 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -42,6 +42,8 @@ typedef enum { typedef enum { /* The managed object is a virtual guest domain */ VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN = 0, + /* The managed object is DAC driver label store file */ + VIR_LOCK_MANAGER_OBJECT_TYPE_DAC = (1 << 0), } virLockManagerObjectType; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 1b92d68..53f8ed6 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -494,6 +494,31 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, } break; + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAC: + for (i = 0; i < nparams; i++) { + if (STREQ(params[i].key, "name")) { + if (VIR_STRDUP(priv->name, params[i].value.str) < 0) + return -1; + } else if (STREQ(params[i].key, "pid")) { + priv->pid = params[i].value.iv; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected parameter %s for object"), + params[i].key); + } + } + if (!priv->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing name parameter for DAC object")); + return -1; + } + if (priv->pid == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing PID parameter for DAC object")); + return -1; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 5ca960f..1a85ebc 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2392,7 +2392,7 @@ int main(int argc, char *argv[]) if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver, LXC_DRIVER_NAME, - false, false, false))) + false, false, false, NULL))) goto cleanup; if (ctrl->def->seclabels) { diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0ab1ba2..9b4d3ff 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1485,7 +1485,8 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg) LXC_DRIVER_NAME, false, cfg->securityDefaultConfined, - cfg->securityRequireConfined); + cfg->securityRequireConfined, + NULL); if (!mgr) goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba470a1..25c1c57 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -338,7 +338,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) QEMU_DRIVER_NAME, cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, - cfg->securityRequireConfined))) + cfg->securityRequireConfined, + driver->lockManager))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) @@ -355,7 +356,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) QEMU_DRIVER_NAME, cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, - cfg->securityRequireConfined))) + cfg->securityRequireConfined, + driver->lockManager))) goto error; if (!(stack = virSecurityManagerNewStack(mgr))) goto error; @@ -369,7 +371,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, cfg->securityRequireConfined, - cfg->dynamicOwnership))) + cfg->dynamicOwnership, + driver->lockManager))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b119825..5527287 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -35,6 +35,8 @@ #include "virstring.h" #include "virutil.h" #include "configmake.h" +#include "lock_driver.h" +#include "lock_manager.h" #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" @@ -50,6 +52,7 @@ struct _virSecurityDACData { bool dynamicOwnership; char *baselabel; char *labelStore; + virLockManagerPluginPtr plugin; }; typedef struct _virDACLabel virDACLabel; @@ -183,6 +186,75 @@ cleanup: return ret; } +static int +virSecurityDACLabelStoreLock(virSecurityDACDataPtr priv, + bool lock) +{ + int ret = -1; + const unsigned int max_retries = 10; + unsigned int retries; + virLockManagerPtr lockMgr = NULL; + virLockManagerParam params[] = { + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING, + .key = "name", + .value = { .str = priv->labelStore }, + }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, + .key = "pid", + .value = { .iv = getppid() }, + }, + }; + + if (!(lockMgr = virLockManagerNew(virLockManagerPluginGetDriver(priv->plugin), + VIR_LOCK_MANAGER_OBJECT_TYPE_DAC, + ARRAY_CARDINALITY(params), + params, + 0))) + goto cleanup; + + if (virLockManagerAddResource(lockMgr, + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, + priv->labelStore, + 0, + NULL, + 0) < 0) + goto cleanup; + + if (lock) { + for (retries = 0; retries < max_retries; retries++) { + struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; + int rv; + + rv = virLockManagerAcquire(lockMgr, NULL, 0, + VIR_DOMAIN_LOCK_FAILURE_IGNORE, NULL); + + if (rv == 0) + break; + + nanosleep(&ts, NULL); + } + + if (retries == max_retries) { + virReportError(VIR_ERR_OPERATION_TIMEOUT, + _("unable to lock %s"), + priv->labelStore); + goto cleanup; + } + } else { + if (virLockManagerRelease(lockMgr, NULL, 0) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("unable to unlock %s"), + priv->labelStore); + goto cleanup; + } + } + + ret = 0; +cleanup: + virLockManagerFree(lockMgr); + return ret; +} + /* returns -1 on error, 0 on success */ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, @@ -350,6 +422,7 @@ virSecurityDACOpen(virSecurityManagerPtr mgr) if (VIR_STRDUP(priv->labelStore, LOCALSTATEDIR "/run/libvirt/labelStore.xml") < 0) return -1; + priv->plugin = virSecurityManagerGetLockPlugin(mgr); return 0; } @@ -401,7 +474,9 @@ virSecurityDACRememberLabel(virSecurityManagerPtr mgr, struct stat sb; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - /* TODO lock the labelStore file */ + if (virSecurityDACLabelStoreLock(priv, true) < 0) + return ret; + if (virSecurityDACLabelStoreParse(priv, &labels, &labels_size) < 0) goto cleanup; @@ -433,7 +508,7 @@ virSecurityDACRememberLabel(virSecurityManagerPtr mgr, ret = 0; cleanup: - /* TODO unlock the labelStore file */ + virSecurityDACLabelStoreLock(priv, false); virSecurityDACLabelStoreFree(labels, labels_size); return ret; } @@ -450,7 +525,9 @@ virSecurityDACRecallLabel(virSecurityManagerPtr mgr, size_t labels_size = 0; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - /* TODO lock the labelStore file */ + if (virSecurityDACLabelStoreLock(priv, true) < 0) + return ret; + if (virSecurityDACLabelStoreParse(priv, &labels, &labels_size) < 0) goto cleanup; @@ -477,7 +554,7 @@ virSecurityDACRecallLabel(virSecurityManagerPtr mgr, ret = 0; cleanup: - /* TODO unlock the labelStore file */ + virSecurityDACLabelStoreLock(priv, false); virSecurityDACLabelStoreFree(labels, labels_size); return ret; } diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 5ecf72f..e5a67f2 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -43,6 +43,7 @@ struct _virSecurityManager { bool requireConfined; const char *virtDriver; void *privateData; + void *lockPlugin; }; static virClassPtr virSecurityManagerClass; @@ -66,7 +67,8 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, - bool requireConfined) + bool requireConfined, + void *lockPlugin) { virSecurityManagerPtr mgr; char *privateData; @@ -94,6 +96,7 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr mgr->requireConfined = requireConfined; mgr->virtDriver = virtDriver; mgr->privateData = privateData; + mgr->lockPlugin = lockPlugin; if (drv->open(mgr) < 0) { virObjectUnref(mgr); @@ -110,7 +113,8 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary) virSecurityManagerGetDriver(primary), virSecurityManagerGetAllowDiskFormatProbing(primary), virSecurityManagerGetDefaultConfined(primary), - virSecurityManagerGetRequireConfined(primary)); + virSecurityManagerGetRequireConfined(primary), + virSecurityManagerGetLockPlugin(primary)); if (!mgr) return NULL; @@ -134,14 +138,16 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership) + bool dynamicOwnership, + void *lockPlugin) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, virtDriver, allowDiskFormatProbing, defaultConfined, - requireConfined); + requireConfined, + lockPlugin); if (!mgr) return NULL; @@ -159,7 +165,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, - bool requireConfined) + bool requireConfined, + void *lockPlugin) { virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver); if (!drv) @@ -189,7 +196,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, virtDriver, allowDiskFormatProbing, defaultConfined, - requireConfined); + requireConfined, + lockPlugin); } @@ -229,6 +237,10 @@ void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) return mgr->privateData; } +void *virSecurityManagerGetLockPlugin(virSecurityManagerPtr mgr) +{ + return mgr->lockPlugin; +} static void virSecurityManagerDispose(void *obj) { diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 81d3160..52b2ff0 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -33,7 +33,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, - bool requireConfined); + bool requireConfined, + void *lockPlugin); virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary); int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, @@ -45,12 +46,14 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership); + bool dynamicOwnership, + void *lockPlugin); int virSecurityManagerPreFork(virSecurityManagerPtr mgr); void virSecurityManagerPostFork(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); +void *virSecurityManagerGetLockPlugin(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 8036adc..041cf36 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -353,7 +353,7 @@ mymain(void) if (!driver.lockManager) return EXIT_FAILURE; - if (!(mgr = virSecurityManagerNew("none", "qemu", false, false, false))) + if (!(mgr = virSecurityManagerNew("none", "qemu", false, false, false, NULL))) return EXIT_FAILURE; if (!(driver.securityManager = virSecurityManagerNewStack(mgr))) return EXIT_FAILURE; diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index cd34b6b..e21a8c1 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -17,7 +17,7 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) if (virThreadInitialize() < 0) return EXIT_FAILURE; - mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false); + mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false, NULL); if (mgr == NULL) { fprintf(stderr, "Failed to start security driver"); return EXIT_FAILURE; diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 3505f8c..8bd2a5b 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -320,7 +320,8 @@ mymain(void) { int ret = 0; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) { + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, + true, false, NULL))) { virErrorPtr err = virGetLastError(); fprintf(stderr, "Unable to initialize security driver: %s\n", err->message); diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index feb5366..99980f4 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -270,7 +270,7 @@ mymain(void) int ret = 0; virSecurityManagerPtr mgr; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) { + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false, NULL))) { virErrorPtr err = virGetLastError(); fprintf(stderr, "Unable to initialize security driver: %s\n", err->message); -- 1.9.0
On 13.03.2014 10:02, Michal Privoznik wrote:
This creates a new file to store the original file labels. Or do we want the whole functionality to reside in virtlockd?
Michal Privoznik (2): security_dac: Keep original file label security_dac: Lock label store file
src/Makefile.am | 3 +- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 25 +++ src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 3 +- src/qemu/qemu_driver.c | 9 +- src/security/security_dac.c | 412 +++++++++++++++++++++++++++++++++++---- src/security/security_manager.c | 24 ++- src/security/security_manager.h | 7 +- tests/qemuhotplugtest.c | 2 +- tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 3 +- tests/securityselinuxtest.c | 2 +- 13 files changed, 438 insertions(+), 58 deletions(-)
Ping? Michal
On 13.03.2014 10:02, Michal Privoznik wrote:
This creates a new file to store the original file labels. Or do we want the whole functionality to reside in virtlockd?
Michal Privoznik (2): security_dac: Keep original file label security_dac: Lock label store file
src/Makefile.am | 3 +- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 25 +++ src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 3 +- src/qemu/qemu_driver.c | 9 +- src/security/security_dac.c | 412 +++++++++++++++++++++++++++++++++++---- src/security/security_manager.c | 24 ++- src/security/security_manager.h | 7 +- tests/qemuhotplugtest.c | 2 +- tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 3 +- tests/securityselinuxtest.c | 2 +- 13 files changed, 438 insertions(+), 58 deletions(-)
Ping2? Michal
participants (1)
-
Michal Privoznik