Re: [libvirt] [PATCH v3 2/2] Add <seclabel> to character devices.

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