[libvirt] [PATCH v2] Introduce Smack security driver for Libvirt
John Ferlan
jferlan at redhat.com
Wed Jul 13 20:19:20 UTC 2016
On 06/30/2016 06:06 AM, Randy Aybar (raybar) wrote:
> Thanks Daniel and Laine for the feedback so far. Below is a brief list of changes since v1 as well as the patch which should be complete this time.
>
>
> For others, introduction and background can be found here along with latest response:
> https://www.redhat.com/archives/libvir-list/2016-May/msg00628.html
>
> Changes since v1:
>
> Security requirements involving kernel patching to bring in a new
> Smack interface are now documented in "docs/drvlxc.html.in”
> Fixed the syntax and spacing as requested and recommended by the
> “make check-syntax” script
> Moved Smack related block in configure.ac to the dedicated m4 file
> (m4/virt-smack.m4)
> Removed the check for Smack-specific driver before calling
> SetChildProcessLabel in lxc_container.c and moved the function call
> outside of the namespaces conditional.
> Replaced calloc/malloc and open/read/close calls with Libvirt specific
> macros in the Smack driver.
> Modified src/Makefile to include security driver libraries when
> compiling libvirt_nss and dependencies
>
> configure.ac | 4 +
> docs/drvlxc.html.in | 14 +-
> m4/virt-smack.m4 | 69 ++
> po/POTFILES.in | 1 +
> src/Makefile.am | 14 +-
> src/lxc/lxc_container.c | 24 +-
> src/lxc/lxc_controller.c | 21 +
> src/security/security_apparmor.c | 9 +
> src/security/security_dac.c | 9 +
> src/security/security_driver.c | 7 +
> src/security/security_driver.h | 4 +
> src/security/security_manager.c | 17 +
> src/security/security_manager.h | 3 +
> src/security/security_nop.c | 8 +
> src/security/security_selinux.c | 9 +
> src/security/security_smack.c | 1484 ++++++++++++++++++++++++++++++++++++++
> src/security/security_smack.h | 37 +
> src/security/security_stack.c | 9 +
> src/util/vircommand.c | 63 ++
> src/util/vircommand.h | 3 +
> 20 files changed, 1798 insertions(+), 11 deletions(-)
> create mode 100644 m4/virt-smack.m4
> create mode 100644 src/security/security_smack.c
> create mode 100644 src/security/security_smack.h
>
> Signed-off-by: Randy Aybar <raybar at cisco.com>
> ---
>
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
> diff --git a/configure.ac b/configure.ac
> index 378069d..c785914 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -255,6 +255,7 @@ LIBVIRT_CHECK_READLINE
> LIBVIRT_CHECK_SANLOCK
> LIBVIRT_CHECK_SASL
> LIBVIRT_CHECK_SELINUX
> +LIBVIRT_CHECK_SMACK
> LIBVIRT_CHECK_SSH2
> LIBVIRT_CHECK_SYSTEMD_DAEMON
> LIBVIRT_CHECK_UDEV
> @@ -1459,6 +1460,7 @@ if test "$with_apparmor" = "no"; then
> fi
> AM_CONDITIONAL([WITH_APPARMOR_PROFILES], [test "$with_apparmor_profiles" != "no"])
>
> +
A rogue extra line?
> dnl DTrace static probes
> AC_ARG_WITH([dtrace],
> [AS_HELP_STRING([--with-dtrace],
> @@ -2754,6 +2756,7 @@ AC_MSG_NOTICE([Security Drivers])
> AC_MSG_NOTICE([])
> AC_MSG_NOTICE([ SELinux: $with_secdriver_selinux ($SELINUX_MOUNT)])
> AC_MSG_NOTICE([AppArmor: $with_secdriver_apparmor (install profiles: $with_apparmor_profiles)])
> +AC_MSG_NOTICE([Smack: $with_secdriver_smack])
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)?
> AC_MSG_NOTICE([])
> AC_MSG_NOTICE([Driver Loadable Modules])
> AC_MSG_NOTICE([])
> @@ -2784,6 +2787,7 @@ LIBVIRT_RESULT_READLINE
> LIBVIRT_RESULT_SANLOCK
> LIBVIRT_RESULT_SASL
> LIBVIRT_RESULT_SELINUX
> +LIBVIRT_RESULT_SMACK
> LIBVIRT_RESULT_SSH2
> LIBVIRT_RESULT_SYSTEMD_DAEMON
> LIBVIRT_RESULT_UDEV
> diff --git a/docs/drvlxc.html.in b/docs/drvlxc.html.in
> index 3dc9d59..f96c9ba 100644
> --- a/docs/drvlxc.html.in
> +++ b/docs/drvlxc.html.in
> @@ -157,8 +157,7 @@ to all containers are
> <li><code>/dev/console</code> symlinked to <code>/dev/pts/0</code></li>
> </ul>
>
> -<p>
> -In addition, for every console defined in the guest configuration,
> +<p> In addition, for every console defined in the guest configuration,
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.
> a symlink will be created from <code>/dev/ttyN</code> symlinked to
> the corresponding <code>/dev/pts/M</code> pseudo TTY device. The
> first console will be <code>/dev/tty1</code>, with further consoles
> @@ -190,6 +189,17 @@ isolation between a container and the host must ensure that they are
> writing a suitable configuration.
> </p>
>
> +<p>
> +NOTE: The SMACK security driver depends on a security interface provided
> +by the SMACK LSM to fully enforced with namespaces. This interface is
> +brought in by the Linux kernel version 4.3. It is recommended to use the
> +appropriate kernel version or backport the below changes to ensure proper
> +opertaion of the SMACK driver with namespaces.
operation
> +
> +Link that references the change in the kernel:
> +<a href="https://lwn.net/Articles/660675/">https://lwn.net/Articles/660675/</a>
> +</p>
> +
So LXC only... is that another build dependency... I've lost track.
> <h3><a name="securenetworking">Network isolation</a></h3>
>
> <p>
> diff --git a/m4/virt-smack.m4 b/m4/virt-smack.m4
> new file mode 100644
> index 0000000..b555733
> --- /dev/null
> +++ b/m4/virt-smack.m4
> @@ -0,0 +1,69 @@
> +dnl The libsmack.so library
> +dnl
> +dnl Copyright (C) 2013 changyaoh.
Using changyaoh on this is strange... Only two other m4/*.m4 files
don't have a Corporation listed (byhve and linker-no-direct).
> +dnl
> +dnl This library is free software; you can redistribute it and/or
> +dnl modify it under the terms of the GNU Lesser General Public
> +dnl License as published by the Free Software Foundation; either
> +dnl version 2.1 of the License, or (at your option) any later version.
> +dnl
> +dnl This library is distributed in the hope that it will be useful,
> +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +dnl Lesser General Public License for more details.
> +dnl
> +dnl You should have received a copy of the GNU Lesser General Public
> +dnl License along with this library. If not, see
> +dnl <http://www.gnu.org/licenses/>.
> +dnl
> +
> +AC_DEFUN([LIBVIRT_CHECK_SMACK],[
> + LIBVIRT_CHECK_LIB([SMACK], [smack],
> + [smack_set_label_for_self], [sys/smack.h])
> +
> + AC_ARG_WITH([secdriver-smack],
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...
> + [AS_HELP_STRING([--with-secdriver-smack],
> + [use Smack security driver @<:@default=check@:>@])],
> + [],
> + [with_secdriver_smack=check])
> +
> + if test "$with_smack" != "yes" ; then
> + if test "$with_secdriver_smack" = "check" ; then
> + with_secdriver_smack=no
> + fi
> + if test "$with_secdriver_smack" != "no" ; then
> + AC_MSG_ERROR([You must install the Smack development package in order to compile libvirt])
> + fi
> + elif test "with_secdriver_smack" != "no" ; then
> + with_secdriver_smack=yes
> + AC_DEFINE_UNQUOTED([WITH_SECDRIVER_SMACK], 1, [whether Smack security driver is available])
> + fi
> + AM_CONDITIONAL([WITH_SECDRIVER_SMACK], [test "$with_secdriver_smack" != "no"])
> +
> +
> +
> + AC_ARG_WITH([smack_mount],
> + [AS_HELP_STRING([--with-smack-mount],
> + [set Smack mount point @<:@default=check@:>@])],
> + [],
> + [with_smack_mount=check])
> +
> + if test "$with_smack" = "yes"; then
> + AC_MSG_CHECKING([Smack mount point])
> + if test "$with_smack_mount" = "check" || test -z "$with_smack_mount"; then
> + if test -d /sys/fs/smackfs ; then
> + SMACK_MOUNT=/sys/fs/smackfs
> + else
> + SMACK_MOUNT=/smack
> + fi
In the original review, Daniel asked/noted that if using /smack is
necessary.... If not then just go with /sys/fs/smackfs...
> + else
> + SMACK_MOUNT=$with_smack_mount
> + fi
> + AC_MSG_RESULT([$SMACK_MOUNT])
> + AC_DEFINE_UNQUOTED([SMACK_MOUNT], ["$SMACK_MOUNT"], [Smack mount point])
> + fi
> +])
> +
> +AC_DEFUN([LIBVIRT_RESULT_SMACK],[
> + LIBVIRT_RESULT_LIB([SMACK])
> +])
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 506d535..99e6d6e 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -159,6 +159,7 @@ src/security/security_dac.c
> src/security/security_driver.c
> src/security/security_manager.c
> src/security/security_selinux.c
> +src/security/security_smack.c
> src/security/virt-aa-helper.c
> src/storage/parthelper.c
> src/storage/storage_backend.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 314f6df..9e53ed9 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -68,6 +68,10 @@ if WITH_SECDRIVER_APPARMOR
> SECDRIVER_CFLAGS += $(APPARMOR_CFLAGS)
> SECDRIVER_LIBS += $(APPARMOR_LIBS)
> endif WITH_SECDRIVER_APPARMOR
> +if WITH_SECDRIVER_SMACK
> +SECDRIVER_CFLAGS += $(SMACK_CFLAGS)
> +SECDRIVER_LIBS += $(SMACK_LIBS)
> +endif WITH_SECDRIVER_SMACK
>
> if WITH_NETWORK
> UUID=$(shell uuidgen 2>/dev/null)
> @@ -1017,6 +1021,9 @@ SECURITY_DRIVER_SELINUX_SOURCES = \
> SECURITY_DRIVER_APPARMOR_SOURCES = \
> security/security_apparmor.h security/security_apparmor.c
>
> +SECURITY_DRIVER_SMACK_SOURCES = \
> + security/security_smack.h security/security_smack.c
> +
> ACCESS_DRIVER_GENERATED = \
> access/viraccessapicheck.h \
> access/viraccessapicheck.c \
> @@ -1765,6 +1772,10 @@ if WITH_SECDRIVER_APPARMOR
> libvirt_security_manager_la_SOURCES += $(SECURITY_DRIVER_APPARMOR_SOURCES)
> libvirt_security_manager_la_CFLAGS += $(APPARMOR_CFLAGS)
> endif WITH_SECDRIVER_APPARMOR
> +if WITH_SECDRIVER_SMACK
> +libvirt_security_manager_la_SOURCES += $(SECURITY_DRIVER_SMACK_SOURCES)
> +libvirt_security_manager_la_CFLAGS += $(SMACK_CFLAGS)
> +endif WITH_SECDRIVER_SMACK
>
> libvirt_driver_access_la_SOURCES = \
> $(ACCESS_DRIVER_SOURCES) $(ACCESS_DRIVER_GENERATED)
> @@ -1896,6 +1907,7 @@ EXTRA_DIST += \
> $(NWFILTER_DRIVER_SOURCES) \
> $(SECURITY_DRIVER_SELINUX_SOURCES) \
> $(SECURITY_DRIVER_APPARMOR_SOURCES) \
> + $(SECURITY_DRIVER_SMACK_SOURCES) \
> $(SECRET_DRIVER_SOURCES) \
> $(SECRET_UTIL_SOURCES) \
> $(VBOX_DRIVER_EXTRA_DIST) \
> @@ -3030,12 +3042,12 @@ libvirt_nss_la_SOURCES = \
> libvirt_nss_la_CFLAGS = \
> -DLIBVIRT_NSS \
> $(AM_CFLAGS) \
> + $(SECDRIVER_LIBS) \
> $(YAJL_CFLAGS) \
> $(NULL)
> libvirt_nss_la_LDFLAGS = \
> $(AM_LDFLAGS) \
> $(NULL)
> -
> libvirt_nss_la_LIBADD = \
> $(YAJL_LIBS) \
> $(NULL)
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index a909b66..ff0e461 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2206,6 +2206,10 @@ static int lxcContainerChild(void *data)
> if (lxcContainerSetID(vmDef) < 0)
> goto cleanup;
>
> + VIR_DEBUG("Setting up security labeling");
> + if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0)
> + goto cleanup;
> +
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.
> root = virDomainGetFilesystemForTarget(vmDef, "/");
>
> if (argv->nttyPaths) {
> @@ -2254,20 +2258,12 @@ static int lxcContainerChild(void *data)
> goto cleanup;
> }
>
> - /* drop a set of root capabilities */
> - if (lxcContainerDropCapabilities(vmDef, !!hasReboot) < 0)
> - goto cleanup;
> -
> if (lxcContainerSendContinue(argv->handshakefd) < 0) {
> virReportSystemError(errno, "%s",
> _("Failed to send continue signal to controller"));
> goto cleanup;
> }
>
> - VIR_DEBUG("Setting up security labeling");
> - if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0)
> - goto cleanup;
> -
> VIR_DEBUG("Setting up inherited FDs");
> VIR_FORCE_CLOSE(argv->handshakefd);
> VIR_FORCE_CLOSE(argv->monitor);
> @@ -2275,6 +2271,10 @@ static int lxcContainerChild(void *data)
> argv->npassFDs, argv->passFDs) < 0)
> goto cleanup;
>
> + /* drop a set of root capabilities */
> + if (lxcContainerDropCapabilities(vmDef, !!hasReboot) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> VIR_FREE(ttyPath);
> @@ -2396,6 +2396,14 @@ int lxcContainerStart(virDomainDefPtr def,
> return -1;
> }
> }
> +
> + VIR_DEBUG("Setting up security labeling");
> + if (virSecurityManagerSetChildProcessLabel(securityDriver, def, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to send label to relabel interface."));
Ran this through coverity - you need a VIR_FREE(stack); here like other
such error/return -1;
> + return -1;
> + }
> +
> if (!nsInheritFDs || nsInheritFDs[VIR_LXC_DOMAIN_NAMESPACE_SHARENET] == -1) {
> if (lxcNeedNetworkNamespace(def)) {
> VIR_DEBUG("Enable network namespaces");
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 0304354..026762e 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -1484,6 +1484,9 @@ static int virLXCControllerSetupDev(virLXCControllerPtr ctrl)
> if (lxcContainerChown(ctrl->def, dev) < 0)
> goto cleanup;
>
> + if (virSecurityManagerSetImagePathLabel(ctrl->securityManager, ctrl->def, dev) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> VIR_FREE(opts);
> @@ -1532,6 +1535,11 @@ static int virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
> if (lxcContainerChown(ctrl->def, path) < 0)
> goto cleanup;
>
> + if (virSecurityManagerSetImagePathLabel(ctrl->securityManager,
> + ctrl->def,
> + path) < 0)
> + goto cleanup;
> +
> VIR_FREE(path);
> }
>
> @@ -2190,6 +2198,14 @@ virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl)
> (lxcContainerChown(ctrl->def, devpts) < 0))
> goto cleanup;
>
> + if ((virSecurityManagerSetImagePathLabel(ctrl->securityManager,
> + ctrl->def,
> + ctrl->devptmx)) < 0 ||
> + (virSecurityManagerSetImagePathLabel(ctrl->securityManager,
> + ctrl->def,
> + devpts) < 0))
More spacing/indention changes, e.g. line up arguments)
> + goto cleanup;
> +
> ret = 0;
>
> cleanup:
> @@ -2234,6 +2250,11 @@ virLXCControllerSetupConsoles(virLXCControllerPtr ctrl,
> if (lxcContainerChown(ctrl->def, ttyHostPath) < 0)
> goto cleanup;
>
> + if (virSecurityManagerSetImagePathLabel(ctrl->securityManager,
> + ctrl->def,
> + ttyHostPath) < 0)
Again, line up your arguments
> + goto cleanup;
> +
> VIR_FREE(ttyHostPath);
> }
>
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index af2b639..bffcf83 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -986,6 +986,14 @@ AppArmorSetFDLabel(virSecurityManagerPtr mgr,
> return reload_profile(mgr, def, fd_path, true);
> }
>
> +static int
> +AppArmorSetPathLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def ATTRIBUTE_UNUSED,
> + const char *path ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
> +
All these domainSetSecurityImagePathLabel seem to be "new" functions and
could be their own/separate patch (prior to smack driver introduction)
> static char *
> AppArmorGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> virDomainDefPtr vm ATTRIBUTE_UNUSED)
> @@ -1043,6 +1051,7 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel,
>
> .domainSetSecurityImageFDLabel = AppArmorSetFDLabel,
> + .domainSetSecurityImagePathLabel = AppArmorSetPathLabel,
> .domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
>
> .domainGetSecurityMountOptions = AppArmorGetMountOptions,
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index df3ed47..0ada728 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1519,6 +1519,14 @@ virSecurityDACSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> }
>
> static int
> +virSecurityDACSetImagePathLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def ATTRIBUTE_UNUSED,
> + const char *path ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
> +
> +static int
> virSecurityDACSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> virDomainDefPtr def ATTRIBUTE_UNUSED,
> int fd ATTRIBUTE_UNUSED)
> @@ -1601,6 +1609,7 @@ virSecurityDriver virSecurityDriverDAC = {
> .domainRestoreSavedStateLabel = virSecurityDACRestoreSavedStateLabel,
>
> .domainSetSecurityImageFDLabel = virSecurityDACSetImageFDLabel,
> + .domainSetSecurityImagePathLabel = virSecurityDACSetImagePathLabel,
> .domainSetSecurityTapFDLabel = virSecurityDACSetTapFDLabel,
>
> .domainGetSecurityMountOptions = virSecurityDACGetMountOptions,
> diff --git a/src/security/security_driver.c b/src/security/security_driver.c
> index 4800d52..3ca3766 100644
> --- a/src/security/security_driver.c
> +++ b/src/security/security_driver.c
> @@ -35,6 +35,10 @@
> # include "security_apparmor.h"
> #endif
>
> +#ifdef WITH_SECDRIVER_SMACK
> +# include "security_smack.h"
> +#endif
> +
> #include "security_nop.h"
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
> @@ -48,6 +52,9 @@ static virSecurityDriverPtr security_drivers[] = {
> #ifdef WITH_SECDRIVER_APPARMOR
> &virAppArmorSecurityDriver,
> #endif
> +#ifdef WITH_SECDRIVER_SMACK
> + &virSecurityDriverSmack,
> +#endif
> &virSecurityDriverNop, /* Must always be last, since it will always probe */
> };
>
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index 7cb62f0..97c0c30 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -104,6 +104,9 @@ typedef int (*virSecurityDomainSecurityVerify) (virSecurityManagerPtr mgr,
> typedef int (*virSecurityDomainSetImageFDLabel) (virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> int fd);
> +typedef int (*virSecurityDomainSetImagePathLabel) (virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + const char *path);
> typedef int (*virSecurityDomainSetTapFDLabel) (virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> int fd);
> @@ -165,6 +168,7 @@ struct _virSecurityDriver {
> virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel;
>
> virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel;
> + virSecurityDomainSetImagePathLabel domainSetSecurityImagePathLabel;
> virSecurityDomainSetTapFDLabel domainSetSecurityTapFDLabel;
>
> virSecurityDomainGetMountOptions domainGetSecurityMountOptions;
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index ecb4a40..507c41e 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -916,6 +916,23 @@ virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr,
> return -1;
> }
>
One extra line here (e.g. 2 lines between functions is the norm)
> +int
> +virSecurityManagerSetImagePathLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr vm,
> + const char* path)
> +{
> + if (mgr->drv->domainSetSecurityImagePathLabel) {
> + int ret;
> + virObjectLock(mgr);
> + ret = mgr->drv->domainSetSecurityImagePathLabel(mgr, vm, path);
> + virObjectUnlock(mgr);
> + return ret;
> + }
> +
> + virReportUnsupportedError();
> + return -1;
> +}
> +
>
> int
> virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr,
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 4cbc2d8..886d00a 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -143,6 +143,9 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr,
> int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> int fd);
> +int virSecurityManagerSetImagePathLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + const char *path);
> int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr,
> virDomainDefPtr vm,
> int fd);
> diff --git a/src/security/security_nop.c b/src/security/security_nop.c
> index 951125d..3d4d47a 100644
> --- a/src/security/security_nop.c
> +++ b/src/security/security_nop.c
> @@ -236,6 +236,13 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> return 0;
> }
>
> +static int
> +virSecurityDomainSetPathLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def ATTRIBUTE_UNUSED,
> + const char* path ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
>
> virSecurityDriver virSecurityDriverNop = {
> .privateDataLen = 0,
> @@ -277,6 +284,7 @@ virSecurityDriver virSecurityDriverNop = {
> .domainRestoreSavedStateLabel = virSecurityDomainRestoreSavedStateLabelNop,
>
> .domainSetSecurityImageFDLabel = virSecurityDomainSetFDLabelNop,
> + .domainSetSecurityImagePathLabel = virSecurityDomainSetPathLabelNop,
> .domainSetSecurityTapFDLabel = virSecurityDomainSetFDLabelNop,
>
> .domainGetSecurityMountOptions = virSecurityDomainGetMountOptionsNop,
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index b33d54a..56e07ca 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -2467,6 +2467,14 @@ virSecuritySELinuxSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> }
>
> static int
> +virSecuritySELinuxSetImagePathLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def ATTRIBUTE_UNUSED,
> + const char *path ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
> +
> +static int
> virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> int fd)
> @@ -2647,6 +2655,7 @@ virSecurityDriver virSecurityDriverSELinux = {
> .domainRestoreSavedStateLabel = virSecuritySELinuxRestoreSavedStateLabel,
>
> .domainSetSecurityImageFDLabel = virSecuritySELinuxSetImageFDLabel,
> + .domainSetSecurityImagePathLabel = virSecuritySELinuxSetImagePathLabel,
> .domainSetSecurityTapFDLabel = virSecuritySELinuxSetTapFDLabel,
>
> .domainGetSecurityMountOptions = virSecuritySELinuxGetSecurityMountOptions,
> diff --git a/src/security/security_smack.c b/src/security/security_smack.c
> new file mode 100644
> index 0000000..6f43db4
> --- /dev/null
> +++ b/src/security/security_smack.c
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.
> @@ -0,0 +1,1484 @@
> +/*
> + * 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 at bupt.edu.cn>
> + * Changyao Han <changyao at bupt.edu.cn>
> + *
> + * Updated to libvirt v1.2.15: (Original was written for libvirt v1.1.4)
We're now at 2.1.0
> + * Raghuram S. Sudhaakar <rsudhaak at cisco.com>
> + * Randy Aybar <raybar at cisco.com>
> + *
> + * Based on security_selinux.c by James Morris <jmorris at namei.org>
> + * and security_apparmor.c by Jamie Strandboge <jamie at canonical.com>
> + *
> + * Smack scurity driver.
security
> + *
> + */
> +
> +#include <config.h>
> +
> +#include <sys/types.h>
> +#include <sys/xattr.h>
> +#include <sys/stat.h>
> +#include <sys/syscall.h>
> +#include <fcntl.h>
> +#include <sys/smack.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <wait.h>
> +#include <stdlib.h>
> +
> +#include "security_smack.h"
> +#include "virerror.h"
> +#include "viralloc.h"
> +#include "datatypes.h"
> +#include "viruuid.h"
> +#include "virlog.h"
> +#include "virpci.h"
> +#include "virusb.h"
> +#include "virscsi.h"
> +#include "virstoragefile.h"
> +#include "virfile.h"
> +#include "configmake.h"
> +#include "vircommand.h"
> +#include "virhash.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_SECURITY
> +VIR_LOG_INIT("security.security_smack");
> +
> +#define SECURITY_SMACK_VOID_DOI "0"
> +#define SECURITY_SMACK_NAME "smack"
> +
> +typedef struct _SmackCallbackData SmackCallbackData;
> +typedef SmackCallbackData *SmackCallbackDataPtr;
> +
> +struct _SmackCallbackData {
> + virSecurityManagerPtr manager;
> + virSecurityLabelDefPtr secdef;
> +};
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.
> +
> +static char *
> +virSecuritySmackGetLabelName(virDomainDefPtr def)
> +{
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + char *name = NULL;
> +
> + virUUIDFormat(def->uuid, uuidstr);
> + if (virAsprintf(&name, "%s%s", SMACK_PREFIX, uuidstr) < 0)
> + return NULL;
> +
> + return name;
> +}
> +
Generally between functions there are now 2 blank lines. I also note
none of these functions is (well) documented.
> +static int
> +virSecuritySmackGetPIDLabel(pid_t pid, char **label)
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.
> +{
> + char *result, *path;
> + int ret;
> +
> + ret = VIR_ALLOC_N(result, SMACK_LABEL_LEN + 1);
> + if (ret < 0 || result == NULL)
> + return -1;
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..
> + ret = virAsprintf(&path, "/proc/%d/attr/current", pid);
> + if (ret < 0)
> + return -1;
> +
> + ret = virFileReadAll(path, SMACK_LABEL_LEN, label);
> +
> + VIR_FREE(path);
> + return ret;
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;
}
> +}
> +
> +int
static int
> +virSecuritySmackSockCreate(const char *label, const char *attr)
> +{
> + int ret = -1;
> + long int tid;
> + char *path;
> +
> + tid = syscall(SYS_gettid);
> +
> + VIR_DEBUG("/proc/self/task/%ld/attr/%s", tid, attr);
> +
> + if (virAsprintf(&path, "/proc/self/task/%ld/attr/%s", tid, attr) < 0)
> + return -1;
> +
> + VIR_DEBUG("setSockCreate pid is in %d", getpid());
> + VIR_DEBUG("real user ID is in %d", getuid());
> + VIR_DEBUG("effective user ID is in %d", geteuid());
> + VIR_DEBUG("label from self %s", label);
> + VIR_DEBUG("location /proc/self/attr/%s", attr);
> +
> + ret = virFileWriteStr(path, label != NULL ? label : "", 0);
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.
> +
> + VIR_FREE(path);
> + return ret;
> +}
> +
> +static int
> +virSecuritySmackSetPathLabel(const char *path, const char *tlabel)
> +{
> + char * elabel = NULL;
s/* elabel/*elabel/
> +
> + VIR_INFO("Setting Smack label on '%s' to '%s'", path, tlabel);
> +
> + if (smack_set_label_for_path(path, "security.SMACK64", 0, tlabel) < 0) {
^^^^^^^^^^^^^^^^^^
This string seems to be used multiple times - should be a #define at the
top...
> + int setfilelabel_errno = errno;
> +
> + if (smack_new_label_from_path(path, "security.SMACK64", 0, &elabel) >= 0) {
That's a long line, split it into two lines and align appropriately
> + if (STREQ(tlabel, elabel)) {
> + VIR_FREE(elabel);
> + /* It's alright, there's nothing to change anyway. */
> + return 0;
> + }
> + VIR_FREE(elabel);
> + }
> +
> + /* if the error complaint is related to an image hosted on
> + * an nfs mount, or a usbfs/sysfs filesystem not supporting
> + * labelling, then just ignore it & hope for the best.
Comment spacing needs adjustment
> + */
> +
> + if (setfilelabel_errno != EOPNOTSUPP && setfilelabel_errno != ENOTSUP) {
long line (over 80 characters), split in two, align appropriately
> + virReportSystemError(setfilelabel_errno,
> + _("unable to set security context '%s' on '%s'"),
> + tlabel, path);
> + return -1;
> +
> + } else {
> + const char *msg;
> + if ((virFileIsSharedFS(path) == 1)) {
> + msg = _("Setting security context '%s' on '%s' not supported. ");
> + VIR_WARN(msg, tlabel, path);
Long lines... VIR_WARN doesn't need I18N messages (eg, no need for _())
> + } else {
> + VIR_INFO("Setting security context '%s' on '%s' not supported",
> + tlabel, path);
Long line, alignment issue
> +
> + }
> +
No need for extra empty line here
> + }
> +
Same
> + }
> + return 0;
> +
> +}
> +
> +static int
> +virSecuritySmackSetHostdevLabelHelper(const char *file, void *opaque)
> +{
> + virSecurityLabelDefPtr seclabel;
> + virDomainDefPtr def = opaque;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
Could all be on one line "if (!(seclabel = virD...(def, ...)))"
> + return virSecuritySmackSetPathLabel(file, seclabel->imagelabel);
> +}
> +
> +static int
> +virSecuritySmackSetUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
> + const char *file, void *opaque)
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.
> +{
> + return virSecuritySmackSetHostdevLabelHelper(file, opaque);
> +}
> +
> +
> +static int
> +virSecuritySmackSetPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED,
> + const char *file, void *opaque)
> +{
> + return virSecuritySmackSetHostdevLabelHelper(file, opaque);
> +}
> +
> +static int
> +virSecuritySmackSetSCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
> + const char *file, void *opaque)
> +{
> + return virSecuritySmackSetHostdevLabelHelper(file, opaque);
> +}
> +
> +
> +static int
> +virSecuritySmackRestoreFileLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + const char *path)
> +{
> + struct stat buf;
> + int ret = -1;
> + char *newpath = NULL;
> + char ebuf[1024];
> +
> + VIR_INFO("Restoring Smack label on '%s'", path);
> +
> + if (virFileResolveLink(path, &newpath) < 0) {
> + VIR_WARN("cannot resolve symlink %s: %s", path,
> + virStrerror(errno, ebuf, sizeof(ebuf)));
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.
> + goto err;
> + }
> +
> + if (stat(newpath, &buf) != 0) {
You're just looking for existence, right?
if (!virFileExists(newpath))
> + VIR_WARN("cannot stat %s: %s", newpath,
> + virStrerror(errno, ebuf, sizeof(ebuf)));
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.
> + goto err;
s/err/cleanup
> + }
> +
> + ret = virSecuritySmackSetPathLabel(newpath, "smack-unused");
> +
> + err:
s/err/cleanup
> + VIR_FREE(newpath);
> + return ret;
> +}
> +
> +
> +static int
> +virSecuritySmackRestoreUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
> + const char *file,
> + void *opaque)
> +{
> + virSecurityManagerPtr mgr = opaque;
> +
> + return virSecuritySmackRestoreFileLabel(mgr, file);
> +}
> +
> +static int
> +virSecuritySmackRestorePCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED,
> + const char *file,
> + void *opaque)
> +{
> + virSecurityManagerPtr mgr = opaque;
> +
> + return virSecuritySmackRestoreFileLabel(mgr, file);
> +}
> +
> +static int
> +virSecuritySmackRestoreSCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
> + const char *file,
> + void *opaque)
> +{
> + virSecurityManagerPtr mgr = opaque;
> +
> + return virSecuritySmackRestoreFileLabel(mgr, file);
> +}
> +
> +static int
> +virSecuritySmackRestoreImageLabelInt(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virStorageSourcePtr src,
> + bool migrated)
> +{
> + virSecurityLabelDefPtr seclabel;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> +
> + if (seclabel == NULL)
> + return -1;
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.
> +
> + if (!seclabel->relabel)
> + return 0;
> +
> + if (src->readonly || src->shared)
> + return 0;
> +
> + if (!src || src->type == VIR_STORAGE_TYPE_NETWORK)
Should probably use virStorageSourceIsLocalStorage like others...
> + return 0;
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)
> +
> + if (migrated) {
> + int ret = virFileIsSharedFS(src->path);
> + if (ret < 0)
> + return -1;
> + if (ret == 1) {
> + VIR_DEBUG("Skipping image label restore on %s because FS is shared", src->path);
> + return 0;
> + }
> +
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?
> +
> + return virSecuritySmackRestoreFileLabel(mgr, src->path);
> +
> +}
> +
> +static int
> +virSecuritySmackSetFileLabel(int fd, char *tlabel)
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...
> +{
> + char *elabel = NULL;
> +
> + VIR_INFO("Setting Smack label on fd %d to '%s'", fd, tlabel);
> +
> + if (smack_set_label_for_file(fd, "security.SMACK64", tlabel) < 0) {
^^^^^^^^^^^^^^^^^^
Again...
> + int fsetfilelabel_errno = errno;
> +
> + if (smack_new_label_from_file(fd, "security.SMACK64", &elabel) >= 0) {
long line...
> + if (STREQ(tlabel, elabel)) {
> + VIR_FREE(elabel);
> + /* It's alright, there's nothing to change anyway. */
> +
> + return 0;
> + }
> +
> + VIR_FREE(elabel);
> + }
> + /* if the error complaint is related to an image hosted on
> + * an nfs mount, or a usbfs/sysfs filesystem not supporting
> + * labelling, then just ignore it & hope for the best.
> + */
Comment alignment is off
> + if (fsetfilelabel_errno != EOPNOTSUPP) {
> + virReportSystemError(fsetfilelabel_errno,
> + _("unable to set security context '%s' on fd %d"), tlabel, fd);
> + return -1;
> + } else {
> + VIR_INFO("Setting security label '%s' on fd %d not supported",
> + tlabel, fd);
> + }
> + }
> + return 0;
> +}
> +
> +
> +static int
> +virSecuritySmackSetHostdevSubsysLabel(virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + const char *vroot)
> +{
> + int ret = -1;
> +
Spacing in here is *really* off
switch() {
case VIRxxx: {
...
break;
}
case VIRyyy: {
...
break;
}
default:
break;
}
> + switch (dev->source.subsys.type) {
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> + {
> + virUSBDevicePtr usb;
> +
> + if (dev->missing)
> + return 0;
> +
> + usb = virUSBDeviceNew(dev->source.subsys.u.usb.bus,
> + dev->source.subsys.u.usb.device,
> + vroot);
> + if (!usb)
> + goto done;
return -1;
> +
> + ret = virUSBDeviceFileIterate(usb, virSecuritySmackSetUSBLabel, def);
> + virUSBDeviceFree(usb);
> +
> + break;
> + }
> +
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> + {
> + virPCIDevicePtr pci =
> + virPCIDeviceNew(dev->source.subsys.u.pci.addr.domain,
> + dev->source.subsys.u.pci.addr.bus,
> + dev->source.subsys.u.pci.addr.slot,
> + dev->source.subsys.u.pci.addr.function);
> +
> + if (!pci)
> + goto done;
return -1;
> +
> + if (dev->source.subsys.u.pci.backend
> + == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> + char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> +
> + if (!vfioGroupDev) {
> + virPCIDeviceFree(pci);
> + goto done;
return -1;
> + }
> + ret = virSecuritySmackRestorePCILabel(pci, vfioGroupDev, def);
> + VIR_FREE(vfioGroupDev);
> + } else {
> + ret = virPCIDeviceFileIterate(pci, virSecuritySmackSetPCILabel, def);
> + }
> + virPCIDeviceFree(pci);
> + break;
> + }
> +
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> + {
> + virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
> + &(dev->source.subsys.u.scsi.u.host);
> + virSCSIDevicePtr scsi =
> + virSCSIDeviceNew(NULL,
> + scsihostsrc->adapter, scsihostsrc->bus,
> + scsihostsrc->target, scsihostsrc->unit,
> + dev->readonly, dev->shareable);
> +
> + if (!scsi)
> + goto done;
return -1;
> +
> + ret = virSCSIDeviceFileIterate(scsi, virSecuritySmackSetSCSILabel, def);
> + virSCSIDeviceFree(scsi);
> +
> + break;
> + }
> +
> + default:
> + ret = 0;
> + break;
> + }
> +
> + done:
^^^
Shouldn't be necessary if return -1
> + return ret;
> +}
> +
> +static int
> +virSecuritySmackSetHostdevCapsLabel(virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + const char *vroot)
> +{
> + int ret = -1;
> + virSecurityLabelDefPtr seclabel;
> + char *path;
path = NULL;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
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.
> +
> + switch (dev->source.caps.type) {
again w/ the switch/case
> + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE:
> + {
> + if (vroot) {
> + if (virAsprintf(&path, "%s/%s", vroot,
> + dev->source.caps.u.storage.block) < 0)
> + return -1;
> + } else {
> + if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0)
> + return -1;
> + }
> + ret = virSecuritySmackSetPathLabel(path, seclabel->imagelabel);
> + VIR_FREE(path);
allow cleanup later..
> + break;
> + }
> +
> + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC:
> + {
> + if (vroot) {
> + if (virAsprintf(&path, "%s/%s", vroot,
> + dev->source.caps.u.misc.chardev) < 0)
> + return -1;
> + } else {
> + if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0)
> + return -1;
> + }
> + ret = virSecuritySmackSetPathLabel(path, seclabel->imagelabel);
> + VIR_FREE(path);
allow cleanup later
> + break;
> + }
> +
> + default:
> + {
> + ret = 0;
> + break;
> + }
> + }
> +
VIR_FREE(path);
> + return ret;
> +
> +}
> +
> +static int
> +virSecuritySmackSetHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + const char *vroot)
> +{
> + virSecurityLabelDefPtr seclabel;
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
Is -1 what is desired? This repeats *a lot* but differs from other
drivers so I have to ask.
> +
> + if (!seclabel->relabel)
> + return 0;
> +
> + switch (dev->mode) {
Too many spaces for switch/case
> + case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
> + return virSecuritySmackSetHostdevSubsysLabel(def, dev, vroot);
> +
> + case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
> + return virSecuritySmackSetHostdevCapsLabel(def, dev, vroot);
> +
> + default:
> + return 0;
> +
> + }
> +}
> +
> +static int
> +virSecuritySmackRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
> + virDomainHostdevDefPtr dev,
> + const char *vroot)
> +{
> + int ret = -1;
> +
> + switch (dev->source.subsys.type) {
More spacing issues.
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> + {
> + virUSBDevicePtr usb;
> +
> + if (dev->missing)
> + return 0;
> +
> + usb = virUSBDeviceNew(dev->source.subsys.u.usb.bus,
> + dev->source.subsys.u.usb.device,
> + vroot);
> + if (!usb)
> + goto done;
return -1;
> +
> + ret = virUSBDeviceFileIterate(usb, virSecuritySmackRestoreUSBLabel, mgr);
> + virUSBDeviceFree(usb);
> +
> + break;
> + }
> +
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> + {
> + virPCIDevicePtr pci =
> + virPCIDeviceNew(dev->source.subsys.u.pci.addr.domain,
> + dev->source.subsys.u.pci.addr.bus,
> + dev->source.subsys.u.pci.addr.slot,
> + dev->source.subsys.u.pci.addr.function);
> +
> + if (!pci)
> + goto done;
return -1;
> +
> + if (dev->source.subsys.u.pci.backend
> + == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> + char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> +
> + if (!vfioGroupDev) {
> + virPCIDeviceFree(pci);
> + goto done;
return -1;
> + }
> + ret = virSecuritySmackRestorePCILabel(pci, vfioGroupDev, mgr);
> + VIR_FREE(vfioGroupDev);
> + } else {
> + ret = virPCIDeviceFileIterate(pci, virSecuritySmackRestorePCILabel, mgr);
> + }
> + virPCIDeviceFree(pci);
> + break;
> + }
> +
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> + {
> + virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
> + &(dev->source.subsys.u.scsi.u.host);
> + virSCSIDevicePtr scsi =
> + virSCSIDeviceNew(NULL,
> + scsihostsrc->adapter, scsihostsrc->bus,
> + scsihostsrc->target, scsihostsrc->unit,
> + dev->readonly, dev->shareable);
> +
> + if (!scsi)
> + goto done;
return -1;
> +
> + ret = virSCSIDeviceFileIterate(scsi, virSecuritySmackRestoreSCSILabel, mgr);
> + virSCSIDeviceFree(scsi);
> +
> + break;
> + }
> +
> + default:
> + {
> + ret = 0;
> + break;
> + }
> + }
> +
> + done:
shouldn't be necessary if you use return -1;
> + return ret;
> +
> +}
> +
> +static int
> +virSecuritySmackRestoreHostdevCapsLabel(virSecurityManagerPtr mgr,
> + virDomainHostdevDefPtr dev,
> + const char *vroot)
> +{
> + int ret = -1;
> + char *path;
> +
more of the same here...
> + switch (dev->source.caps.type) {
> + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE:
> + {
> + if (vroot) {
> + if (virAsprintf(&path, "%s/%s", vroot,
> + dev->source.caps.u.storage.block) < 0)
> + return -1;
> + } else {
> + if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0)
> + return -1;
> + }
> + ret = virSecuritySmackRestoreFileLabel(mgr, path);
> + VIR_FREE(path);
> + break;
> + }
> +
> + case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC:
> + {
> + if (vroot) {
> + if (virAsprintf(&path, "%s/%s", vroot,
> + dev->source.caps.u.misc.chardev) < 0)
> + return -1;
> + } else {
> + if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0)
> + return -1;
> + }
> + ret = virSecuritySmackRestoreFileLabel(mgr, path);
> + VIR_FREE(path);
> + break;
> + }
> +
> + default:
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int
> +virSecuritySmackRestoreHostdevLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainHostdevDefPtr dev,
> + const char *vroot)
> +{
> + virSecurityLabelDefPtr seclabel;
> +
again...
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + if (!seclabel->relabel)
> + return 0;
> +
> + switch (dev->mode) {
> + 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. */
> +static int
> +virSecuritySmackSecurityDriverOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
> +
> +static int
> +virSecuritySmackSecurityDriverClose(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
> +
> +static const char *
> +virSecuritySmackSecurityDriverGetModel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> +{
> + return SECURITY_SMACK_NAME;
> +}
> +
> +static const char *
> +virSecuritySmackSecurityDriverGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> +{
> + return SECURITY_SMACK_VOID_DOI;
> +}
> +
> +static int
> +virSecuritySmackSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def)
> +{
> + virSecurityLabelDefPtr seclabel;
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + if (STREQ(SECURITY_SMACK_NAME, seclabel->model) != 1) {
I assume you mean STRNEQ and could definitely ditch the != 1
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("security label driver mismatch: "
> + "'%s' model configured for domain, but "
> + "hypervisor driver is '%s'."),
> + seclabel->model, SECURITY_SMACK_NAME);
> + return -1;
> + }
> +
> + if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC) {
> + if (smack_label_length(seclabel->label) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid security label %s"), seclabel->label);
> + return -1;
> + }
> + }
> +
> + return 0;
> +
> +}
> +
> +static int
> +virSecuritySmackSetDiskLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def,
> + virDomainDiskDefPtr disk)
> +{
> + virSecurityLabelDefPtr seclabel;
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> +
> + if (seclabel == NULL)
> + return -1;
> +
> + if (!seclabel->relabel)
> + return 0;
> +
> + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK)
> + return 0;
Shouldn't this be using virStorageSourceIsLocalStorage...
> +
> + VIR_DEBUG("set disk image security label before");
> +
> + if (setxattr(disk->src->path, "security.SMACK64", seclabel->imagelabel,
^^^^^^^^^^^^^^^^^
Seen this before...
> + strlen(seclabel->imagelabel) + 1, 0) < 0)
> + return -1;
> +
> + VIR_DEBUG("disk image %s", disk->src->path);
> + VIR_DEBUG("set disk image security label after");
> +
> + return 0;
> +
> +}
> +
> +static int
> +virSecuritySmackRestoreDiskLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virDomainDiskDefPtr disk)
> +{
> + return virSecuritySmackRestoreImageLabelInt(mgr, def, disk->src, false);
> +}
> +
> +static int
> +virSecuritySmackSetImageLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def,
> + virStorageSourcePtr src)
> +{
> + virSecurityLabelDefPtr seclabel;
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> +
> + if (seclabel == NULL)
> + return -1;
> +
> + if (!seclabel->relabel)
> + return 0;
> +
> + if (src->type == VIR_STORAGE_TYPE_NETWORK)
> + return 0;
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.
> +
> + VIR_DEBUG("set disk image security label before");
> +
> + if (setxattr(src->path, "security.SMACK64", seclabel->imagelabel,
> + strlen(seclabel->imagelabel) + 1, 0) < 0)
> + return -1;
> +
> + VIR_DEBUG("disk image %s", src->path);
> + VIR_DEBUG("set disk image security label after");
Could combine these...
> +
> + return 0;
> +
> +}
> +
> +static int
> +virSecuritySmackRestoreImageLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + virStorageSourcePtr src)
> +{
> + return virSecuritySmackRestoreImageLabelInt(mgr, def, src, false);
> +
> +}
> +
> +static int
> +virSecuritySmackSetDaemonSocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr vm)
> +{
> +
> + return 0;
^^^!!!!
> + virSecurityLabelDefPtr seclabel;
> + char *label = NULL;
> + int ret = -1;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(vm, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + if (seclabel->label == NULL)
> + return 0;
> +
> + if (STREQ(SECURITY_SMACK_NAME, seclabel->model) != 1) {
Similar to previous, STRNEQ
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("security label driver mismatch: "
> + "'%s' model configured for domain, but "
> + "hypervisor driver is '%s'."),
> + seclabel->model, SECURITY_SMACK_NAME);
> + return -1;
> + }
> +
> + if (smack_new_label_from_self(&label) == -1) {
> + virReportSystemError(errno,
> + _("unable to get current process context '%s'"), seclabel->label);
> + goto done;
> + }
> +
> + VIR_DEBUG("SmackSetSecurityDaemonSocketLabel is in %d", getpid());
> + VIR_DEBUG("label from self %s", label);
> +
> +
> + if (virSecuritySmackSockCreate(label, "sockincreate") == -1) {
> + virReportSystemError(errno,
> + _("unable to set socket smack label '%s'"), seclabel->label);
> + goto done;
> + }
> +
> + ret = 0;
> + done:
> +
> + VIR_FREE(label);
> + return ret;
> +
> +}
> +
> +static int
> +virSecuritySmackSetSocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr vm)
> +{
> +
> + virSecurityLabelDefPtr seclabel;
> +
> + return 0;
again !!
> + seclabel = virDomainDefGetSecurityLabelDef(vm, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + if (seclabel->label == NULL)
> + return 0;
> +
> + if (STREQ(SECURITY_SMACK_NAME, seclabel->model) != 1) {
Similar to previous, STRNEQ
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("security label driver mismatch: "
> + "'%s' model configured for domain, but "
> + "hypervisor driver is '%s'."),
> + seclabel->model, SECURITY_SMACK_NAME);
> + return -1;
> + }
> +
> + VIR_DEBUG("Setting VM %s socket label %s", vm->name, seclabel->label);
> +
> + if (virSecuritySmackSockCreate(seclabel->label, "sockoutcreate") == -1) {
> + virReportSystemError(errno,
> + _("unable to set socket smack label '%s'"),
> + seclabel->label);
> + return -1;
> + }
> +
> +
> + return 0;
> +
> +}
> +
> +static int
> +virSecuritySmackClearSocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def)
> +{
> +
> + virSecurityLabelDefPtr seclabel;
> +
> + return 0;
again!!!
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + if (seclabel->label == NULL)
> + return 0;
> +
> + if (STREQ(SECURITY_SMACK_NAME, seclabel->model) != 1) {
Similar to previous, STRNEQ
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("security label driver mismatch: "
> + "'%s' model configured for domain, but "
> + "hypervisor driver is '%s'."),
> + seclabel->model, SECURITY_SMACK_NAME);
> + return -1;
> + }
> +
> + VIR_DEBUG("clear sock label");
> +
> + if (virSecuritySmackSockCreate(NULL, "sockincreate") == -1 ||
> + virSecuritySmackSockCreate(NULL, "sockoutcreate") == -1) {
> + virReportSystemError(errno,
> + _("unable to clear socket smack label '%s'"),
> + seclabel->label);
> +
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> +*Current called in qemuStartVMDaemon to setup a 'label'. We make the
> +*label based on UUID.
> +*this is called on 'start'with RestoreSecurityLabel being called on
> +*shutdown
> + */
Comment alignment
> +static int
> +virSecuritySmackGenLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def)
> +{
> + int ret = -1;
> + char *label_name = NULL;
> + virSecurityLabelDefPtr seclabel;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return ret;
> +
> + VIR_DEBUG("label=%s", virSecurityManagerGetDriver(mgr));
> + if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
> + seclabel->label) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("security label already defined for VM"));
> + return ret;
> + }
> +
> + if (seclabel->imagelabel) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("security image label already defined for VM"));
> + return ret;
> + }
> +
> + if (seclabel->model &&
> + STRNEQ(seclabel->model, SECURITY_SMACK_NAME)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("security label model %s is not supported with smack"),
> + seclabel->model);
> + return ret;
> + }
> +
> + VIR_DEBUG("type=%d", seclabel->type);
> +
> + if ((label_name = virSecuritySmackGetLabelName(def)) == NULL)
> + return ret;
> +
> + if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> +
> + /*set process label*/
> + if (VIR_STRDUP(seclabel->label, label_name) < 0)
Could this overwrite an existing ->label that needs to be free'd first?
> + goto cleanup;
> + }
> +
> + /*set imagelabel the same as label*/
> + if (VIR_STRDUP(seclabel->imagelabel, label_name) < 0)
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;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + 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
> +static int
> +virSecuritySmackGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def ATTRIBUTE_UNUSED,
> + pid_t pid,
> + virSecurityLabelPtr sec)
> +{
> +
> + char *label_name = NULL;
> +
> + if (virSecuritySmackGetPIDLabel(pid, &label_name) == -1) {
> + virReportSystemError(errno,
> + _("unable to get PID %d security label"),
> + pid);
This would overwrite VIR_ALLOC_N or virFileReadAll errors, so it's
unnecessary...
> + return -1;
> + }
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);
> + return -1;
> + }
> +
> + 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;
}
> + VIR_FREE(label_name);
> + /*Smack default enforced*/
Comment should be "/* Smack default enforced */"
> + sec->enforcing = 1;
> +
> + return label_name == NULL ? -1 : 0;
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
> +}
> +
> +static int
> +virSecuritySmackSetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr def)
> +{
> + virSecurityLabelDefPtr seclabel;
> +
> + 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 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);
> +
> + return -1;
> + }
> +
> + if (smack_set_label_for_self(seclabel->label) < 0) {
> + virReportError(errno,
> + _("unable to set security label '%s'"),
> + seclabel->label);
> +
> + return -1;
> + }
> +
> + 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);
> +
> + return -1;
> + }
> +
> + /*
> + * 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;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + if (!seclabel->relabel)
> + return 0;
> +
> + return virSecuritySmackSetPathLabel(savefile, seclabel->imagelabel);
> +}
> +
> +static int
> +virSecuritySmackRestoreSavedStateLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + const char *savefile)
> +{
> + virSecurityLabelDefPtr seclabel;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + 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)
> + return 0;
> +
> + 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;
> +
> + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SMACK_NAME);
> + if (seclabel == NULL)
> + return -1;
> +
> + if (seclabel->label == NULL)
> + return 0;
> +
> +
> + if (fstat(fd, &buf) < 0) {
> + virReportSystemError(errno, _("cannot stat tap fd %d"), fd);
> + return -1;
> + }
> +
> + if ((buf.st_mode & S_IFMT) != S_IFCHR) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("tap fd %d is not character device"), fd);
> + return -1;
> + }
> +
> + 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 at bupt.edu,cn>
> + * Changyao Han <changyao at bupt.edu.cn>
> + * Raghuram S. Sudhaakar <rssudhaakar at gmail.com>
> + * Randy Aybar <raybar at cisco.com>
> + */
> +
> +#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.
> + * VIR_DEBUG("smack label is %s",label);
> + * free(label);
> + *
> + *
> + */
Wouldn't the above hunk need some sort of WITH_*SMACK... too?
> /* The steps above may need to do something privileged, so we delay
> * setuid and clearing capabilities until the last minute.
> */
> @@ -1197,6 +1228,35 @@ virCommandSetAppArmorProfile(virCommandPtr cmd,
> }
>
>
> +
> +/**
> + * virCommandSetSmackLabel:
> + * @cmd: the command to modify
> + * @label: the Smack label to use for the child process
> + *
> + * Saves a copy of @label to use when setting the Smack context
> + * label (write to /proc/self/attr/current ) after the child process has
> + * been started. If Smack isn't compiled into libvirt, or if label is
> + * NULL, nothing will be done.
> + */
> +void
> +virCommandSetSmackLabel(virCommandPtr cmd,
> + const char *label ATTRIBUTE_UNUSED)
> +
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> +#if defined(WITH_SECDRIVER_SMACK)
> + VIR_FREE(cmd->smackLabel);
> + if (VIR_STRDUP_QUIET(cmd->smackLabel, label) < 0)
> + cmd->has_error = ENOMEM;
> +#endif
> + return;
> +
> +}
> +
> +
> /**
> * virCommandDaemonize:
> * @cmd: the command to modify
> @@ -2796,6 +2856,9 @@ virCommandFree(virCommandPtr cmd)
> #if defined(WITH_SECDRIVER_APPARMOR)
> VIR_FREE(cmd->appArmorProfile);
> #endif
> +#if defined(WITH_SECDRIVER_SMACK)
> + VIR_FREE(cmd->smackLabel);
> +#endif
>
> VIR_FREE(cmd);
> }
> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> index 198da2f..dfc8a65 100644
> --- a/src/util/vircommand.h
> +++ b/src/util/vircommand.h
> @@ -88,6 +88,9 @@ void virCommandSetSELinuxLabel(virCommandPtr cmd,
> void virCommandSetAppArmorProfile(virCommandPtr cmd,
> const char *profile);
>
> +void virCommandSetSmackLabel(virCommandPtr cmd,
> + const char *label);
> +
> void virCommandDaemonize(virCommandPtr cmd);
>
> void virCommandNonblockingFDs(virCommandPtr cmd);
>
More information about the libvir-list
mailing list