Re: [libvirt] [PATCH] security_dac: Honor norelabel attribute

The inspiration for this patch comes from a question on the list asking if there's a way to not label some disks. Well, in DAC driver there's not. Even if user have requested norelabel: <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/some/dummy/path/test.bin'> <seclabel model='dac' relabel='no'/> </source> <target dev='vdb' bus='virtio'/> <readonly/> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </disk> the DAC driver ignores this completely. When adjusting the code, I realized it's a ragbag with plenty of things that we try to avoid. >From the variety just a few things: callback data were passed as: void params[2]; params[0] = mgr; params[1] = def; Or my favorite - checking for passed pointer being non NULL on each level of the stack, in each callee. As a pattern of readable code the selinux driver was used. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 244 ++++++++++++++++++++++++-------------------- 1 file changed, 131 insertions(+), 113 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9f45063..3b8fe04 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -53,6 +53,15 @@ struct _virSecurityDACData { char *baselabel; }; +typedef struct _virSecurityDACCallbackData virSecurityDACCallbackData; +typedef virSecurityDACCallbackData *virSecurityDACCallbackDataPtr; + +struct _virSecurityDACCallbackData { + virSecurityManagerPtr manager; + virSecurityLabelDefPtr secdef; +}; + + /* returns -1 on error, 0 on success */ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, @@ -81,65 +90,42 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, /* returns 1 if label isn't found, 0 on success, -1 on error */ static int -virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) +virSecurityDACParseIds(virSecurityLabelDefPtr seclabel, + uid_t *uidPtr, gid_t *gidPtr) { - uid_t uid; - gid_t gid; - virSecurityLabelDefPtr seclabel; - - if (def == NULL) - return 1; - - seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (seclabel == NULL || seclabel->label == NULL) { - VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name); + if (!seclabel || !seclabel->label) return 1; - } - if (virParseOwnershipIds(seclabel->label, &uid, &gid) < 0) + if (virParseOwnershipIds(seclabel->label, uidPtr, gidPtr) < 0) return -1; - if (uidPtr) - *uidPtr = uid; - if (gidPtr) - *gidPtr = gid; - return 0; } static int -virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, +virSecurityDACGetIds(virSecurityLabelDefPtr seclabel, + virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr, gid_t **groups, int *ngroups) { int ret; - if (!def && !priv) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to determine default DAC seclabel " - "for an unknown object")); - return -1; - } - if (groups) *groups = priv ? priv->groups : NULL; if (ngroups) *ngroups = priv ? priv->ngroups : 0; - if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) + if ((ret = virSecurityDACParseIds(seclabel, uidPtr, gidPtr)) <= 0) return ret; if (!priv) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("DAC seclabel couldn't be determined " - "for domain '%s'"), def->name); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("DAC seclabel couldn't be determined")); return -1; } - if (uidPtr) - *uidPtr = priv->user; - if (gidPtr) - *gidPtr = priv->group; + *uidPtr = priv->user; + *gidPtr = priv->group; return 0; } @@ -147,60 +133,36 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, /* returns 1 if label isn't found, 0 on success, -1 on error */ static int -virSecurityDACParseImageIds(virDomainDefPtr def, +virSecurityDACParseImageIds(virSecurityLabelDefPtr seclabel, uid_t *uidPtr, gid_t *gidPtr) { - uid_t uid; - gid_t gid; - virSecurityLabelDefPtr seclabel; - - if (def == NULL) - return 1; - - seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (seclabel == NULL || seclabel->imagelabel == NULL) { - VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name); + if (!seclabel || !seclabel->imagelabel) return 1; - } - if (virParseOwnershipIds(seclabel->imagelabel, &uid, &gid) < 0) + if (virParseOwnershipIds(seclabel->imagelabel, uidPtr, gidPtr) < 0) return -1; - if (uidPtr) - *uidPtr = uid; - if (gidPtr) - *gidPtr = gid; - return 0; } static int -virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, +virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, + virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr) { int ret; - if (!def && !priv) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to determine default DAC imagelabel " - "for an unknown object")); - return -1; - } - - if ((ret = virSecurityDACParseImageIds(def, uidPtr, gidPtr)) <= 0) + if ((ret = virSecurityDACParseImageIds(seclabel, uidPtr, gidPtr)) <= 0) return ret; if (!priv) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("DAC imagelabel couldn't be determined " - "for domain '%s'"), def->name); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("DAC imagelabel couldn't be determined")); return -1; } - if (uidPtr) - *uidPtr = priv->user; - if (gidPtr) - *gidPtr = priv->group; + *uidPtr = priv->user; + *gidPtr = priv->group; return 0; }

@@ -324,20 +286,32 @@ err: static int -virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, +virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, const char *path, size_t depth ATTRIBUTE_UNUSED, void *opaque) { - void **params = opaque; - virSecurityManagerPtr mgr = params[0]; - virDomainDefPtr def = params[1]; + virSecurityDACCallbackDataPtr cbdata = opaque; + virSecurityManagerPtr mgr = cbdata->manager; + virSecurityLabelDefPtr secdef = cbdata->secdef; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityDeviceLabelDefPtr disk_seclabel; uid_t user; gid_t group; - if (virSecurityDACGetImageIds(def, priv, &user, &group)) - return -1; + disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + SECURITY_DAC_NAME); + + if (disk_seclabel && disk_seclabel->norelabel) + return 0; + + if (disk_seclabel && disk_seclabel->label) { + if (virParseOwnershipIds(disk_seclabel->label, &user, &group) < 0) + return -1; + } else { + if (virSecurityDACGetImageIds(secdef, priv, &user, &group))

+ return -1; + } return virSecurityDACSetOwnership(path, user, group); } @@ -349,8 +323,9 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk) { - void *params[2]; + virSecurityDACCallbackData cbdata; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; if (!priv->dynamicOwnership) return 0; @@ -358,12 +333,16 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0; - params[0] = mgr; - params[1] = def; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + if (secdef && secdef->norelabel) + return 0; + + cbdata.manager = mgr; + cbdata.secdef = secdef; return virDomainDiskDefForeachPath(disk, false, virSecurityDACSetSecurityFileLabel, - params); + &cbdata); } @@ -428,14 +407,14 @@ static int virSecurityDACSetSecurityHostdevLabelHelper(const char *file, void *opaque) { - void **params = opaque; - virSecurityManagerPtr mgr = params[0]; - virDomainDefPtr def = params[1]; + virSecurityDACCallbackDataPtr cbdata = opaque; + virSecurityManagerPtr mgr = cbdata->manager; + virSecurityLabelDefPtr secdef = cbdata->secdef; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) + if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL)) return -1; return virSecurityDACSetOwnership(file, user, group); @@ -475,8 +454,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, virDomainHostdevDefPtr dev, const char *vroot) { - void *params[] = {mgr, def}; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityDACCallbackData cbdata; int ret = -1; if (!priv->dynamicOwnership) @@ -485,7 +464,13 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) return 0; - switch (dev->source.subsys.type) { + cbdata.manager = mgr; + cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (cbdata.secdef && cbdata.secdef->norelabel) + return 0; + + switch ((enum virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { virUSBDevicePtr usb; @@ -498,8 +483,9 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!usb) goto done; - ret = virUSBDeviceFileIterate(usb, virSecurityDACSetSecurityUSBLabel, - params); + ret = virUSBDeviceFileIterate(usb, + virSecurityDACSetSecurityUSBLabel, + &cbdata); virUSBDeviceFree(usb); break; } @@ -522,11 +508,12 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, virPCIDeviceFree(pci); goto done; } - ret = virSecurityDACSetSecurityPCILabel(pci, vfioGroupDev, params); + ret = virSecurityDACSetSecurityPCILabel(pci, vfioGroupDev, &cbdata); VIR_FREE(vfioGroupDev); } else { - ret = virPCIDeviceFileIterate(pci, virSecurityDACSetSecurityPCILabel, - params); + ret = virPCIDeviceFileIterate(pci, + virSecurityDACSetSecurityPCILabel, + &cbdata); } virPCIDeviceFree(pci); @@ -546,15 +533,15 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!scsi) goto done; - ret = virSCSIDeviceFileIterate(scsi, virSecurityDACSetSecuritySCSILabel, - params); + ret = virSCSIDeviceFileIterate(scsi, + virSecurityDACSetSecuritySCSILabel, + &cbdata); virSCSIDeviceFree(scsi); break; } - default: - ret = 0; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: break; } @@ -606,7 +593,7 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) return 0; - switch (dev->source.subsys.type) { + switch ((enum virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { virUSBDevicePtr usb; @@ -684,34 +671,52 @@ done: static int virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virDomainChrSourceDefPtr dev) + virDomainChrDefPtr dev, + virDomainChrSourceDefPtr dev_source) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; int ret = -1; uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) - return -1; + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - switch (dev->type) { + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_DAC_NAME); + + if (seclabel->norelabel || (chr_seclabel && chr_seclabel->norelabel)) + return 0; + + if (chr_seclabel && chr_seclabel->label) { + if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0) + return -1; + } else { + if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL))

+ return -1; + } + + switch ((enum virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(dev->data.file.path, user, group); + ret = virSecurityDACSetOwnership(dev_source->data.file.path, + user, group); 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)) goto done; if (virFileExists(in) && virFileExists(out)) { if ((virSecurityDACSetOwnership(in, user, group) < 0) || (virSecurityDACSetOwnership(out, user, group) < 0)) { goto done; } - } else if (virSecurityDACSetOwnership(dev->data.file.path, + } else if (virSecurityDACSetOwnership(dev_source->data.file.path, user, group) < 0) { goto done; } @@ -736,7 +741,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, char *in = NULL, *out = NULL; int ret = -1; - switch (dev->type) { + switch ((enum virDomainChrType) dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path); @@ -789,7 +794,7 @@ virSecurityDACSetSecurityTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACSetChardevLabel(mgr, def, + ret = virSecurityDACSetChardevLabel(mgr, def, NULL, &tpm->data.passthrough.source); break; case VIR_DOMAIN_TPM_TYPE_LAST: @@ -885,7 +890,7 @@ virSecurityDACSetChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecurityDACSetChardevLabel(mgr, def, &dev->source); + return virSecurityDACSetChardevLabel(mgr, def, dev, &dev->source);

} @@ -895,13 +900,17 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, const char *stdin_path ATTRIBUTE_UNUSED) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; size_t i; uid_t user; gid_t group; - if (!priv->dynamicOwnership) + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) return 0; +

for (i = 0; i < def->ndisks; i++) { /* XXX fixme - we need to recursively label the entire tree :-( */ if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) @@ -932,7 +941,7 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; } - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(secdef, priv, &user, &group)) return -1; if (def->os.kernel && @@ -956,11 +965,14 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *savefile) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; uid_t user; gid_t group; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (virSecurityDACGetImageIds(secdef, priv, &user, &group)) return -1; return virSecurityDACSetOwnership(savefile, user, group); @@ -985,13 +997,16 @@ static int virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr def ATTRIBUTE_UNUSED) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; uid_t user; gid_t group; gid_t *groups; int ngroups; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (virSecurityDACGetIds(def, priv, &user, &group, &groups, &ngroups)) + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (virSecurityDACGetIds(secdef, priv, &user, &group, &groups, &ngroups)) return -1; VIR_DEBUG("Dropping privileges of DEF to %u:%u, %d supplemental groups", @@ -1009,11 +1024,14 @@ virSecurityDACSetChildProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr def ATTRIBUTE_UNUSED, virCommandPtr cmd) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; uid_t user; gid_t group; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL)) return -1; VIR_DEBUG("Setting child to drop privileges of DEF to %u:%u",