Re: [libvirt] [PATCH v2] Introduce Smack security driver for Libvirt

On 06/30/2016 06:06 AM, Randy Aybar (raybar) wrote: Since it was new/there - I ran the changes through Coverity - only found one new issue (noted below), but I wouldn't be building the smack driver code, so that specific file had more "by hand" checks. I'm certainly not the expert in this area, but figured I could look and help the process along, although based on the sheer volume of comments who knows I could have done more harm... I'm not an configuration or other build environment/files expert, but it seems libvirt.spec.in would need some changes as well especially with respect to source rpm builds and dependency notes listed in these changes (e.g. linux kernel version, some smack package, etc). Typically we try to make separate patches for unrelated changes. There SMACK changes intermixed with other changes here. Make multiple patches, it's OK. Far easier to review *and* if separable enough they can be pushed helping reduce the "final" review load. There's a *lot* of places where the return value is checked vs. == -1, where what the norm to do is check < 0 A rogue extra line? It seems the goal is to line up the ":", so this should be "[ Smack:" (e.g., 3 spaces before Smack) Also similar to SELinux, would this need a second argument e.g. ($SMACK_MOUNT)? Doesn't seem to need a change. Plus other usages are <p> on its own line followed by the paragraph of text. IOW: Drop this change. operation So LXC only... is that another build dependency... I've lost track. Using changyaoh on this is strange... Only two other m4/*.m4 files don't have a Corporation listed (byhve and linker-no-direct). Is this supposed to use _ or - ? I see apparmor and selinux use _ These files are not in my area of any sort of detailed knowledge... In the original review, Daniel asked/noted that if using /smack is necessary.... If not then just go with /sys/fs/smackfs... Is this related? In Daniel's previous noted this was unnecessary. I guess my question would be are these changes related to Smack security driver needs or are they more general purpose. If general purpose, then they could be added on their own. Ran this through coverity - you need a VIR_FREE(stack); here like other such error/return -1; More spacing/indention changes, e.g. line up arguments) Again, line up your arguments All these domainSetSecurityImagePathLabel seem to be "new" functions and could be their own/separate patch (prior to smack driver introduction) One extra line here (e.g. 2 lines between functions is the norm) NB: Since this is "new" I'll be super picky here about stuff that is the "norms" for libvirt that is be part of the check-syntax processing. We're now at 2.1.0 security Indention is off here and continues throughout... it's 4 characters for each indention layer. I'll (try to) mention it just this once, but the whole file needs fixing. Your usage of goto labels deviates from our (mostly) standard "cleanup:" and "error:" labels. If you don't have something to clean up or the error label is merely returning, then to the return rather than a goto label and return. Generally between functions there are now 2 blank lines. I also note none of these functions is (well) documented. Typically, one argument per line, with aligned arguments, e.g.: virSecuritySmackGetPIDLabel(pid_t pid, char **label) I'll mention it once, but it happens multiple times. FWIW: If VIR_ALLOC_N fails, result will be NULL. If VIR_ALLOC_N succeeds, result will be set. No need for the || check, so it could be replaced with: if (VIR_ALLOC_N(result, SMACK_LABEL_LEN + 1) < 0) return -1; However, I don't see how it's used here, so why even have it? Besides 'result' would be leaked on any exit. Unless of course you meant for it to become the target of **label.. Honestly, I'd write the following and adjust callers appropriately static char * virSecuritySmackGetPIDLabel(pid_t pid) { int ret = -1; char *label = NULL; char *path = NULL; if (VIR_ALLOC_N(result, SMACK_LABEL_LEN + 1) < 0) return -1; if (virAsprintf(&path, "/proc/%d/attr/current", pid) < 0) goto error; if (virFileReadAll(path, SMACK_LABEL_LEN, label) < 0) goto error; VIR_FREE(path); return label; cleanup: VIR_FREE(path); VIR_FREE(label); return NULL; } static int Curious what this is used for... Rather than NULL, if the file doesn't exist what would that mean then? That is, why not in remove the file if label == NULL? Perhaps you could add a few intro comments to the function to make it clearer. s/* elabel/*elabel/ ^^^^^^^^^^^^^^^^^^ This string seems to be used multiple times - should be a #define at the top... That's a long line, split it into two lines and align appropriately Comment spacing needs adjustment long line (over 80 characters), split in two, align appropriately Long lines... VIR_WARN doesn't need I18N messages (eg, no need for _()) Long line, alignment issue No need for extra empty line here Same Could all be on one line "if (!(seclabel = virD...(def, ...)))" Using multiple lines - align your arguments one on each line too: virSecuritySmackSetUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, const char *file, void *opaque) Again, happens multiple times, I'll mention it just this once. VIR_WARN? is this an error or just a VIR_DEBUG message? Since you go to err, I would think the former... You could also just return -1 here... newpath will only be allocated if ResolveLink returns 0. You're just looking for existence, right? if (!virFileExists(newpath)) Again, if this is error, then use virReportError and "stat" will no longer be valid, it just cannot find the file. Many examples to look at. s/err/cleanup s/err/cleanup Could be all on one line: if (!(seclabel = virDom...(def, ...))) return -1; But is -1 a valid return here? Other backends don't treat as error. Should probably use virStorageSourceIsLocalStorage like others... If "src" were NULL, then the previous checks would core... It does seem that these disk storage checks should become before the seclabel setting (see apparmor, dac, and selinux each has examples) Extra line I see this chunk is copied from selinux, but that seems to be for a migration specific path... Since IIRC this is targeted more towards LXC do you have the same issues? This is really similar to virSecuritySmackSetPathLabel - I would say I have similar comments for both I think there are some synergies between the two which may be better served by a single helper function that can take the specific 'smack_set_label_for_{path|file}' function as a parameter... Since setfilelabel_errno is used in both, I have a guess as to which came first... ^^^^^^^^^^^^^^^^^^ Again... long line... Comment alignment is off Spacing in here is *really* off switch() { case VIRxxx: { ... break; } case VIRyyy: { ... break; } default: break; } return -1; return -1; return -1; return -1; ^^^ Shouldn't be necessary if return -1 path = NULL; if (!(seclabel ...))) return -1; Is -1 really desired, if selinux is your model, then no... This is repetitive throughout... Check apparmor and dac as well. again w/ the switch/case allow cleanup later.. allow cleanup later VIR_FREE(path); Is -1 what is desired? This repeats *a lot* but differs from other drivers so I have to ask. Too many spaces for switch/case More spacing issues. return -1; return -1; return -1; return -1; > + > + ret = virSCSIDeviceFileIterate(scsi, virSecuritySmackRestoreSCSILabel, mgr); > + virSCSIDeviceFree(scsi); > + > + done: shouldn't be necessary if you use return -1; more of the same here... again... > + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME); > + if (seclabel == NULL) > + return -1; > + case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: > + return virSecuritySmackRestoreHostdevSubsysLabel(mgr, dev, vroot); > + > + case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: > + return virSecuritySmackRestoreHostdevCapsLabel(mgr, dev, vroot); > + > + default: > + return 0; > + } > +} > + > +/*Called on libvirtd startup to see if Smack is available*/ > +static int > +virSecuritySmackSecurityDriverProbe(const char *virtDriver) > +{ > + if (!smack_smackfs_path() || NULL == virtDriver) > + return SECURITY_DRIVER_DISABLE; > + > + return SECURITY_DRIVER_ENABLE; > + > +} > + > +/*Security dirver initialization .*/ /* Security driver initialization. */ I assume you mean STRNEQ and could definitely ditch the != 1 Shouldn't this be using virStorageSourceIsLocalStorage... ^^^^^^^^^^^^^^^^^ Seen this before... Again.. disk->src suff... I think by this point you have plenty to adjust for this file. There's a lot of repetitive changes and I've gone on too long. There are a few more comments later on since I started at the top and started looking for how things were used later on. Making comments in the using function. There's also a few comment/spacing comments. When you find yourself cutting-n-pasting *a lot*, I think it's time for helper functions. Could combine these... ^^^!!!! Similar to previous, STRNEQ again !! Similar to previous, STRNEQ again!!! Similar to previous, STRNEQ Comment alignment Could this overwrite an existing ->label that needs to be free'd first? Could this overwrite something that already exists in ->imagelabel? I see the -> model is checked and not overwritten. > + goto cleanup; > + > + if (!seclabel->model && > + VIR_STRDUP(seclabel->model, SECURITY_SMACK_NAME) < 0) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + > + if (ret != 0) { > + if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) > + VIR_FREE(seclabel->label); > + VIR_FREE(seclabel->imagelabel); > + if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC && > + !seclabel->baselabel) > + VIR_FREE(seclabel->model); > + } > + > + VIR_FREE(label_name); > + > + VIR_DEBUG("model=%s label=%s imagelabel=%s", > + NULLSTR(seclabel->model), > + NULLSTR(seclabel->label), > + NULLSTR(seclabel->imagelabel)); > + > + return ret; > + > +} > + > +static int > +virSecuritySmackReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def ATTRIBUTE_UNUSED, > + pid_t pid ATTRIBUTE_UNUSED) > +{ > + /*Security label is based UUID,*/ > + return 0; > +} > + > +/* > +*Called on VM shutdown and destroy. > +*/ > +static int > +virSecuritySmackReleaseLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def) > +{ > + virSecurityLabelDefPtr seclabel; > + > + if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { > + VIR_FREE(seclabel->label); > + VIR_FREE(seclabel->model); > + } > + VIR_FREE(seclabel->imagelabel); > + > + return 0; > + > +} > + > +/* Seen with 'virsh dominfo <vm>'. This function only called if the VM is > +* running. > +*/ Spacing on comments This would overwrite VIR_ALLOC_N or virFileReadAll errors, so it's unnecessary... Using my suggestion above thus, nets: if (!(label_name = virSecuritySmackGetPIDLabel(pid))) return -1 > + > + if (strlen(label_name) >= VIR_SECURITY_LABEL_BUFLEN) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("security label exceeds " > + "maximum length: %d"), > + VIR_SECURITY_LABEL_BUFLEN - 1); > + VIR_FREE(label_name); > + > + label_name = virStrcpy(sec->label, label_name, VIR_SECURITY_LABEL_BUFLEN); This is usually written as: if (!virStrcpy(sec->label, label_name, VIR_SECURITY_LABEL_BUFLEN)) virReportError(...); VIR_FREE(label_name); return -1; } Comment should be "/* Smack default enforced */" Well at this point label_name will *always* be NULL since VIR_FREE() will do that for you! At this point, it seems you'd just have: return 0; as long as you've followed the above virStrcpy() change Ironic considering other calls (see what I mean about repetitive code) > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("security label driver mismatch: " > + "\'%s\' model configured for domain, but " > + "hypervisor driver is \'%s\'."), > + seclabel->model, SECURITY_SMACK_NAME); > + > + > + if (smack_set_label_for_self(seclabel->label) < 0) { > + virReportError(errno, > + _("unable to set security label '%s'"), > + seclabel->label); > + > + > + return 0; > + > +} > + > +static int > +virSecuritySmackSetChildProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def, > + virCommandPtr cmd) > +{ > + virSecurityLabelDefPtr seclabel; > + int rlbl; > + char *smackfs_path = NULL; > + > + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME); > + > + if (seclabel == NULL) > + return -1; > + > + if (seclabel->label == NULL) > + return 0; > + > + if (STRNEQ(SECURITY_SMACK_NAME, seclabel->model)) { Ironic that you use STRNEQ here > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("security label driver mismatch: " > + "\'%s\' model configured for domain, but " > + "hypervisor driver is \'%s\'."), > + seclabel->model, SECURITY_SMACK_NAME); > + > + > + /* > + * Send label to relabel-self interface to allow child to label > + * its self once it finishes setting up. Apply only if interface is > + * available and user namespace is enabled. > + */ > + > + if (STREQ(virSecurityManagerGetDriver(mgr), "LXC")) { > + > + if (!def->idmap.nuidmap) > + return 0; > + > + VIR_DEBUG("Applying label %s to relabel-self interface.", seclabel->label); > + > + if (virAsprintf(&smackfs_path, "%s/relabel-self", smack_smackfs_path()) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Unable to obtain path for smackfs. Is smack enabled? ")); > + return -1; > + } > + > + rlbl = open(smackfs_path, O_WRONLY); virFileOpen() > + > + if (rlbl < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not open relabel interface \'%s\' for writing. Is it " > + "enabled in the kernel?"), > + smackfs_path); > + return -1; > + } > + > + if (safewrite(rlbl, seclabel->label, strlen(seclabel->label)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not write to relabel interface \'%s\'."), > + smackfs_path); > + return -1; > + } > + > + VIR_FORCE_CLOSE(rlbl); > + > + /* save in cmd to be set after fork/before child process is exec'ed */ > + virCommandSetSmackLabel(cmd, seclabel->label); > + VIR_DEBUG("save smack label in cmd %s", seclabel->label); > + > + return 0; > + > +} > + > +static int > +virSecuritySmackSetAllLabel(virSecurityManagerPtr mgr, > + virDomainDefPtr def, > + const char *stdin_path) > +{ > + > + size_t i; > + virSecurityLabelDefPtr seclabel; > + > + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME); > + > + if (seclabel == NULL) > + return -1; > + > + if (!seclabel->relabel) > + return 0; > + > + VIR_DEBUG("set image security label before"); > + > + for (i = 0; i < def->ndisks; i++) { > + if (def->disks[i]->src->type == VIR_STORAGE_TYPE_DIR) { > + VIR_WARN("Unable to relabel directory tree %s for disk %s", > + def->disks[i]->src->path, def->disks[i]->dst); > + continue; > + } > + > + VIR_DEBUG("set image security label"); > + > + if (virSecuritySmackSetImageLabel(mgr, > + def, def->disks[i]->src) < 0) > + return -1; > + } > + > + VIR_DEBUG("set image security label after"); > + > + for (i = 0; i< def->nhostdevs; i++) { > + if (virSecuritySmackSetHostdevLabel(mgr, > + def, > + def->hostdevs[i], > + NULL) < 0) > + return -1; > + > + } > + > + if (stdin_path) { > + if (setxattr(stdin_path, "security.SMACK64", seclabel->imagelabel, > + strlen(seclabel->imagelabel) + 1, 0)< 0 && > + virFileIsSharedFS(stdin_path) != 1) > + return -1; > + } > + > + return 0; > + > +} > + > +static int > +virSecuritySmackRestoreAllLabel(virSecurityManagerPtr mgr, > + virDomainDefPtr def, > + bool migrated ATTRIBUTE_UNUSED) > +{ > + size_t i; > + virSecurityLabelDefPtr seclabel; > + > + VIR_DEBUG("Restoring security label on %s", def->name); > + > + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME); > + > + if (seclabel == NULL) > + return -1; > + > + if (!seclabel->relabel) > + return 0; > + > + for (i = 0; i < def->ndisks; i++) { > + > + if (virSecuritySmackRestoreImageLabelInt(mgr, > + def, > + def->disks[i]->src, > + migrated) < 0) > + > + return -1; > + > + } > + > + return 0; > + > +} > + > + > +static int > +virSecuritySmackSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def, > + const char *savefile) > +{ > + virSecurityLabelDefPtr seclabel; > + > + if (!seclabel->relabel) > + return 0; > + > + return virSecuritySmackSetPathLabel(savefile, seclabel->imagelabel); > +} > + > +static int > +virSecuritySmackRestoreSavedStateLabel(virSecurityManagerPtr mgr, > + virDomainDefPtr def, > + const char *savefile) > +{ > + virSecurityLabelDefPtr seclabel; > + > + if (!seclabel->relabel) > + return 0; > + > + return virSecuritySmackRestoreFileLabel(mgr, savefile); > +} > + > +static int > +virSecuritySmackSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def, > + int fd) > +{ > + virSecurityLabelDefPtr seclabel; > + > + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME); > + > + if (seclabel == NULL) > + return -1; > + > + if (seclabel->imagelabel == NULL) > + return 0; > + > + return virSecuritySmackSetFileLabel(fd, seclabel->imagelabel); > + > +} > + > +static int > +virSecuritySmackSetImagePathLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def, > + const char *path) > +{ > + virSecurityLabelDefPtr seclabel; > + > + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME); > + > + if (seclabel == NULL) > + return -1; > + > + if (seclabel->imagelabel == NULL) > + > + if (virSecuritySmackSetPathLabel(path, seclabel->imagelabel) < 0) > + return -1; > + > + return 0; > +} > + > +static int > +virSecuritySmackSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def, > + int fd) > +{ > + struct stat buf; > + virSecurityLabelDefPtr seclabel; > + > + if (seclabel->label == NULL) > + return 0; > + > + > + if (fstat(fd, &buf) < 0) { > + virReportSystemError(errno, _("cannot stat tap fd %d"), fd); > + > + if ((buf.st_mode & S_IFMT) != S_IFCHR) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("tap fd %d is not character device"), fd); > + > + return virSecuritySmackSetFileLabel(fd, seclabel->label); > + > +} > + > +static char * > +virSecuritySmackGetSecurityMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr def) > +{ > + char *opts = NULL; > + virSecurityLabelDefPtr seclabel; > + > + if ((seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME))) { > + if (!seclabel->imagelabel) { > + if (!seclabel->label) > + seclabel->imagelabel = virSecuritySmackGetLabelName(def); If NULL is returned, should that be an error? > + else > + seclabel->imagelabel = seclabel->label; > + } > + if (seclabel->imagelabel && > + virAsprintf(&opts, > + ",smackfsdef=\"%s\"", > + (const char*) seclabel->imagelabel) < 0) > + return NULL; > + } > + > + if (!opts && VIR_STRDUP(opts, "") < 0) > + return NULL; > + > + return opts; > + > +} > + > +static const char * > +virSecuritySmackGetBaseLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + int virtType ATTRIBUTE_UNUSED) > +{ > + return NULL; > +} > + > +virSecurityDriver virSecurityDriverSmack = { > + .privateDataLen = 0, > + .name = SECURITY_SMACK_NAME, > + .probe = virSecuritySmackSecurityDriverProbe, > + .open = virSecuritySmackSecurityDriverOpen, > + .close = virSecuritySmackSecurityDriverClose, > + > + .getModel = virSecuritySmackSecurityDriverGetModel, > + .getDOI = virSecuritySmackSecurityDriverGetDOI, > + > + .domainSecurityVerify = virSecuritySmackSecurityVerify, > + > + .domainSetSecurityDiskLabel = virSecuritySmackSetDiskLabel, > + .domainRestoreSecurityDiskLabel = virSecuritySmackRestoreDiskLabel, > + > + .domainSetSecurityImageLabel = virSecuritySmackSetImageLabel, > + .domainRestoreSecurityImageLabel = virSecuritySmackRestoreImageLabel, > + > + .domainSetSecurityDaemonSocketLabel = virSecuritySmackSetDaemonSocketLabel, > + .domainSetSecuritySocketLabel = virSecuritySmackSetSocketLabel, > + .domainClearSecuritySocketLabel = virSecuritySmackClearSocketLabel, > + > + .domainGenSecurityLabel = virSecuritySmackGenLabel, > + .domainReserveSecurityLabel = virSecuritySmackReserveLabel, > + .domainReleaseSecurityLabel = virSecuritySmackReleaseLabel, > + > + .domainGetSecurityProcessLabel = virSecuritySmackGetProcessLabel, > + .domainSetSecurityProcessLabel = virSecuritySmackSetProcessLabel, > + .domainSetSecurityChildProcessLabel = virSecuritySmackSetChildProcessLabel, > + > + .domainSetSecurityAllLabel = virSecuritySmackSetAllLabel, > + .domainRestoreSecurityAllLabel = virSecuritySmackRestoreAllLabel, > + > + .domainSetSecurityHostdevLabel = virSecuritySmackSetHostdevLabel, > + .domainRestoreSecurityHostdevLabel = virSecuritySmackRestoreHostdevLabel, > + > + .domainSetSavedStateLabel = virSecuritySmackSetSavedStateLabel, > + .domainRestoreSavedStateLabel = virSecuritySmackRestoreSavedStateLabel, > + > + .domainSetSecurityImageFDLabel = virSecuritySmackSetImageFDLabel, > + .domainSetSecurityImagePathLabel = virSecuritySmackSetImagePathLabel, > + .domainSetSecurityTapFDLabel = virSecuritySmackSetTapFDLabel, > + > + .domainGetSecurityMountOptions = virSecuritySmackGetSecurityMountOptions, > + > + .getBaseLabel = virSecuritySmackGetBaseLabel, > + > +}; > diff --git a/src/security/security_smack.h b/src/security/security_smack.h > new file mode 100644 > index 0000000..3d9fad9 > --- /dev/null > +++ b/src/security/security_smack.h > @@ -0,0 +1,37 @@ > +/* > + * Copyright (C) 2015 Cisco Systems, Inc. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + * > + * Author: > + * Hongliang Liang <hliang(a)bupt.edu,cn&gt; > + * Changyao Han <changyao(a)bupt.edu.cn&gt; > + * Raghuram S. Sudhaakar <rssudhaakar(a)gmail.com&gt; > + * Randy Aybar <raybar(a)cisco.com&gt; > + */ > + > +#ifndef __VIR_SECURITY_SMACK_H__ > +# define __VIR_SECURITY_SMACK_H__ > + > +# include "security_driver.h" > + > +int virSecuritySmackSockCreate(const char *label, const char *attr); > + > + > +extern virSecurityDriver virSecurityDriverSmack; > + > +# define SMACK_PREFIX "smack-" > + > +#endif /* __VIR_SECURITY_SMACK_H__ */ > diff --git a/src/security/security_stack.c b/src/security/security_stack.c > index 3ea2751..e30f003 100644 > --- a/src/security/security_stack.c > +++ b/src/security/security_stack.c > @@ -495,6 +495,14 @@ virSecurityStackSetImageFDLabel(virSecurityManagerPtr mgr, > } > > static int > +virSecurityStackSetImagePathLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > + virDomainDefPtr vm ATTRIBUTE_UNUSED, > + const char *path ATTRIBUTE_UNUSED) > +{ > + return 0; > +} > + > +static int > virSecurityStackSetTapFDLabel(virSecurityManagerPtr mgr, > virDomainDefPtr vm, > int fd) > @@ -659,6 +667,7 @@ virSecurityDriver virSecurityDriverStack = { > .domainRestoreSavedStateLabel = virSecurityStackRestoreSavedStateLabel, > > .domainSetSecurityImageFDLabel = virSecurityStackSetImageFDLabel, > + .domainSetSecurityImagePathLabel = virSecurityStackSetImagePathLabel, > .domainSetSecurityTapFDLabel = virSecurityStackSetTapFDLabel, > > .domainGetSecurityMountOptions = virSecurityStackGetMountOptions, > diff --git a/src/util/vircommand.c b/src/util/vircommand.c > index 027cb64..cdcb3a2 100644 > --- a/src/util/vircommand.c > +++ b/src/util/vircommand.c > @@ -41,6 +41,9 @@ > #if defined(WITH_SECDRIVER_APPARMOR) > # include <sys/apparmor.h> > #endif > +#if defined(WITH_SECDRIVER_SMACK) > +# include <sys/smack.h> > +#endif > > #define __VIR_COMMAND_PRIV_H_ALLOW__ > #include "vircommandpriv.h" > @@ -134,6 +137,10 @@ struct _virCommand { > #if defined(WITH_SECDRIVER_APPARMOR) > char *appArmorProfile; > #endif > +#if defined(WITH_SECDRIVER_SMACK) > + char *smackLabel; > +#endif > + > int mask; > }; > > @@ -722,6 +729,30 @@ virExec(virCommandPtr cmd) > } > # endif > > +# if defined(WITH_SECDRIVER_SMACK) > + if (cmd->smackLabel) { > + VIR_DEBUG("Setting child security label to %s", cmd->smackLabel); > + > + if (smack_set_label_for_self(cmd->smackLabel) < 0) { > + virReportSystemError(errno, > + _("unable to set Smack label '%s' " > + "for '%s'"), > + cmd->smackLabel, cmd->args[0]); > + goto fork_error; > + } > +# endif > + > +/* > + * if (smack_new_label_from_self(&label) == -1) > + * { > + * goto fork_error; > + * } Strange that this wasn't flagged by the { } parenthesis rule regarding the lack of need when there's only one statement inside. Wouldn't the above hunk need some sort of WITH_*SMACK... too?