[libvirt] [PATCHv2 1/2] security_dac: Rework to make it more readable

While going through the security DAC code, I realized it's a ragbag with plenty of thing we try to avoid. For instance, callback data are passed as: void params[2]; params[0] = mgr; params[1] = def; Moreover, there's no need to pass the whole virDomainDef in the callback as the only thing needed in the callbacks is virSecurityLabelDefPtr. Okay, in some cases the callbacks report error with a domain name, but that can be changed. The other thing, in switch() we ought use enum types as it is much safer when adding a new item to the enum. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 271 +++++++++++++++++++++++--------------------- 1 file changed, 144 insertions(+), 127 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 8512767..8835d49 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; } @@ -329,14 +291,14 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, 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); uid_t user; gid_t group; - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; return virSecurityDACSetOwnership(path, user, group); @@ -349,8 +311,9 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk) { - void *params[2]; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityDACCallbackData cbdata; + virSecurityLabelDefPtr secdef; if (!priv->dynamicOwnership) return 0; @@ -358,12 +321,14 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) return 0; - params[0] = mgr; - params[1] = def; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + cbdata.manager = mgr; + cbdata.secdef = secdef; return virDomainDiskDefForeachPath(disk, false, virSecurityDACSetSecurityFileLabel, - params); + &cbdata); } @@ -429,14 +394,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) < 0) return -1; return virSecurityDACSetOwnership(file, user, group); @@ -476,8 +441,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) @@ -486,7 +451,10 @@ 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); + + switch ((enum virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { virUSBDevicePtr usb; @@ -499,8 +467,9 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!usb) goto done; - ret = virUSBDeviceFileIterate(usb, virSecurityDACSetSecurityUSBLabel, - params); + ret = virUSBDeviceFileIterate(usb, + virSecurityDACSetSecurityUSBLabel, + &cbdata); virUSBDeviceFree(usb); break; } @@ -523,11 +492,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); @@ -547,14 +517,15 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!scsi) goto done; - ret = virSCSIDeviceFileIterate(scsi, virSecurityDACSetSecuritySCSILabel, - params); + ret = virSCSIDeviceFileIterate(scsi, + virSecurityDACSetSecuritySCSILabel, + &cbdata); virSCSIDeviceFree(scsi); break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: ret = 0; break; } @@ -607,7 +578,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; @@ -672,7 +643,7 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: ret = 0; break; } @@ -685,41 +656,65 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, 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 (chr_seclabel && chr_seclabel->label) { + if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0) + return -1; + } else { + if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) + 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; } ret = 0; break; - default: + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_UNIX: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_LAST: ret = 0; break; } @@ -737,7 +732,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); @@ -750,7 +745,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, if (virFileExists(in) && virFileExists(out)) { if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) || (virSecurityDACRestoreSecurityFileLabel(in) < 0)) { - goto done; + goto done; } } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path) < 0) { goto done; @@ -758,7 +753,16 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, ret = 0; break; - default: + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_UNIX: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + case VIR_DOMAIN_CHR_TYPE_LAST: ret = 0; break; } @@ -788,9 +792,9 @@ virSecurityDACSetSecurityTPMFileLabel(virSecurityManagerPtr mgr, { int ret = 0; - switch (tpm->type) { + switch ((enum virDomainTPMBackendType) 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: @@ -807,7 +811,7 @@ virSecurityDACRestoreSecurityTPMFileLabel(virSecurityManagerPtr mgr, { int ret = 0; - switch (tpm->type) { + switch ((enum virDomainTPMBackendType) tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: ret = virSecurityDACRestoreChardevLabel(mgr, &tpm->data.passthrough.source); @@ -880,13 +884,13 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, static int -virSecurityDACSetChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, +virSecurityDACSetChardevCallback(virDomainDefPtr def, virDomainChrDefPtr dev, void *opaque) { virSecurityManagerPtr mgr = opaque; - return virSecurityDACSetChardevLabel(mgr, def, &dev->source); + return virSecurityDACSetChardevLabel(mgr, def, dev, &dev->source); } @@ -896,6 +900,7 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, const char *stdin_path ATTRIBUTE_UNUSED) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; size_t i; uid_t user; gid_t group; @@ -903,6 +908,8 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + for (i = 0; i < def->ndisks; i++) { /* XXX fixme - we need to recursively label the entire tree :-( */ if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR) @@ -933,7 +940,7 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; } - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; if (def->os.kernel && @@ -957,11 +964,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) < 0) return -1; return virSecurityDACSetOwnership(savefile, user, group); @@ -984,15 +994,18 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, static int virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def ATTRIBUTE_UNUSED) + virDomainDefPtr def) { + 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) < 0) return -1; VIR_DEBUG("Dropping privileges of DEF to %u:%u, %d supplemental groups", @@ -1007,14 +1020,17 @@ virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, static int virSecurityDACSetChildProcessLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDefPtr def, 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", @@ -1037,14 +1053,13 @@ static int virSecurityDACGenLabel(virSecurityManagerPtr mgr, virDomainDefPtr def) { - int rc = -1; - virSecurityLabelDefPtr seclabel; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr seclabel; + int rc = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (seclabel == NULL) { + if (!seclabel) return rc; - } if (seclabel->imagelabel) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1062,7 +1077,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, return rc; } - switch (seclabel->type) { + switch ((enum virDomainSeclabelType) seclabel->type) { case VIR_DOMAIN_SECLABEL_STATIC: if (seclabel->label == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1086,7 +1101,8 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_SECLABEL_NONE: /* no op */ return 0; - default: + case VIR_DOMAIN_SECLABEL_DEFAULT: + case VIR_DOMAIN_SECLABEL_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected security label type '%s'"), virDomainSeclabelTypeToString(seclabel->type)); @@ -1119,12 +1135,13 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDefPtr def, pid_t pid ATTRIBUTE_UNUSED, virSecurityLabelPtr seclabel ATTRIBUTE_UNUSED) { - virSecurityLabelDefPtr secdef = - virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + virSecurityLabelDefPtr secdef; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (!secdef || !seclabel) return -1; -- 1.9.0