[libvirt] [PATCH v3 0/2] Add <seclabel> to character devices.
Previous discussion: https://www.redhat.com/archives/libvir-list/2012-September/thread.html#01037 This adds <seclabel> to character devices' <source/> elements, like this: <serial type="unix"> <source mode="connect" path="/tmp/console.sock"> <seclabel model="selinux" relabel="no"/> </source> <target port="0"/> </serial> I tested it by controlling the labelling of the libguestfs console socket (when unlabelled, SELinux prevents libguestfs from starting), and it appears to work. By the way, I could only get this to work by explicitly adding the model="selinux" attribute. Looking at the code, it seems the same would be true for disk-specific seclabels too, so the documentation is wrong. Rich.
From: "Richard W.M. Jones" <rjones@redhat.com> This is just code motion, allowing us to reuse the same function to parse the <seclabel> from character devices too. --- src/conf/domain_conf.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35814fb..06b3209 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3253,8 +3253,10 @@ error: return -1; } +/* Parse the <seclabel> from a disk or serial device. */ static int -virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, +virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, + size_t *nseclabels_rtn, virSecurityLabelDefPtr *vmSeclabels, int nvmSeclabels, xmlXPathContextPtr ctxt) { @@ -3263,19 +3265,16 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, virSecurityLabelDefPtr vmDef = NULL; char *model, *relabel, *label; - if (def == NULL) - return 0; - if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) == 0) return 0; - def->nseclabels = n; - if (VIR_ALLOC_N(def->seclabels, n) < 0) { + *nseclabels_rtn = n; + if (VIR_ALLOC_N(*seclabels_rtn, n) < 0) { virReportOOMError(); goto error; } for (i = 0; i < n; i++) { - if (VIR_ALLOC(def->seclabels[i]) < 0) { + if (VIR_ALLOC((*seclabels_rtn)[i]) < 0) { virReportOOMError(); goto error; } @@ -3292,7 +3291,7 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, break; } } - def->seclabels[i]->model = model; + (*seclabels_rtn)[i]->model = model; } /* Can't use overrides if top-level doesn't allow relabeling. */ @@ -3306,9 +3305,9 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, relabel = virXMLPropString(list[i], "relabel"); if (relabel != NULL) { if (STREQ(relabel, "yes")) { - def->seclabels[i]->norelabel = false; + (*seclabels_rtn)[i]->norelabel = false; } else if (STREQ(relabel, "no")) { - def->seclabels[i]->norelabel = true; + (*seclabels_rtn)[i]->norelabel = true; } else { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), @@ -3318,19 +3317,19 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, } VIR_FREE(relabel); } else { - def->seclabels[i]->norelabel = false; + (*seclabels_rtn)[i]->norelabel = false; } ctxt->node = list[i]; label = virXPathStringLimit("string(./label)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - def->seclabels[i]->label = label; + (*seclabels_rtn)[i]->label = label; - if (label && def->seclabels[i]->norelabel) { + if (label && (*seclabels_rtn)[i]->norelabel) { virReportError(VIR_ERR_XML_ERROR, _("Cannot specify a label if relabelling is " "turned off. model=%s"), - NULLSTR(def->seclabels[i]->model)); + NULLSTR((*seclabels_rtn)[i]->model)); goto error; } } @@ -3339,10 +3338,11 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, error: for (i = 0; i < n; i++) { - virSecurityDeviceLabelDefFree(def->seclabels[i]); + virSecurityDeviceLabelDefFree((*seclabels_rtn)[i]); } - VIR_FREE(def->seclabels); + VIR_FREE(*seclabels_rtn); VIR_FREE(list); + *nseclabels_rtn = 0; return -1; } @@ -3834,7 +3834,9 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (sourceNode) { xmlNodePtr saved_node = ctxt->node; ctxt->node = sourceNode; - if (virSecurityDeviceLabelDefParseXML(def, vmSeclabels, + if (virSecurityDeviceLabelDefParseXML(&def->seclabels, + &def->nseclabels, + vmSeclabels, nvmSeclabels, ctxt) < 0) goto error; -- 1.7.10.4
On 20.09.2012 17:29, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
This is just code motion, allowing us to reuse the same function to parse the <seclabel> from character devices too. --- src/conf/domain_conf.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35814fb..06b3209 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3253,8 +3253,10 @@ error: return -1; }
+/* Parse the <seclabel> from a disk or serial device. */ static int -virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, +virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, + size_t *nseclabels_rtn, virSecurityLabelDefPtr *vmSeclabels, int nvmSeclabels, xmlXPathContextPtr ctxt) { @@ -3263,19 +3265,16 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, virSecurityLabelDefPtr vmDef = NULL; char *model, *relabel, *label;
- if (def == NULL) - return 0; - if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) == 0) return 0;
- def->nseclabels = n; - if (VIR_ALLOC_N(def->seclabels, n) < 0) { + *nseclabels_rtn = n; + if (VIR_ALLOC_N(*seclabels_rtn, n) < 0) {
I know it is not your fault, but this may lead to SIGSEGV. if this fails then we proceed to error label where we dereference *seclabels_rtn. I think the correct solution is: - if VIR_ALLOC_N() fails, virReportOOMError(); return -1; - set nseclabels_rtn = n; after alloc succeeds. or check for *seclabels_rtn != NULL within error label.
virReportOOMError(); goto error; } for (i = 0; i < n; i++) { - if (VIR_ALLOC(def->seclabels[i]) < 0) { + if (VIR_ALLOC((*seclabels_rtn)[i]) < 0) { virReportOOMError(); goto error; }
This is okay, since previous VIR_ALLOC zeroed the allocated memory.
@@ -3292,7 +3291,7 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, break; } } - def->seclabels[i]->model = model; + (*seclabels_rtn)[i]->model = model; }
/* Can't use overrides if top-level doesn't allow relabeling. */ @@ -3306,9 +3305,9 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, relabel = virXMLPropString(list[i], "relabel"); if (relabel != NULL) { if (STREQ(relabel, "yes")) { - def->seclabels[i]->norelabel = false; + (*seclabels_rtn)[i]->norelabel = false; } else if (STREQ(relabel, "no")) { - def->seclabels[i]->norelabel = true; + (*seclabels_rtn)[i]->norelabel = true; } else { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), @@ -3318,19 +3317,19 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, } VIR_FREE(relabel); } else { - def->seclabels[i]->norelabel = false; + (*seclabels_rtn)[i]->norelabel = false; }
ctxt->node = list[i]; label = virXPathStringLimit("string(./label)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - def->seclabels[i]->label = label; + (*seclabels_rtn)[i]->label = label;
- if (label && def->seclabels[i]->norelabel) { + if (label && (*seclabels_rtn)[i]->norelabel) { virReportError(VIR_ERR_XML_ERROR, _("Cannot specify a label if relabelling is " "turned off. model=%s"), - NULLSTR(def->seclabels[i]->model)); + NULLSTR((*seclabels_rtn)[i]->model)); goto error; } } @@ -3339,10 +3338,11 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def,
error: for (i = 0; i < n; i++) { - virSecurityDeviceLabelDefFree(def->seclabels[i]); + virSecurityDeviceLabelDefFree((*seclabels_rtn)[i]); } - VIR_FREE(def->seclabels); + VIR_FREE(*seclabels_rtn); VIR_FREE(list); + *nseclabels_rtn = 0; return -1; }
@@ -3834,7 +3834,9 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (sourceNode) { xmlNodePtr saved_node = ctxt->node; ctxt->node = sourceNode; - if (virSecurityDeviceLabelDefParseXML(def, vmSeclabels, + if (virSecurityDeviceLabelDefParseXML(&def->seclabels, + &def->nseclabels, + vmSeclabels, nvmSeclabels, ctxt) < 0) goto error;
ACK with the issue fixed. Michal
From: "Richard W.M. Jones" <rjones@redhat.com> This allows the user to control labelling of each character device separately (the default is to inherit from the VM). --- docs/formatdomain.html.in | 8 ++++ src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 6 +++ src/security/security_selinux.c | 86 +++++++++++++++++++++++++++------------ 4 files changed, 147 insertions(+), 30 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 51f897c..939312c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3434,6 +3434,14 @@ qemu-kvm -net nic,model=? /dev/null </p> <p> + The <code>source</code> element may contain an optional + <code>seclabel</code> to override the way that labelling + is done on the socket path. If this element is not present, + the <a href="#seclabel">security label is inherited from + the per-domain setting</a>. + </p> + + <p> Each character device element has an optional sub-element <code><address></code> which can tie the device to a diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 06b3209..7023897 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1273,6 +1273,8 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, void virDomainChrDefFree(virDomainChrDefPtr def) { + size_t i; + if (!def) return; @@ -1296,6 +1298,12 @@ void virDomainChrDefFree(virDomainChrDefPtr def) virDomainChrSourceDefClear(&def->source); virDomainDeviceInfoClear(&def->info); + if (def->seclabels) { + for (i = 0; i < def->nseclabels; i++) + virSecurityDeviceLabelDefFree(def->seclabels[i]); + VIR_FREE(def->seclabels); + } + VIR_FREE(def); } @@ -5343,7 +5351,11 @@ error: * <target>, which is used by <serial> but not <smartcard>). */ static int virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, - xmlNodePtr cur, unsigned int flags) + xmlNodePtr cur, unsigned int flags, + virDomainChrDefPtr chr_def, + xmlXPathContextPtr ctxt, + virSecurityLabelDefPtr* vmSeclabels, + int nvmSeclabels) { char *bindHost = NULL; char *bindService = NULL; @@ -5398,6 +5410,21 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, if (def->type == VIR_DOMAIN_CHR_TYPE_UDP) VIR_FREE(mode); } + + /* Check for an optional seclabel override in <source/>. */ + if (chr_def) { + xmlNodePtr saved_node = ctxt->node; + ctxt->node = cur; + if (virSecurityDeviceLabelDefParseXML (&chr_def->seclabels, + &chr_def->nseclabels, + vmSeclabels, + nvmSeclabels, + ctxt) < 0) { + ctxt->node = saved_node; + goto error; + } + ctxt->node = saved_node; + } } else if (xmlStrEqual(cur->name, BAD_CAST "protocol")) { if (protocol == NULL) protocol = virXMLPropString(cur, "type"); @@ -5591,7 +5618,10 @@ virDomainChrDefNew(void) { static virDomainChrDefPtr virDomainChrDefParseXML(virCapsPtr caps, virDomainDefPtr vmdef, + xmlXPathContextPtr ctxt, xmlNodePtr node, + virSecurityLabelDefPtr* vmSeclabels, + int nvmSeclabels, unsigned int flags) { xmlNodePtr cur; @@ -5622,7 +5652,8 @@ virDomainChrDefParseXML(virCapsPtr caps, } cur = node->children; - remaining = virDomainChrSourceDefParseXML(&def->source, cur, flags); + remaining = virDomainChrSourceDefParseXML(&def->source, cur, flags, + def, ctxt, vmSeclabels, nvmSeclabels); if (remaining < 0) goto error; if (remaining) { @@ -5759,7 +5790,8 @@ virDomainSmartcardDefParseXML(xmlNodePtr node, } cur = node->children; - if (virDomainChrSourceDefParseXML(&def->data.passthru, cur, flags) < 0) + if (virDomainChrSourceDefParseXML(&def->data.passthru, cur, flags, + NULL, NULL, NULL, 0) < 0) goto error; if (def->data.passthru.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { @@ -7240,7 +7272,8 @@ virDomainRedirdevDefParseXML(const xmlNodePtr node, if (xmlStrEqual(cur->name, BAD_CAST "source")) { int remaining; - remaining = virDomainChrSourceDefParseXML(&def->source.chr, cur, flags); + remaining = virDomainChrSourceDefParseXML(&def->source.chr, cur, flags, + NULL, NULL, NULL, 0); if (remaining != 0) goto error; } @@ -9282,7 +9315,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -9309,7 +9345,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -9339,7 +9378,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, bool create_stub = true; virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -9415,7 +9457,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -12359,6 +12404,7 @@ virDomainChrDefFormat(virBufferPtr buf, const char *targetType = virDomainChrTargetTypeToString(def->deviceType, def->targetType); bool tty_compat; + size_t n; int ret = 0; @@ -12438,6 +12484,14 @@ virDomainChrDefFormat(virBufferPtr buf, return -1; } + /* Security label overrides, if any. */ + if (def->seclabels && def->nseclabels > 0) { + virBufferAdjustIndent(buf, 2); + for (n = 0; n < def->nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); + virBufferAdjustIndent(buf, -2); + } + virBufferAsprintf(buf, " </%s>\n", elementName); return ret; @@ -15262,6 +15316,21 @@ virDomainDiskDefGetSecurityLabelDef(virDomainDiskDefPtr def, const char *model) return NULL; } +virSecurityDeviceLabelDefPtr +virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model) +{ + int i; + + if (def == NULL) + return NULL; + + for (i = 0; i < def->nseclabels; i++) { + if (STREQ_NULLABLE(def->seclabels[i]->model, model)) + return def->seclabels[i]; + } + return NULL; +} + virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 510406a..84f566f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -966,6 +966,9 @@ struct _virDomainChrDef { virDomainChrSourceDef source; virDomainDeviceInfo info; + + size_t nseclabels; + virSecurityDeviceLabelDefPtr *seclabels; }; enum virDomainSmartcardType { @@ -2123,6 +2126,9 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model); virSecurityDeviceLabelDefPtr virDomainDiskDefGetSecurityLabelDef(virDomainDiskDefPtr def, const char *model); +virSecurityDeviceLabelDefPtr +virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); + virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a7e2420..c47c872 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1213,38 +1213,58 @@ done: static int virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, - virDomainChrSourceDefPtr dev) + virDomainChrDefPtr dev, + virDomainChrSourceDefPtr dev_source) { - virSecurityLabelDefPtr secdef; + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr chr_seclabel = NULL; + char *imagelabel; char *in = NULL, *out = NULL; int ret = -1; - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (seclabel == NULL) return -1; - if (secdef->norelabel) + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_SELINUX_NAME); + + if (seclabel->norelabel || (chr_seclabel && chr_seclabel->norelabel)) return 0; - switch (dev->type) { + if (chr_seclabel) + imagelabel = chr_seclabel->label; + else + imagelabel = seclabel->imagelabel; + + switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecuritySELinuxSetFilecon(dev->data.file.path, secdef->imagelabel); + ret = virSecuritySELinuxSetFilecon(dev_source->data.file.path, imagelabel); + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (!dev_source->data.nix.listen) { + if (virSecuritySELinuxSetFilecon(dev_source->data.file.path, imagelabel) < 0) + goto done; + } + ret = 0; break; case VIR_DOMAIN_CHR_TYPE_PIPE: - if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) || - (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) { + if ((virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0) || + (virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)) { virReportOOMError(); goto done; } if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxSetFilecon(in, secdef->imagelabel) < 0) || - (virSecuritySELinuxSetFilecon(out, secdef->imagelabel) < 0)) { + if ((virSecuritySELinuxSetFilecon(in, imagelabel) < 0) || + (virSecuritySELinuxSetFilecon(out, imagelabel) < 0)) { goto done; } - } else if (virSecuritySELinuxSetFilecon(dev->data.file.path, secdef->imagelabel) < 0) { + } else if (virSecuritySELinuxSetFilecon(dev_source->data.file.path, imagelabel) < 0) { goto done; } ret = 0; @@ -1263,30 +1283,44 @@ done: static int virSecuritySELinuxRestoreSecurityChardevLabel(virDomainDefPtr def, - virDomainChrSourceDefPtr dev) + virDomainChrDefPtr dev, + virDomainChrSourceDefPtr dev_source) { - virSecurityLabelDefPtr secdef; + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; int ret = -1; - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (seclabel == NULL) return -1; - if (secdef->norelabel) + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_SELINUX_NAME); + if (seclabel->norelabel || (chr_seclabel && chr_seclabel->norelabel)) return 0; - switch (dev->type) { + switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - if (virSecuritySELinuxRestoreSecurityFileLabel(dev->data.file.path) < 0) + if (virSecuritySELinuxRestoreSecurityFileLabel(dev_source->data.file.path) < 0) goto done; ret = 0; break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (!dev_source->data.nix.listen) { + if (virSecuritySELinuxRestoreSecurityFileLabel(dev_source->data.file.path) < 0) + goto done; + } + ret = 0; + break; + case VIR_DOMAIN_CHR_TYPE_PIPE: - if ((virAsprintf(&out, "%s.out", dev->data.file.path) < 0) || - (virAsprintf(&in, "%s.in", dev->data.file.path) < 0)) { + if ((virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) || + (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0)) { virReportOOMError(); goto done; } @@ -1295,7 +1329,7 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virDomainDefPtr def, (virSecuritySELinuxRestoreSecurityFileLabel(in) < 0)) { goto done; } - } else if (virSecuritySELinuxRestoreSecurityFileLabel(dev->data.file.path) < 0) { + } else if (virSecuritySELinuxRestoreSecurityFileLabel(dev_source->data.file.path) < 0) { goto done; } ret = 0; @@ -1323,7 +1357,7 @@ virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def, dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) return 0; - return virSecuritySELinuxRestoreSecurityChardevLabel(def, &dev->source); + return virSecuritySELinuxRestoreSecurityChardevLabel(def, dev, &dev->source); } @@ -1345,7 +1379,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, return virSecuritySELinuxRestoreSecurityFileLabel(database); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: - return virSecuritySELinuxRestoreSecurityChardevLabel(def, &dev->data.passthru); + return virSecuritySELinuxRestoreSecurityChardevLabel(def, NULL, &dev->data.passthru); default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1703,7 +1737,7 @@ virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def, dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) return 0; - return virSecuritySELinuxSetSecurityChardevLabel(def, &dev->source); + return virSecuritySELinuxSetSecurityChardevLabel(def, dev, &dev->source); } @@ -1727,7 +1761,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def, return virSecuritySELinuxSetFilecon(database, data->content_context); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: - return virSecuritySELinuxSetSecurityChardevLabel(def, &dev->data.passthru); + return virSecuritySELinuxSetSecurityChardevLabel(def, NULL, &dev->data.passthru); default: virReportError(VIR_ERR_INTERNAL_ERROR, -- 1.7.10.4
On 20.09.2012 17:29, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
This allows the user to control labelling of each character device separately (the default is to inherit from the VM). --- docs/formatdomain.html.in | 8 ++++ src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 6 +++ src/security/security_selinux.c | 86 +++++++++++++++++++++++++++------------ 4 files changed, 147 insertions(+), 30 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 51f897c..939312c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3434,6 +3434,14 @@ qemu-kvm -net nic,model=? /dev/null </p>
<p> + The <code>source</code> element may contain an optional + <code>seclabel</code> to override the way that labelling + is done on the socket path. If this element is not present, + the <a href="#seclabel">security label is inherited from + the per-domain setting</a>. + </p> + + <p> Each character device element has an optional sub-element <code><address></code> which can tie the device to a diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 06b3209..7023897 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1273,6 +1273,8 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
void virDomainChrDefFree(virDomainChrDefPtr def) { + size_t i; + if (!def) return;
@@ -1296,6 +1298,12 @@ void virDomainChrDefFree(virDomainChrDefPtr def) virDomainChrSourceDefClear(&def->source); virDomainDeviceInfoClear(&def->info);
+ if (def->seclabels) { + for (i = 0; i < def->nseclabels; i++) + virSecurityDeviceLabelDefFree(def->seclabels[i]); + VIR_FREE(def->seclabels); + } + VIR_FREE(def); }
@@ -5343,7 +5351,11 @@ error: * <target>, which is used by <serial> but not <smartcard>). */ static int virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, - xmlNodePtr cur, unsigned int flags) + xmlNodePtr cur, unsigned int flags, + virDomainChrDefPtr chr_def, + xmlXPathContextPtr ctxt, + virSecurityLabelDefPtr* vmSeclabels, + int nvmSeclabels) { char *bindHost = NULL; char *bindService = NULL; @@ -5398,6 +5410,21 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, if (def->type == VIR_DOMAIN_CHR_TYPE_UDP) VIR_FREE(mode); } + + /* Check for an optional seclabel override in <source/>. */ + if (chr_def) { + xmlNodePtr saved_node = ctxt->node; + ctxt->node = cur; + if (virSecurityDeviceLabelDefParseXML (&chr_def->seclabels, + &chr_def->nseclabels, + vmSeclabels, + nvmSeclabels, + ctxt) < 0) {
extra space; we don't type space between function name and (
+ ctxt->node = saved_node; + goto error; + } + ctxt->node = saved_node; + } } else if (xmlStrEqual(cur->name, BAD_CAST "protocol")) { if (protocol == NULL) protocol = virXMLPropString(cur, "type"); @@ -5591,7 +5618,10 @@ virDomainChrDefNew(void) { static virDomainChrDefPtr virDomainChrDefParseXML(virCapsPtr caps, virDomainDefPtr vmdef, + xmlXPathContextPtr ctxt, xmlNodePtr node, + virSecurityLabelDefPtr* vmSeclabels, + int nvmSeclabels, unsigned int flags) { xmlNodePtr cur; @@ -5622,7 +5652,8 @@ virDomainChrDefParseXML(virCapsPtr caps, }
cur = node->children; - remaining = virDomainChrSourceDefParseXML(&def->source, cur, flags); + remaining = virDomainChrSourceDefParseXML(&def->source, cur, flags, + def, ctxt, vmSeclabels, nvmSeclabels);
long line
if (remaining < 0) goto error; if (remaining) { @@ -5759,7 +5790,8 @@ virDomainSmartcardDefParseXML(xmlNodePtr node, }
cur = node->children; - if (virDomainChrSourceDefParseXML(&def->data.passthru, cur, flags) < 0) + if (virDomainChrSourceDefParseXML(&def->data.passthru, cur, flags, + NULL, NULL, NULL, 0) < 0) goto error;
if (def->data.passthru.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { @@ -7240,7 +7272,8 @@ virDomainRedirdevDefParseXML(const xmlNodePtr node, if (xmlStrEqual(cur->name, BAD_CAST "source")) { int remaining;
- remaining = virDomainChrSourceDefParseXML(&def->source.chr, cur, flags); + remaining = virDomainChrSourceDefParseXML(&def->source.chr, cur, flags, + NULL, NULL, NULL, 0); if (remaining != 0) goto error; } @@ -9282,7 +9315,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -9309,7 +9345,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -9339,7 +9378,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, bool create_stub = true; virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -9415,7 +9457,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainChrDefPtr chr = virDomainChrDefParseXML(caps, def, + ctxt, nodes[i], + def->seclabels, + def->nseclabels, flags); if (!chr) goto error; @@ -12359,6 +12404,7 @@ virDomainChrDefFormat(virBufferPtr buf, const char *targetType = virDomainChrTargetTypeToString(def->deviceType, def->targetType); bool tty_compat; + size_t n;
int ret = 0;
@@ -12438,6 +12484,14 @@ virDomainChrDefFormat(virBufferPtr buf, return -1; }
+ /* Security label overrides, if any. */ + if (def->seclabels && def->nseclabels > 0) { + virBufferAdjustIndent(buf, 2); + for (n = 0; n < def->nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]); + virBufferAdjustIndent(buf, -2); + } + virBufferAsprintf(buf, " </%s>\n", elementName);
return ret; @@ -15262,6 +15316,21 @@ virDomainDiskDefGetSecurityLabelDef(virDomainDiskDefPtr def, const char *model) return NULL; }
+virSecurityDeviceLabelDefPtr +virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model) +{ + int i; + + if (def == NULL) + return NULL; + + for (i = 0; i < def->nseclabels; i++) { + if (STREQ_NULLABLE(def->seclabels[i]->model, model)) + return def->seclabels[i]; + } + return NULL; +} + virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 510406a..84f566f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -966,6 +966,9 @@ struct _virDomainChrDef { virDomainChrSourceDef source;
virDomainDeviceInfo info; + + size_t nseclabels; + virSecurityDeviceLabelDefPtr *seclabels; };
enum virDomainSmartcardType { @@ -2123,6 +2126,9 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model); virSecurityDeviceLabelDefPtr virDomainDiskDefGetSecurityLabelDef(virDomainDiskDefPtr def, const char *model);
+virSecurityDeviceLabelDefPtr +virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); + virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a7e2420..c47c872 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1213,38 +1213,58 @@ done:
static int virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, - virDomainChrSourceDefPtr dev) + virDomainChrDefPtr dev, + virDomainChrSourceDefPtr dev_source)
{ - virSecurityLabelDefPtr secdef; + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr chr_seclabel = NULL; + char *imagelabel; char *in = NULL, *out = NULL; int ret = -1;
- secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (seclabel == NULL) return -1;
- if (secdef->norelabel) + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_SELINUX_NAME); + + if (seclabel->norelabel || (chr_seclabel && chr_seclabel->norelabel)) return 0;
- switch (dev->type) { + if (chr_seclabel) + imagelabel = chr_seclabel->label; + else + imagelabel = seclabel->imagelabel; + + switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecuritySELinuxSetFilecon(dev->data.file.path, secdef->imagelabel); + ret = virSecuritySELinuxSetFilecon(dev_source->data.file.path, imagelabel); + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (!dev_source->data.nix.listen) { + if (virSecuritySELinuxSetFilecon(dev_source->data.file.path, imagelabel) < 0)
again long line
+ goto done; + } + ret = 0; break;
case VIR_DOMAIN_CHR_TYPE_PIPE: - if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) || - (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) { + if ((virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0) || + (virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)) { virReportOOMError(); goto done; } if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxSetFilecon(in, secdef->imagelabel) < 0) || - (virSecuritySELinuxSetFilecon(out, secdef->imagelabel) < 0)) { + if ((virSecuritySELinuxSetFilecon(in, imagelabel) < 0) || + (virSecuritySELinuxSetFilecon(out, imagelabel) < 0)) { goto done; } - } else if (virSecuritySELinuxSetFilecon(dev->data.file.path, secdef->imagelabel) < 0) { + } else if (virSecuritySELinuxSetFilecon(dev_source->data.file.path, imagelabel) < 0) {
and again (there are more so I am stopping reporting them). Beside that I haven't spotted anything bad with this patch. Hence ACK if you fix coding style issues. Michal
On 21.09.2012 11:27, Michal Privoznik wrote:
On 20.09.2012 17:29, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
This allows the user to control labelling of each character device separately (the default is to inherit from the VM). --- docs/formatdomain.html.in | 8 ++++ src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 6 +++ src/security/security_selinux.c | 86 +++++++++++++++++++++++++++------------ 4 files changed, 147 insertions(+), 30 deletions(-)
Ah, I gave premature ACK; you should have updated RNG as well. But I believe you can handle it without any special review. So my ACK holds as long as you update the schema and coding style raised by the first review. Michal
On Fri, Sep 21, 2012 at 11:37:08AM +0200, Michal Privoznik wrote:
On 21.09.2012 11:27, Michal Privoznik wrote:
On 20.09.2012 17:29, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
This allows the user to control labelling of each character device separately (the default is to inherit from the VM). --- docs/formatdomain.html.in | 8 ++++ src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 6 +++ src/security/security_selinux.c | 86 +++++++++++++++++++++++++++------------ 4 files changed, 147 insertions(+), 30 deletions(-)
Ah, I gave premature ACK; you should have updated RNG as well. But I believe you can handle it without any special review. So my ACK holds as long as you update the schema and coding style raised by the first review.
I'll post an updated version anyway. Are there style guidelines on long lines? I'm seeing a lot of long lines in the existing code .. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On Fri, Sep 21, 2012 at 10:52:08AM +0100, Richard W.M. Jones wrote:
On Fri, Sep 21, 2012 at 11:37:08AM +0200, Michal Privoznik wrote:
On 21.09.2012 11:27, Michal Privoznik wrote:
On 20.09.2012 17:29, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
This allows the user to control labelling of each character device separately (the default is to inherit from the VM). --- docs/formatdomain.html.in | 8 ++++ src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 6 +++ src/security/security_selinux.c | 86 +++++++++++++++++++++++++++------------ 4 files changed, 147 insertions(+), 30 deletions(-)
Ah, I gave premature ACK; you should have updated RNG as well. But I believe you can handle it without any special review. So my ACK holds as long as you update the schema and coding style raised by the first review.
I'll post an updated version anyway.
Are there style guidelines on long lines? I'm seeing a lot of long lines in the existing code ..
Informally, 80 characters, but as you see we've not really enforced it. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Richard W.M. Jones