On Mon, Mar 24, 2014 at 17:37:07 +0100, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
> ---
> 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;
> }
It would have been nice to split the refactoring (such as the one above)
and the code implementing relabel=no for DAC driver :-)
> @@ -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))
Shouldn't this check for < 0?
> + 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))
Missing < 0 again.
> + 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);
s/, /, /
> }
>
>
> @@ -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;
>
> +
Leftover empty line.
> 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",
The patch looks correct, although I'd be more confident about its
correctness if it was split into more patches doing one thing at a time.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list