[libvirt] [PATCH v4 0/2] Add <seclabel> to character devices.
The updates compared to the previous version: - All style issues fixed, except for a few long lines where it's just not possible to split the lines and keep the code looking reasonable. - Added RNG. - Fixed the potential segfault on the error path in virSecurityDeviceLabelDefParseXML - Simplified the changes to virSecurityDeviceLabelDefParseXML by using local variables to store the returned values until just before we return, when we assign them to the final '_rtn' parameters. 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. However it also fixes a possible segfault in the original code if VIR_ALLOC_N returns an error and the cleanup code (at the error: label) tries to iterate over the unallocated array (thanks Michal Privoznik for spotting this). --- src/conf/domain_conf.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 947cc7a..26c2042 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3258,29 +3258,30 @@ error: return -1; } +/* Parse the <seclabel> from a disk or character device. */ static int -virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, +virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, + size_t *nseclabels_rtn, virSecurityLabelDefPtr *vmSeclabels, int nvmSeclabels, xmlXPathContextPtr ctxt) { + virSecurityDeviceLabelDefPtr *seclabels; + size_t nseclabels = 0; int n, i, j; xmlNodePtr *list = NULL; 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) { + if (VIR_ALLOC_N(seclabels, n) < 0) { virReportOOMError(); goto error; } + nseclabels = n; for (i = 0; i < n; i++) { - if (VIR_ALLOC(def->seclabels[i]) < 0) { + if (VIR_ALLOC(seclabels[i]) < 0) { virReportOOMError(); goto error; } @@ -3297,7 +3298,7 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, break; } } - def->seclabels[i]->model = model; + seclabels[i]->model = model; } /* Can't use overrides if top-level doesn't allow relabeling. */ @@ -3311,9 +3312,9 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, relabel = virXMLPropString(list[i], "relabel"); if (relabel != NULL) { if (STREQ(relabel, "yes")) { - def->seclabels[i]->norelabel = false; + seclabels[i]->norelabel = false; } else if (STREQ(relabel, "no")) { - def->seclabels[i]->norelabel = true; + seclabels[i]->norelabel = true; } else { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), @@ -3323,30 +3324,34 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def, } VIR_FREE(relabel); } else { - def->seclabels[i]->norelabel = false; + seclabels[i]->norelabel = false; } ctxt->node = list[i]; label = virXPathStringLimit("string(./label)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - def->seclabels[i]->label = label; + seclabels[i]->label = label; - if (label && def->seclabels[i]->norelabel) { + if (label && seclabels[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[i]->model)); goto error; } } VIR_FREE(list); + + *nseclabels_rtn = nseclabels; + *seclabels_rtn = seclabels; + return 0; error: - for (i = 0; i < n; i++) { - virSecurityDeviceLabelDefFree(def->seclabels[i]); + for (i = 0; i < nseclabels; i++) { + virSecurityDeviceLabelDefFree(seclabels[i]); } - VIR_FREE(def->seclabels); + VIR_FREE(seclabels); VIR_FREE(list); return -1; } @@ -3839,7 +3844,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
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 ++++ docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_conf.c | 78 +++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 6 +++ src/security/security_selinux.c | 90 ++++++++++++++++++++++++++++----------- 5 files changed, 155 insertions(+), 30 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 04de870..bc4cc4a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3445,6 +3445,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/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 75afac2..f47fdad 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2454,6 +2454,9 @@ <optional> <attribute name="wiremode"/> </optional> + <optional> + <ref name='devSeclabel'/> + </optional> </element> </zeroOrMore> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 26c2042..4aa08d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1278,6 +1278,8 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, void virDomainChrDefFree(virDomainChrDefPtr def) { + size_t i; + if (!def) return; @@ -1301,6 +1303,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); } @@ -5353,7 +5361,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; @@ -5408,6 +5420,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"); @@ -5601,7 +5628,10 @@ virDomainChrDefNew(void) { static virDomainChrDefPtr virDomainChrDefParseXML(virCapsPtr caps, virDomainDefPtr vmdef, + xmlXPathContextPtr ctxt, xmlNodePtr node, + virSecurityLabelDefPtr* vmSeclabels, + int nvmSeclabels, unsigned int flags) { xmlNodePtr cur; @@ -5632,7 +5662,9 @@ 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) { @@ -5769,7 +5801,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) { @@ -7250,7 +7283,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; } @@ -9319,7 +9353,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; @@ -9346,7 +9383,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; @@ -9376,7 +9416,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; @@ -9452,7 +9495,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; @@ -12396,6 +12442,7 @@ virDomainChrDefFormat(virBufferPtr buf, const char *targetType = virDomainChrTargetTypeToString(def->deviceType, def->targetType); bool tty_compat; + size_t n; int ret = 0; @@ -12475,6 +12522,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; @@ -15310,6 +15365,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 328f9e5..1a61318 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 { @@ -2135,6 +2138,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 1d3a8fe..883a82b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1213,38 +1213,61 @@ done: static int virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, - virDomainChrSourceDefPtr dev) + virDomainChrDefPtr dev, + virDomainChrSourceDefPtr dev_source) { - virSecurityLabelDefPtr secdef; + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr chr_seclabel = NULL; + char *imagelabel = 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) { + if (chr_seclabel) + imagelabel = chr_seclabel->label; + if (!imagelabel) + 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 +1286,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 +1332,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 +1360,8 @@ 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 +1383,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 +1741,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 +1765,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 21.09.2012 12:40, Richard W.M. Jones wrote:
The updates compared to the previous version:
- All style issues fixed, except for a few long lines where it's just not possible to split the lines and keep the code looking reasonable.
- Added RNG.
- Fixed the potential segfault on the error path in virSecurityDeviceLabelDefParseXML
- Simplified the changes to virSecurityDeviceLabelDefParseXML by using local variables to store the returned values until just before we return, when we assign them to the final '_rtn' parameters.
Rich.
ACK to whole patch set. Michal
participants (2)
-
Michal Privoznik -
Richard W.M. Jones