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 <mpriv...@redhat.com>
> ---
>  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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to