[libvirt] [PATCH V3 0/7] Honor DAC norelabel attribute
V3 of Michal's series to honor relabel='no' in device config https://www.redhat.com/archives/libvir-list/2014-April/msg00196.html In V3, the patches have been further split to ease review as requested by Jan Tomko. Jim Fehlig (7): security_dac: annotate some functions with ATTRIBUTE_NONNULL security_dac: cleanup use of enum types security_dac: rework callback parameter passing security_dac: avoid relabeling when relabel='no' security_dac: honor relabel='no' in disk config security_dac: avoid relabeling hostdevs when relabel='no' security_dac: honor relabel='no' in chardev config src/security/security_dac.c | 333 +++++++++++++++++++++++++++----------------- 1 file changed, 203 insertions(+), 130 deletions(-) -- 1.8.1.4
Annotate some static function parameters with ATTRIBUTE_NONNULL and remove checks for NULL inputs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index ed79857..19742ed 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -81,10 +81,9 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, /* returns 1 if label isn't found, 0 on success, -1 on error */ static int +ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) { - uid_t uid; - gid_t gid; virSecurityLabelDefPtr seclabel; if (def == NULL) @@ -96,18 +95,14 @@ virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) 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 +ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr, gid_t **groups, int *ngroups) @@ -136,10 +131,8 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return -1; } - if (uidPtr) - *uidPtr = priv->user; - if (gidPtr) - *gidPtr = priv->group; + *uidPtr = priv->user; + *gidPtr = priv->group; return 0; } @@ -147,11 +140,10 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, /* returns 1 if label isn't found, 0 on success, -1 on error */ static int +ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virSecurityDACParseImageIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) { - uid_t uid; - gid_t gid; virSecurityLabelDefPtr seclabel; if (def == NULL) @@ -163,18 +155,14 @@ virSecurityDACParseImageIds(virDomainDefPtr def, 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 +ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr) { @@ -197,10 +185,8 @@ virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return -1; } - if (uidPtr) - *uidPtr = priv->user; - if (gidPtr) - *gidPtr = priv->group; + *uidPtr = priv->user; + *gidPtr = priv->group; return 0; } -- 1.8.1.4
In switch statements, use enum types since it is safer when adding new items to the enum. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- Hmm, may conflict with some of the work I've seen lately to cleanup enum declarations throughout the code. src/security/security_dac.c | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 19742ed..28ffbdb 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -472,7 +472,7 @@ virSecurityDACSetSecurityHostdevLabel(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; @@ -540,7 +540,7 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: ret = 0; break; } @@ -593,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; @@ -658,7 +658,7 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: ret = 0; break; } @@ -683,7 +683,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) return -1; - switch (dev->type) { + switch ((enum virDomainChrType) dev->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecurityDACSetOwnership(dev->data.file.path, user, group); @@ -705,7 +705,17 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, 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_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: ret = 0; break; } @@ -723,7 +733,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); @@ -744,7 +754,17 @@ 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_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: ret = 0; break; } @@ -1047,7 +1067,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, return rc; } - switch (seclabel->type) { + switch ((virDomainSeclabelType) seclabel->type) { case VIR_DOMAIN_SECLABEL_STATIC: if (seclabel->label == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1071,7 +1091,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)); -- 1.8.1.4
Currently, the DAC security driver passes callback data as void params[2]; params[0] = mgr; params[1] = def; Clean this up by defining a structure for passing the callback data. Moreover, there's no need to pass the whole virDomainDef in the callback as the only thing needed in the callbacks is virSecurityLabelDefPtr. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 147 ++++++++++++++++++++++---------------------- 1 file changed, 75 insertions(+), 72 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 28ffbdb..2928c1d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -53,6 +53,14 @@ 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, @@ -82,19 +90,12 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, /* returns 1 if label isn't found, 0 on success, -1 on error */ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) -virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) +virSecurityDACParseIds(virSecurityLabelDefPtr seclabel, + uid_t *uidPtr, gid_t *gidPtr) { - virSecurityLabelDefPtr seclabel; - - if (def == NULL) + if (!seclabel || !seclabel->label) 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); - return 1; - } - if (virParseOwnershipIds(seclabel->label, uidPtr, gidPtr) < 0) return -1; @@ -103,31 +104,24 @@ virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) -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; } @@ -141,20 +135,12 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, /* returns 1 if label isn't found, 0 on success, -1 on error */ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) -virSecurityDACParseImageIds(virDomainDefPtr def, +virSecurityDACParseImageIds(virSecurityLabelDefPtr seclabel, uid_t *uidPtr, gid_t *gidPtr) { - virSecurityLabelDefPtr seclabel; - - if (def == NULL) + if (!seclabel || !seclabel->imagelabel) 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); - return 1; - } - if (virParseOwnershipIds(seclabel->imagelabel, uidPtr, gidPtr) < 0) return -1; @@ -163,25 +149,18 @@ virSecurityDACParseImageIds(virDomainDefPtr def, static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) -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; } @@ -315,14 +294,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)) return -1; return virSecurityDACSetOwnership(path, user, group); @@ -335,8 +314,9 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk) { - void *params[2]; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityDACCallbackData cbdata; + virSecurityLabelDefPtr secdef; if (!priv->dynamicOwnership) return 0; @@ -344,12 +324,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); } @@ -415,14 +397,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); @@ -462,8 +444,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) @@ -472,6 +454,9 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) return 0; + 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; @@ -485,8 +470,9 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!usb) goto done; - ret = virUSBDeviceFileIterate(usb, virSecurityDACSetSecurityUSBLabel, - params); + ret = virUSBDeviceFileIterate(usb, + virSecurityDACSetSecurityUSBLabel, + &cbdata); virUSBDeviceFree(usb); break; } @@ -509,11 +495,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); @@ -533,8 +520,9 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!scsi) goto done; - ret = virSCSIDeviceFileIterate(scsi, virSecurityDACSetSecuritySCSILabel, - params); + ret = virSCSIDeviceFileIterate(scsi, + virSecurityDACSetSecuritySCSILabel, + &cbdata); virSCSIDeviceFree(scsi); break; @@ -675,12 +663,15 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr seclabel; char *in = NULL, *out = NULL; int ret = -1; uid_t user; gid_t group; - if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL)) return -1; switch ((enum virDomainChrType) dev->type) { @@ -902,6 +893,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; @@ -909,6 +901,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) @@ -939,7 +933,7 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; } - if (virSecurityDACGetImageIds(def, priv, &user, &group)) + if (virSecurityDACGetImageIds(secdef, priv, &user, &group)) return -1; if (def->os.kernel && @@ -963,11 +957,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); @@ -992,13 +989,16 @@ static int virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, 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", @@ -1016,11 +1016,14 @@ virSecurityDACSetChildProcessLabel(virSecurityManagerPtr mgr, 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", -- 1.8.1.4
If relabel='no' at the domain level, no need to attempt relabeling in virSecurityDAC{Set,Restore}SecurityAllLabel(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2928c1d..f46b642 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -823,12 +823,14 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, int migrated) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; size_t i; int rc = 0; - if (!priv->dynamicOwnership) - return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + return 0; VIR_DEBUG("Restoring security label on %s migrated=%d", def->name, migrated); @@ -898,11 +900,11 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, uid_t user; gid_t group; - if (!priv->dynamicOwnership) - return 0; - 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 (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR) -- 1.8.1.4
https://bugzilla.redhat.com/show_bug.cgi?id=999301 The DAC driver ignores the relabel='no' attribute in disk config <disk type='file' device='floppy'> <driver name='qemu' type='raw'/> <source file='/some/path/floppy.img'> <seclabel model='dac' relabel='no'/> </source> <target dev='fda' bus='fdc'/> <readonly/> </disk> This patch avoid labeling disks when relabel='no' is specified. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index f46b642..d6ca303 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -289,7 +289,7 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) static int -virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, +virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, const char *path, size_t depth ATTRIBUTE_UNUSED, void *opaque) @@ -298,11 +298,23 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, virSecurityManagerPtr mgr = cbdata->manager; virSecurityLabelDefPtr secdef = cbdata->secdef; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityDeviceLabelDefPtr disk_seclabel; uid_t user; gid_t group; - if (virSecurityDACGetImageIds(secdef, 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); } @@ -326,6 +338,9 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + if (secdef && secdef->norelabel) + return 0; + cbdata.manager = mgr; cbdata.secdef = secdef; return virDomainDiskDefForeachPath(disk, @@ -337,11 +352,13 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, - virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDefPtr def, virDomainDiskDefPtr disk, int migrated) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; + virSecurityDeviceLabelDefPtr disk_seclabel; const char *src = virDomainDiskGetSource(disk); if (!priv->dynamicOwnership) @@ -350,6 +367,17 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (secdef && secdef->norelabel) + return 0; + + disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + SECURITY_DAC_NAME); + + if (disk_seclabel && disk_seclabel->norelabel) + return 0; + /* Don't restore labels on readoly/shared disks, because * other VMs may still be accessing these * Alternatively we could iterate over all running -- 1.8.1.4
When relabel='no' at the domain level, there is no need to call the hostdev relabeling functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index d6ca303..4434cd0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -485,6 +485,9 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, 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; @@ -601,10 +604,13 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; int ret = -1; - if (!priv->dynamicOwnership) - return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + return 0; if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) return 0; -- 1.8.1.4
On 05/16/2014 06:16 AM, Jim Fehlig wrote:
When relabel='no' at the domain level, there is no need to call the hostdev relabeling functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index d6ca303..4434cd0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -485,6 +485,9 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, 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; @@ -601,10 +604,13 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
{ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; int ret = -1;
- if (!priv->dynamicOwnership) - return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + return 0;
Indentation is off here. Jan
The DAC driver ignores the relabel='no' attribute in chardev config <serial type='file'> <source path='/tmp/jim/test.file'> <seclabel model='dac' relabel='no'/> </source> <target port='0'/> </serial> This patch avoids labeling chardevs when relabel='no' is specified. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 19 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4434cd0..20f349f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -693,11 +693,13 @@ 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; @@ -705,25 +707,35 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL)) - return -1; + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_DAC_NAME); - switch ((enum virDomainChrType) dev->type) { + 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; } @@ -753,27 +765,40 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainChrSourceDefPtr dev) + virDomainDefPtr def, + virDomainChrDefPtr dev, + virDomainChrSourceDefPtr dev_source) { + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; int ret = -1; - switch ((enum virDomainChrType) dev->type) { + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_DAC_NAME); + + if (seclabel->norelabel || (chr_seclabel && chr_seclabel->norelabel)) + return 0; + + switch ((enum virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path); + ret = virSecurityDACRestoreSecurityFileLabel(dev_source->data.file.path); 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)) goto done; if (virFileExists(in) && virFileExists(out)) { if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) || (virSecurityDACRestoreSecurityFileLabel(in) < 0)) { goto done; } - } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path) < 0) { + } else if (virSecurityDACRestoreSecurityFileLabel(dev_source->data.file.path) < 0) { goto done; } ret = 0; @@ -802,13 +827,13 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int -virSecurityDACRestoreChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, +virSecurityDACRestoreChardevCallback(virDomainDefPtr def, virDomainChrDefPtr dev, void *opaque) { virSecurityManagerPtr mgr = opaque; - return virSecurityDACRestoreChardevLabel(mgr, &dev->source); + return virSecurityDACRestoreChardevLabel(mgr, def, dev, &dev->source); } @@ -821,7 +846,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: @@ -834,13 +859,14 @@ virSecurityDACSetSecurityTPMFileLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreSecurityTPMFileLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, virDomainTPMDefPtr tpm) { int ret = 0; switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACRestoreChardevLabel(mgr, + ret = virSecurityDACRestoreChardevLabel(mgr, def, NULL, &tpm->data.passthrough.source); break; case VIR_DOMAIN_TPM_TYPE_LAST: @@ -892,6 +918,7 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, if (def->tpm) { if (virSecurityDACRestoreSecurityTPMFileLabel(mgr, + def, def->tpm) < 0) rc = -1; } @@ -919,7 +946,7 @@ virSecurityDACSetChardevCallback(virDomainDefPtr def, { virSecurityManagerPtr mgr = opaque; - return virSecurityDACSetChardevLabel(mgr, def, &dev->source); + return virSecurityDACSetChardevLabel(mgr, def, dev, &dev->source); } -- 1.8.1.4
On 05/16/2014 06:16 AM, Jim Fehlig wrote:
The DAC driver ignores the relabel='no' attribute in chardev config
<serial type='file'> <source path='/tmp/jim/test.file'> <seclabel model='dac' relabel='no'/> </source> <target port='0'/> </serial>
This patch avoids labeling chardevs when relabel='no' is specified.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 19 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4434cd0..20f349f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -705,25 +707,35 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL)) - return -1; + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_DAC_NAME);
A check for seclabel->norelabel and chr_seclabel->norelabel is missing here.
- switch ((enum virDomainChrType) dev->type) { + 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:
Jan
Ján Tomko wrote:
On 05/16/2014 06:16 AM, Jim Fehlig wrote:
The DAC driver ignores the relabel='no' attribute in chardev config
<serial type='file'> <source path='/tmp/jim/test.file'> <seclabel model='dac' relabel='no'/> </source> <target port='0'/> </serial>
This patch avoids labeling chardevs when relabel='no' is specified.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 19 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4434cd0..20f349f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -705,25 +707,35 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL)) - return -1; + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_DAC_NAME);
A check for seclabel->norelabel and chr_seclabel->norelabel is missing here.
virSecurityDACSetChardevLabel() is only called internally, and in all cases via virSecurityDACSetSecurityAllLabel(), which already checks for seclabel->norelabel. But you are right about the missing check for chr_seclabel->norelabel. I added it to the patch before pushing. Regards, Jim
On 05/16/2014 06:16 AM, Jim Fehlig wrote:
V3 of Michal's series to honor relabel='no' in device config
https://www.redhat.com/archives/libvir-list/2014-April/msg00196.html
In V3, the patches have been further split to ease review as requested by Jan Tomko.
Jim Fehlig (7): security_dac: annotate some functions with ATTRIBUTE_NONNULL security_dac: cleanup use of enum types security_dac: rework callback parameter passing security_dac: avoid relabeling when relabel='no' security_dac: honor relabel='no' in disk config security_dac: avoid relabeling hostdevs when relabel='no' security_dac: honor relabel='no' in chardev config
src/security/security_dac.c | 333 +++++++++++++++++++++++++++----------------- 1 file changed, 203 insertions(+), 130 deletions(-)
ACK series with 7/7 fixed. Thanks for splitting these, I forgot about them. Jan
Ján Tomko wrote:
On 05/16/2014 06:16 AM, Jim Fehlig wrote:
V3 of Michal's series to honor relabel='no' in device config
https://www.redhat.com/archives/libvir-list/2014-April/msg00196.html
In V3, the patches have been further split to ease review as requested by Jan Tomko.
Jim Fehlig (7): security_dac: annotate some functions with ATTRIBUTE_NONNULL security_dac: cleanup use of enum types security_dac: rework callback parameter passing security_dac: avoid relabeling when relabel='no' security_dac: honor relabel='no' in disk config security_dac: avoid relabeling hostdevs when relabel='no' security_dac: honor relabel='no' in chardev config
src/security/security_dac.c | 333 +++++++++++++++++++++++++++----------------- 1 file changed, 203 insertions(+), 130 deletions(-)
ACK series with 7/7 fixed.
I fixed the indentation in 6/7, addressed your comment on 7/7 (see my response), and pushed the series. Thanks for the review! Regards, Jim
participants (2)
-
Jim Fehlig -
Ján Tomko