[libvirt] [PATCH 00/17] Avoid races with udev
I've came across interesting bug recently. The problem was that user tried to start a domain, but qemu was denied access to some device. Even though we relabelled it initially. By debugging I found the root cause: while we were starting qemu, udev came and restored original security labels. Sigh. We have two options here: a) write out series of udev rules so that whenever it tries to relabel something our rule will stop it from doing so b) write a small helper binary that will udev call in order to: 1) detect whether device is in use by libvirt 2) get seclabel that was set by libvirt These patches implement the latter approach. While these patches make life easier for us, there is still a race when udev might restore the device's seclabel before we had chance to flush internal database of seclabels for the helper binary. This is something I'm currently focusing on. But before I get there, here are patches that makes the problem much more bearable. In case you want to try these patches, here are some scratch builds: https://mprivozn.fedorapeople.org/udev/ Also, you can find them on my branch: https://github.com/zippy2/libvirt/commits/udev_labels2 This beast is turned off by default, to turn it on you'll need to add: write_udev=1 to qemu.conf. Michal Privoznik (17): virseclabel.h: Include stdbool.h virseclabel: Introduce virSecurityDeviceLabelDefNewLabel security_dac: Pass manager to virSecurityDACSetImageLabel security_dac: Pass manager to virSecurityDACRestoreFileLabelInternal virudev: Introduce basic skeleton virudev: Implement virUdevMgrAddLabel and virUdevMgrRemoveAllLabels virudev: Introduce virUdevMgrDump tests: Introduce virudevtest virudev: Parse virUdevMgr from JSON virudev: Introduce virUdevMgrLookupLabels util: Introduce libvirt_udevhelper security: Wire up virUdevMgr qemu.conf: Introduce write_udev qemu: Wire up virUdevMgr qemu: Reload virUdevMgr on start virudevtest: Introduce device filtering qemu: Filter uninteresting paths for virUdevMgr libvirt.spec.in | 1 + mingw-libvirt.spec.in | 2 + po/POTFILES.in | 2 + src/Makefile.am | 21 ++ src/libvirt_private.syms | 15 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 + src/qemu/qemu_conf.c | 3 + src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 12 +- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_driver.c | 40 +- src/qemu/qemu_hotplug.c | 35 +- src/qemu/qemu_process.c | 47 ++- src/qemu/qemu_process.h | 3 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/security/security_dac.c | 103 ++++-- src/security/security_manager.c | 16 + src/security/security_manager.h | 5 + src/security/security_selinux.c | 47 ++- src/util/udevhelper.c | 137 +++++++ src/util/virseclabel.c | 14 + src/util/virseclabel.h | 6 + src/util/virudev.c | 588 ++++++++++++++++++++++++++++++ src/util/virudev.h | 63 ++++ tests/Makefile.am | 12 + tests/virudevmock.c | 29 ++ tests/virudevtest.c | 312 ++++++++++++++++ tests/virudevtestdata/complex.json | 30 ++ tests/virudevtestdata/empty.json | 5 + tests/virudevtestdata/simple-dac.json | 13 + tests/virudevtestdata/simple-selinux.json | 13 + 32 files changed, 1535 insertions(+), 54 deletions(-) create mode 100644 src/util/udevhelper.c create mode 100644 src/util/virudev.c create mode 100644 src/util/virudev.h create mode 100644 tests/virudevmock.c create mode 100644 tests/virudevtest.c create mode 100644 tests/virudevtestdata/complex.json create mode 100644 tests/virudevtestdata/empty.json create mode 100644 tests/virudevtestdata/simple-dac.json create mode 100644 tests/virudevtestdata/simple-selinux.json -- 2.8.4
The internal representation of security labels contains some bools (e.g. @relabel, @implicit). But the stdbool.h file is not included anywhere in the header file, therefore if somebody just includes "virseclabel.h" they also need to include <stdbool.h>. That's not right as it should be virseclabel.h who drags in all the dependencies. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virseclabel.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 94c4dfc..558d4eb 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -22,6 +22,8 @@ #ifndef __SECLABEL_H # define __SECLABEL_H +# include <stdbool.h> + typedef enum { VIR_DOMAIN_SECLABEL_DEFAULT, VIR_DOMAIN_SECLABEL_NONE, -- 2.8.4
So far, this function is not needed with the current code. But it is going to do so in subsequent commits. We already have virSecurityDeviceLabelDefNew() which sets just model for new seclabel, this new API sets both model and label. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virseclabel.c | 14 ++++++++++++++ src/util/virseclabel.h | 4 ++++ 3 files changed, 19 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b4210f4..bec2628 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2303,6 +2303,7 @@ virSCSIDeviceSetUsedBy; # util/virseclabel.h virSecurityDeviceLabelDefFree; virSecurityDeviceLabelDefNew; +virSecurityDeviceLabelDefNewLabel; virSecurityLabelDefFree; virSecurityLabelDefNew; diff --git a/src/util/virseclabel.c b/src/util/virseclabel.c index 02a8342..d762a15 100644 --- a/src/util/virseclabel.c +++ b/src/util/virseclabel.c @@ -83,6 +83,20 @@ virSecurityDeviceLabelDefNew(const char *model) return seclabel; } +virSecurityDeviceLabelDefPtr +virSecurityDeviceLabelDefNewLabel(const char *model, + const char *label) +{ + virSecurityDeviceLabelDefPtr seclabel; + + if (!(seclabel = virSecurityDeviceLabelDefNew(model)) || + VIR_STRDUP(seclabel->label, label) < 0) { + virSecurityDeviceLabelDefFree(seclabel); + seclabel = NULL; + } + + return seclabel; +} virSecurityDeviceLabelDefPtr virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 558d4eb..a0af1fd 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -64,6 +64,10 @@ virSecurityDeviceLabelDefPtr virSecurityDeviceLabelDefNew(const char *model); virSecurityDeviceLabelDefPtr +virSecurityDeviceLabelDefNewLabel(const char *model, + const char *label); + +virSecurityDeviceLabelDefPtr virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) ATTRIBUTE_NONNULL(1); -- 2.8.4
This change alone is not needed, but it prepares environment for subsequent patches where we will need virSecurityManager much deeper in the code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index fd74e8b..97be862 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -349,12 +349,13 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, static int -virSecurityDACSetOwnership(virSecurityDACDataPtr priv, +virSecurityDACSetOwnership(virSecurityManagerPtr mgr, virStorageSourcePtr src, const char *path, uid_t uid, gid_t gid) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); struct stat sb; if (!path && src && src->path && @@ -442,7 +443,7 @@ virSecurityDACSetImageLabel(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnership(priv, src, NULL, user, group); + return virSecurityDACSetOwnership(mgr, src, NULL, user, group); } @@ -550,7 +551,7 @@ virSecurityDACSetHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(priv, NULL, file, user, group); + return virSecurityDACSetOwnership(mgr, NULL, file, user, group); } @@ -850,7 +851,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(priv, NULL, + ret = virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, user, group); break; @@ -860,10 +861,10 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACSetOwnership(priv, NULL, in, user, group) < 0 || - virSecurityDACSetOwnership(priv, NULL, out, user, group) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, in, user, group) < 0 || + virSecurityDACSetOwnership(mgr, NULL, out, user, group) < 0) goto done; - } else if (virSecurityDACSetOwnership(priv, NULL, + } else if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, user, group) < 0) { goto done; @@ -873,7 +874,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { - if (virSecurityDACSetOwnership(priv, NULL, + if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, user, group) < 0) goto done; @@ -1033,7 +1034,7 @@ virSecurityDACSetInputLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - ret = virSecurityDACSetOwnership(priv, NULL, input->source.evdev, user, group); + ret = virSecurityDACSetOwnership(mgr, NULL, input->source.evdev, user, group); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1199,27 +1200,27 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.loader && def->os.loader->nvram && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.loader->nvram, user, group) < 0) return -1; if (def->os.kernel && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.kernel, user, group) < 0) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.initrd, user, group) < 0) return -1; if (def->os.dtb && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.dtb, user, group) < 0) return -1; if (def->os.slic_table && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.slic_table, user, group) < 0) return -1; @@ -1242,7 +1243,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; - return virSecurityDACSetOwnership(priv, NULL, savefile, user, group); + return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group); } @@ -1561,7 +1562,7 @@ virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(priv, NULL, path, user, group); + return virSecurityDACSetOwnership(mgr, NULL, path, user, group); } virSecurityDriver virSecurityDriverDAC = { -- 2.8.4
This change alone is not needed, but it prepares environment for subsequent patches where we will need virSecurityManager much deeper in the code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 97be862..7f17124 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -377,10 +377,11 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, static int -virSecurityDACRestoreFileLabelInternal(virSecurityDACDataPtr priv, +virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, virStorageSourcePtr src, const char *path) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int rv; uid_t uid = 0; /* By default return to root:root */ gid_t gid = 0; @@ -405,10 +406,10 @@ virSecurityDACRestoreFileLabelInternal(virSecurityDACDataPtr priv, static int -virSecurityDACRestoreFileLabel(virSecurityDACDataPtr priv, +virSecurityDACRestoreFileLabel(virSecurityManagerPtr mgr, const char *path) { - return virSecurityDACRestoreFileLabelInternal(priv, NULL, path); + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path); } @@ -515,7 +516,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreFileLabelInternal(priv, src, NULL); + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL); } @@ -693,8 +694,7 @@ virSecurityDACRestorePCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virSecurityManagerPtr mgr = opaque; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - return virSecurityDACRestoreFileLabel(priv, file); + return virSecurityDACRestoreFileLabel(mgr, file); } @@ -704,8 +704,7 @@ virSecurityDACRestoreUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virSecurityManagerPtr mgr = opaque; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - return virSecurityDACRestoreFileLabel(priv, file); + return virSecurityDACRestoreFileLabel(mgr, file); } @@ -715,8 +714,7 @@ virSecurityDACRestoreSCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virSecurityManagerPtr mgr = opaque; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - return virSecurityDACRestoreFileLabel(priv, file); + return virSecurityDACRestoreFileLabel(mgr, file); } @@ -908,7 +906,6 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainChrDefPtr dev, virDomainChrSourceDefPtr dev_source) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; int ret = -1; @@ -923,7 +920,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreFileLabel(priv, dev_source->data.file.path); + ret = virSecurityDACRestoreFileLabel(mgr, dev_source->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -931,10 +928,10 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACRestoreFileLabel(priv, out) < 0 || - virSecurityDACRestoreFileLabel(priv, in) < 0) + if (virSecurityDACRestoreFileLabel(mgr, out) < 0 || + virSecurityDACRestoreFileLabel(mgr, in) < 0) goto done; - } else if (virSecurityDACRestoreFileLabel(priv, dev_source->data.file.path) < 0) { + } else if (virSecurityDACRestoreFileLabel(mgr, dev_source->data.file.path) < 0) { goto done; } ret = 0; @@ -1053,12 +1050,11 @@ virSecurityDACRestoreInputLabel(virSecurityManagerPtr mgr, virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainInputDefPtr input) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int ret = -1; switch ((virDomainInputType) input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - ret = virSecurityDACRestoreFileLabel(priv, input->source.evdev); + ret = virSecurityDACRestoreFileLabel(mgr, input->source.evdev); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1126,7 +1122,7 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, } if (def->os.loader && def->os.loader->nvram && - virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) < 0) + virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) rc = -1; return rc; @@ -1257,7 +1253,7 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - return virSecurityDACRestoreFileLabel(priv, savefile); + return virSecurityDACRestoreFileLabel(mgr, savefile); } -- 2.8.4
This is new internal class that is going to remember <device,[array of seclabels]> pairs. Moreover, it is going to be able to flush the pairs into a file so that a helper (which is introduced later in the series) can look into the file and answer question: "Is this path in use by libvirt and if so what security labels should it have?" You can say that we already have security drivers for that. And you would be right. But unfortunately on a Linux system, some processes running in it reset security labels sometimes, possibly cutting of a running domain. For instance udev. There has been a problem (race you can say), where libvirt set seclabels on a disk device, and wanted to start a domain but meanwhile udev came and restored the seclabels. With this module we can have a small helper that could be used by udev to find out what seclabels should a device have. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/Makefile.am | 1 + src/libvirt_private.syms | 4 +++ src/util/virudev.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virudev.h | 31 ++++++++++++++++++++++ 4 files changed, 104 insertions(+) create mode 100644 src/util/virudev.c create mode 100644 src/util/virudev.h diff --git a/src/Makefile.am b/src/Makefile.am index 8ee5567..2ea6f2b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -179,6 +179,7 @@ UTIL_SOURCES = \ util/virtime.h util/virtime.c \ util/virtpm.h util/virtpm.c \ util/virtypedparam.c util/virtypedparam.h \ + util/virudev.c util/virudev.h \ util/virusb.c util/virusb.h \ util/viruri.h util/viruri.c \ util/virutil.c util/virutil.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bec2628..5ae8037 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2573,6 +2573,10 @@ virTypedParamsSerialize; virTypedParamsValidate; +# util/virudev.h +virUdevMgrNew; + + # util/viruri.h virURIFormat; virURIFormatParams; diff --git a/src/util/virudev.c b/src/util/virudev.c new file mode 100644 index 0000000..66b5a58 --- /dev/null +++ b/src/util/virudev.c @@ -0,0 +1,68 @@ +/* + * virudev.c: udev rules engine + * + * Copyright (C) 2016 Red Hat, 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: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "virudev.h" +#include "virobject.h" + +struct _virUdevMgr { + virObjectLockable parent; +}; + +static virClassPtr virUdevMgrClass; + + +static void +virUdevMgrDispose(void *obj ATTRIBUTE_UNUSED) +{ + /* nada */ +} + + +static int virUdevMgrOnceInit(void) +{ + if (!(virUdevMgrClass = virClassNew(virClassForObjectLockable(), + "virUdevMgr", + sizeof(virUdevMgr), + virUdevMgrDispose))) + return -1; + + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virUdevMgr) + + +virUdevMgrPtr virUdevMgrNew(void) +{ + virUdevMgrPtr mgr; + + if (virUdevMgrInitialize() < 0) + return NULL; + + if (!(mgr = virObjectLockableNew(virUdevMgrClass))) + return NULL; + + return mgr; +} diff --git a/src/util/virudev.h b/src/util/virudev.h new file mode 100644 index 0000000..28e336f --- /dev/null +++ b/src/util/virudev.h @@ -0,0 +1,31 @@ +/* + * virudev.h: udev rules engine + * + * Copyright (C) 2016 Red Hat, 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: Michal Privoznik <mprivozn@redhat.com> + */ + +#ifndef __VIR_UDEV_H__ +# define __VIR_UDEV_H__ + +typedef struct _virUdevMgr virUdevMgr; +typedef virUdevMgr *virUdevMgrPtr; + +virUdevMgrPtr virUdevMgrNew(void); + +#endif -- 2.8.4
These APIs start to implement what was laid out in the module description. We need to be able to store given security label (on domain startup, device attach), and then remove all security labels associated with it (on domain shutdown, device detach). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 + src/util/virudev.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/virudev.h | 8 +++ 3 files changed, 149 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5ae8037..184317a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2574,7 +2574,9 @@ virTypedParamsValidate; # util/virudev.h +virUdevMgrAddLabel; virUdevMgrNew; +virUdevMgrRemoveAllLabels; # util/viruri.h diff --git a/src/util/virudev.c b/src/util/virudev.c index 66b5a58..f4799e7 100644 --- a/src/util/virudev.c +++ b/src/util/virudev.c @@ -22,20 +22,101 @@ #include <config.h> -#include "virudev.h" +#include "internal.h" +#include "viralloc.h" +#include "virhash.h" #include "virobject.h" +#include "virudev.h" + +#define VIR_FROM_THIS VIR_FROM_SECURITY struct _virUdevMgr { virObjectLockable parent; + virHashTablePtr labels; }; +struct _udevSeclabel { + virSecurityDeviceLabelDefPtr *seclabels; + size_t nseclabels; +}; + +typedef struct _udevSeclabel udevSeclabel; +typedef udevSeclabel *udevSeclabelPtr; + static virClassPtr virUdevMgrClass; +static virSecurityDeviceLabelDefPtr +udevSeclabelFindByModel(udevSeclabelPtr list, + const char *model) +{ + size_t i; + + for (i = 0; list && i < list->nseclabels; i++) { + if (STREQ(list->seclabels[i]->model, model)) + return list->seclabels[i]; + } + + return NULL; +} + + +static int +udevSeclabelAppend(udevSeclabelPtr list, + const virSecurityDeviceLabelDef *seclabel) +{ + virSecurityDeviceLabelDefPtr copy = virSecurityDeviceLabelDefCopy(seclabel); + if (VIR_APPEND_ELEMENT_COPY(list->seclabels, list->nseclabels, copy) < 0) { + virSecurityDeviceLabelDefFree(copy); + return -1; + } + return 0; +} + + +static void +udevSeclabelFree(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + udevSeclabelPtr list = payload; + size_t i; + + if (!list) + return; + + for (i = 0; i < list->nseclabels; i++) + virSecurityDeviceLabelDefFree(list->seclabels[i]); + VIR_FREE(list->seclabels); + VIR_FREE(list); +} + + +static int +udevSeclabelUpdate(udevSeclabelPtr list, + const virSecurityDeviceLabelDef *seclabel) +{ + size_t i; + + for (i = 0; list && i < list->nseclabels; i++) { + if (STREQ(list->seclabels[i]->model, seclabel->model)) { + virSecurityDeviceLabelDefPtr copy = virSecurityDeviceLabelDefCopy(seclabel); + if (!copy) + return -1; + virSecurityDeviceLabelDefFree(list->seclabels[i]); + list->seclabels[i] = copy; + return 0; + } + } + + return -1; +} + + static void -virUdevMgrDispose(void *obj ATTRIBUTE_UNUSED) +virUdevMgrDispose(void *obj) { - /* nada */ + virUdevMgrPtr mgr = obj; + virHashFree(mgr->labels); } @@ -64,5 +145,60 @@ virUdevMgrPtr virUdevMgrNew(void) if (!(mgr = virObjectLockableNew(virUdevMgrClass))) return NULL; + if (!(mgr->labels = virHashCreate(10, udevSeclabelFree))) + goto error; + return mgr; + + error: + virObjectUnref(mgr); + return NULL; +} + + +int +virUdevMgrAddLabel(virUdevMgrPtr mgr, + const char *device, + const virSecurityDeviceLabelDef *seclabel) +{ + int ret = -1; + udevSeclabelPtr list; + + virObjectLock(mgr); + + if ((list = virHashLookup(mgr->labels, device))) { + virSecurityDeviceLabelDefPtr entry = udevSeclabelFindByModel(list, seclabel->model); + + if (entry) { + udevSeclabelUpdate(list, seclabel); + } else { + if (udevSeclabelAppend(list, seclabel) < 0) + goto cleanup; + } + } else { + if (VIR_ALLOC(list) < 0 || + udevSeclabelAppend(list, seclabel) < 0 || + virHashAddEntry(mgr->labels, device, list) < 0) { + udevSeclabelFree(list, NULL); + goto cleanup; + } + } + + ret = 0; + cleanup: + virObjectUnlock(mgr); + return ret; +} + + +int +virUdevMgrRemoveAllLabels(virUdevMgrPtr mgr, + const char *device) +{ + int ret = -1; + + virObjectLock(mgr); + ret = virHashRemoveEntry(mgr->labels, device); + virObjectUnlock(mgr); + return ret; } diff --git a/src/util/virudev.h b/src/util/virudev.h index 28e336f..692b39b 100644 --- a/src/util/virudev.h +++ b/src/util/virudev.h @@ -23,9 +23,17 @@ #ifndef __VIR_UDEV_H__ # define __VIR_UDEV_H__ +# include "virseclabel.h" + typedef struct _virUdevMgr virUdevMgr; typedef virUdevMgr *virUdevMgrPtr; virUdevMgrPtr virUdevMgrNew(void); +int virUdevMgrAddLabel(virUdevMgrPtr mgr, + const char *device, + const virSecurityDeviceLabelDef *seclabel); +int virUdevMgrRemoveAllLabels(virUdevMgrPtr mgr, + const char *device); + #endif -- 2.8.4
Now that we are able to store security labels for devices, next step is to flush them into a file. For more convenience I've chosen JSON format (as we have all the APIs needed for processing the format). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- po/POTFILES.in | 1 + src/libvirt_private.syms | 2 + src/util/virudev.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virudev.h | 5 ++ 4 files changed, 165 insertions(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index 1469240..dabc612 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -248,6 +248,7 @@ src/util/virthreadpool.c src/util/virtime.c src/util/virtpm.c src/util/virtypedparam.c +src/util/virudev.c src/util/viruri.c src/util/virusb.c src/util/virutil.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 184317a..b465a0d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2575,6 +2575,8 @@ virTypedParamsValidate; # util/virudev.h virUdevMgrAddLabel; +virUdevMgrDumpFile; +virUdevMgrDumpStr; virUdevMgrNew; virUdevMgrRemoveAllLabels; diff --git a/src/util/virudev.c b/src/util/virudev.c index f4799e7..7f52149 100644 --- a/src/util/virudev.c +++ b/src/util/virudev.c @@ -24,7 +24,9 @@ #include "internal.h" #include "viralloc.h" +#include "virfile.h" #include "virhash.h" +#include "virjson.h" #include "virobject.h" #include "virudev.h" @@ -112,6 +114,68 @@ udevSeclabelUpdate(udevSeclabelPtr list, } +static virJSONValuePtr +udevSeclabelDump(const virSecurityDeviceLabelDef *seclabel) +{ + virJSONValuePtr object; + + if (!(object = virJSONValueNewObject()) || + virJSONValueObjectAppendString(object, "model", seclabel->model) < 0 || + virJSONValueObjectAppendString(object, "label", seclabel->label) < 0) + goto error; + + return object; + + error: + virJSONValueFree(object); + return NULL; +} + + +static int +udevSeclabelsDump(void *payload, + const void *name, + void *opaque) +{ + udevSeclabelPtr list = payload; + const char *device = name; + virJSONValuePtr seclabels = opaque; + virJSONValuePtr deviceLabels = NULL; + virJSONValuePtr deviceJSON = NULL; + size_t i; + int ret = -1; + + if (!(deviceLabels = virJSONValueNewArray())) + return ret; + + for (i = 0; i < list->nseclabels; i++) { + virJSONValuePtr seclabel = udevSeclabelDump(list->seclabels[i]); + + if (!seclabel || + virJSONValueArrayAppend(deviceLabels, seclabel) < 0) { + virJSONValueFree(seclabel); + goto cleanup; + } + } + + if (!(deviceJSON = virJSONValueNewObject()) || + virJSONValueObjectAppendString(deviceJSON, "device", device) < 0 || + virJSONValueObjectAppend(deviceJSON, "labels", deviceLabels) < 0) + goto cleanup; + deviceLabels = NULL; + + if (virJSONValueArrayAppend(seclabels, deviceJSON) < 0) + goto cleanup; + deviceJSON = NULL; + + ret = 0; + cleanup: + virJSONValueFree(deviceJSON); + virJSONValueFree(deviceLabels); + return ret; +} + + static void virUdevMgrDispose(void *obj) { @@ -202,3 +266,96 @@ virUdevMgrRemoveAllLabels(virUdevMgrPtr mgr, virObjectUnlock(mgr); return ret; } + + +static virJSONValuePtr +virUdevSeclabelDump(virUdevMgrPtr mgr) +{ + virJSONValuePtr seclabels; + + if (!(seclabels = virJSONValueNewArray())) + return NULL; + + if (virHashForEach(mgr->labels, udevSeclabelsDump, seclabels) < 0) { + virJSONValueFree(seclabels); + return NULL; + } + + return seclabels; +} + + +static char * +virUdevMgrDumpInternal(virUdevMgrPtr mgr) +{ + virJSONValuePtr object = NULL; + virJSONValuePtr child = NULL; + char *ret = NULL; + + if (!(object = virJSONValueNewObject())) + goto cleanup; + + if (!(child = virUdevSeclabelDump(mgr))) + goto cleanup; + + if (virJSONValueObjectAppend(object, "labels", child) < 0) { + virJSONValueFree(child); + goto cleanup; + } + + ret = virJSONValueToString(object, true); + cleanup: + virJSONValueFree(object); + return ret; +} + + +char * +virUdevMgrDumpStr(virUdevMgrPtr mgr) +{ + char *ret; + + virObjectLock(mgr); + ret = virUdevMgrDumpInternal(mgr); + virObjectUnlock(mgr); + return ret; +} + + +static int +virUdevMgrRewriter(int fd, void *opaque) +{ + const char *str = opaque; + + return safewrite(fd, str, strlen(str)); +} + + +int +virUdevMgrDumpFile(virUdevMgrPtr mgr, + const char *filename) +{ + int ret = -1; + char *state; + + virObjectLock(mgr); + + if (!(state = virUdevMgrDumpInternal(mgr))) + goto cleanup; + + /* Here we shouldn't use pure virFileWriteStr() as that one is not atomic. + * We can be interrupted in the middle (e.g. due to a context switch) and + * thus leave the file partially written. */ + if (virFileRewrite(filename, 0644, virUdevMgrRewriter, state) < 0) { + virReportSystemError(errno, + _("Unable to save state file %s"), + filename); + goto cleanup; + } + + ret = 0; + cleanup: + virObjectUnlock(mgr); + VIR_FREE(state); + return ret; +} diff --git a/src/util/virudev.h b/src/util/virudev.h index 692b39b..1d5a23e 100644 --- a/src/util/virudev.h +++ b/src/util/virudev.h @@ -36,4 +36,9 @@ int virUdevMgrAddLabel(virUdevMgrPtr mgr, int virUdevMgrRemoveAllLabels(virUdevMgrPtr mgr, const char *device); +char *virUdevMgrDumpStr(virUdevMgrPtr mgr); + +int virUdevMgrDumpFile(virUdevMgrPtr mgr, + const char *filename); + #endif -- 2.8.4
On Wed, Oct 26, 2016 at 02:36:54PM +0200, Michal Privoznik wrote:
Now that we are able to store security labels for devices, next step is to flush them into a file. For more convenience I've chosen JSON format (as we have all the APIs needed for processing the format).
I wonder if we're better off avoiding using a structured file format entirely. IIUC, we essentially just need a boolean indicator for the udev helper against each device. Could we do this with a zero length file per device. eg consider a device '/dev/sda1' That has a sha256 checksum d8d8d2c47b672edb91407c0014286b472673b2ff9d6636fd567d11650986ba3c so we could just touch a zero length file /var/lib/libvirt/udev/$CHECKSUM that way the udev helper doesn't need to parse anything - it merely needs to see if the file exists or not. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Now that we have the virudev module somewhat working, lets introduce some testing of it. We also need mock for virRandomBits function. Without it, the order in which entries occur in the hash table would be random and thus test would fail some times (as we expect certain ordering). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 12 +++ tests/virudevmock.c | 29 +++++++ tests/virudevtest.c | 124 ++++++++++++++++++++++++++++++ tests/virudevtestdata/complex.json | 30 ++++++++ tests/virudevtestdata/empty.json | 5 ++ tests/virudevtestdata/simple-dac.json | 13 ++++ tests/virudevtestdata/simple-selinux.json | 13 ++++ 7 files changed, 226 insertions(+) create mode 100644 tests/virudevmock.c create mode 100644 tests/virudevtest.c create mode 100644 tests/virudevtestdata/complex.json create mode 100644 tests/virudevtestdata/empty.json create mode 100644 tests/virudevtestdata/simple-dac.json create mode 100644 tests/virudevtestdata/simple-selinux.json diff --git a/tests/Makefile.am b/tests/Makefile.am index 924029a..824796b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -150,6 +150,7 @@ EXTRA_DIST = \ virpcitestdata \ virscsidata \ virsh-uriprecedence \ + virudevtestdata \ virusbtestdata \ vmwareverdata \ vmx2xmldata \ @@ -193,6 +194,7 @@ test_programs = virshtest sockettest \ vircaps2xmltest \ virnetdevtest \ virtypedparamtest \ + virudevtest \ $(NULL) if WITH_REMOTE @@ -406,6 +408,7 @@ test_libraries = libshunload.la \ virhostcpumock.la \ nssmock.la \ domaincapsmock.la \ + virudevmock.la \ $(NULL) if WITH_QEMU test_libraries += libqemumonitortestutils.la \ @@ -1343,6 +1346,15 @@ virtypedparamtest_SOURCES = \ virtypedparamtest.c testutils.h testutils.c virtypedparamtest_LDADD = $(LDADDS) +virudevtest_SOURCES = \ + virudevtest.c testutils.h testutils.c +virudevtest_LDADD = $(LDADDS) + +virudevmock_la_SOURCES = virudevmock.c +virudevmock_la_CFLAGS = $(AM_CFLAGS) +virudevmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) +virudevmock_la_LIBADD = $(MOCKLIBS_LIBS) + if WITH_LINUX fchosttest_SOURCES = \ diff --git a/tests/virudevmock.c b/tests/virudevmock.c new file mode 100644 index 0000000..d01f1c9 --- /dev/null +++ b/tests/virudevmock.c @@ -0,0 +1,29 @@ +/* + * Copyright (C) 2016 Red Hat, 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: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "virrandom.h" + +uint64_t virRandomBits(int nbits ATTRIBUTE_UNUSED) +{ + return 4; /* chosen by fair dice roll. + guaranteed to be random. */ +} diff --git a/tests/virudevtest.c b/tests/virudevtest.c new file mode 100644 index 0000000..883d751 --- /dev/null +++ b/tests/virudevtest.c @@ -0,0 +1,124 @@ +/* + * Copyright (C) 2016 Red Hat, 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: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "testutils.h" +#include "virudev.h" +#include "virjson.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct testUdevData { + const char *file; + const char *const *labels; +}; + + +static int +testDump(const void *opaque) +{ + const struct testUdevData *data = opaque; + virUdevMgrPtr mgr = NULL; + int ret = -1; + const char * const *tmp; + char *state = NULL; + char *filename = NULL; + + if (virAsprintf(&filename, "%s/virudevtestdata/%s.json", + abs_srcdir, data->file) < 0) + goto cleanup; + + if (!(mgr = virUdevMgrNew())) + goto cleanup; + + tmp = data->labels; + while (*tmp) { + const char *device; + const char *model; + const char *label; + virSecurityDeviceLabelDefPtr seclabel; + + device = *tmp; + if (!++tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Invalid seclabels list"); + goto cleanup; + } + model = *tmp; + if (!++tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Invalid seclabels list"); + goto cleanup; + } + label = *tmp; + tmp++; + + if (!(seclabel = virSecurityDeviceLabelDefNewLabel(model, label))) + goto cleanup; + + if (virUdevMgrAddLabel(mgr, device, seclabel) < 0) + goto cleanup; + virSecurityDeviceLabelDefFree(seclabel); + } + + if (!(state = virUdevMgrDumpStr(mgr))) + goto cleanup; + + if (virTestCompareToFile(state, filename) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(filename); + VIR_FREE(state); + virObjectUnref(mgr); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST_DUMP(filename, ...) \ + do { \ + const char *labels[] = {__VA_ARGS__, NULL}; \ + struct testUdevData data = { \ + .file = filename, .labels = labels, \ + }; \ + if (virTestRun("Dump " filename, testDump, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_DUMP("empty", NULL); + DO_TEST_DUMP("simple-selinux", + "/dev/sda", "selinux", "someSELinuxLabel"); + DO_TEST_DUMP("simple-dac", + "/dev/sda", "dac", "someDACLabel"); + DO_TEST_DUMP("complex", + "/dev/sda", "dac", "someDACLabel", + "/dev/sda", "selinux", "someSELinuxLabel", + "/dev/sdb", "dac", "otherDACLabel", + "/dev/sdb", "selinux", "otherSELinuxLabel"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virudevmock.so") diff --git a/tests/virudevtestdata/complex.json b/tests/virudevtestdata/complex.json new file mode 100644 index 0000000..e1c8e69 --- /dev/null +++ b/tests/virudevtestdata/complex.json @@ -0,0 +1,30 @@ +{ + "labels": [ + { + "device": "/dev/sda", + "labels": [ + { + "model": "dac", + "label": "someDACLabel" + }, + { + "model": "selinux", + "label": "someSELinuxLabel" + } + ] + }, + { + "device": "/dev/sdb", + "labels": [ + { + "model": "dac", + "label": "otherDACLabel" + }, + { + "model": "selinux", + "label": "otherSELinuxLabel" + } + ] + } + ] +} diff --git a/tests/virudevtestdata/empty.json b/tests/virudevtestdata/empty.json new file mode 100644 index 0000000..763c0f2 --- /dev/null +++ b/tests/virudevtestdata/empty.json @@ -0,0 +1,5 @@ +{ + "labels": [ + + ] +} diff --git a/tests/virudevtestdata/simple-dac.json b/tests/virudevtestdata/simple-dac.json new file mode 100644 index 0000000..bbda6bb --- /dev/null +++ b/tests/virudevtestdata/simple-dac.json @@ -0,0 +1,13 @@ +{ + "labels": [ + { + "device": "/dev/sda", + "labels": [ + { + "model": "dac", + "label": "someDACLabel" + } + ] + } + ] +} diff --git a/tests/virudevtestdata/simple-selinux.json b/tests/virudevtestdata/simple-selinux.json new file mode 100644 index 0000000..7398d7f --- /dev/null +++ b/tests/virudevtestdata/simple-selinux.json @@ -0,0 +1,13 @@ +{ + "labels": [ + { + "device": "/dev/sda", + "labels": [ + { + "model": "selinux", + "label": "someSELinuxLabel" + } + ] + } + ] +} -- 2.8.4
Now that we are able to dump internal state into a JSON file/string, we should be able to reverse the process and reconstruct the internal state from a JSON file/string. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 + src/util/virudev.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virudev.h | 2 + tests/virudevtest.c | 45 ++++++++++++++ 4 files changed, 208 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b465a0d..3369600 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2578,6 +2578,8 @@ virUdevMgrAddLabel; virUdevMgrDumpFile; virUdevMgrDumpStr; virUdevMgrNew; +virUdevMgrNewFromFile; +virUdevMgrNewFromStr; virUdevMgrRemoveAllLabels; diff --git a/src/util/virudev.c b/src/util/virudev.c index 7f52149..d60fbf9 100644 --- a/src/util/virudev.c +++ b/src/util/virudev.c @@ -28,6 +28,7 @@ #include "virhash.h" #include "virjson.h" #include "virobject.h" +#include "virstring.h" #include "virudev.h" #define VIR_FROM_THIS VIR_FROM_SECURITY @@ -176,6 +177,49 @@ udevSeclabelsDump(void *payload, } +static udevSeclabelPtr +udevSeclabelRestore(virJSONValuePtr labels) +{ + udevSeclabelPtr ret = NULL; + size_t i; + + if (VIR_ALLOC(ret) < 0) + goto error; + + for (i = 0; i < virJSONValueArraySize(labels); i++) { + virJSONValuePtr labelJSON = virJSONValueArrayGet(labels, i); + virSecurityDeviceLabelDefPtr seclabel; + const char *model; + const char *label; + + if (!(model = virJSONValueObjectGetString(labelJSON, "model"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("seclabel missing model in JSON")); + goto error; + } + + if (!(label = virJSONValueObjectGetString(labelJSON, "label"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("seclabel missing label in JSON")); + goto error; + } + + if (!(seclabel = virSecurityDeviceLabelDefNewLabel(model, label)) || + udevSeclabelAppend(ret, seclabel) < 0) { + virSecurityDeviceLabelDefFree(seclabel); + goto error; + } + virSecurityDeviceLabelDefFree(seclabel); + } + + return ret; + + error: + udevSeclabelFree(ret, NULL); + return NULL; +} + + static void virUdevMgrDispose(void *obj) { @@ -359,3 +403,118 @@ virUdevMgrDumpFile(virUdevMgrPtr mgr, VIR_FREE(state); return ret; } + + +static int +virUdevRestoreLabels(virUdevMgrPtr mgr ATTRIBUTE_UNUSED, + virJSONValuePtr labelsArray) +{ + int ret = -1; + size_t i; + udevSeclabelPtr list = NULL; + + for (i = 0; i < virJSONValueArraySize(labelsArray); i++) { + virJSONValuePtr deviceJSON = virJSONValueArrayGet(labelsArray, i); + virJSONValuePtr labels; + const char *device; + + if (!(device = virJSONValueObjectGetString(deviceJSON, "device"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing device name")); + goto cleanup; + } + + if (!(labels = virJSONValueObjectGetArray(deviceJSON, "labels"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing device labels array")); + goto cleanup; + } + + if (!(list = udevSeclabelRestore(labels))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed seclabels for device %s"), device); + goto cleanup; + } + + if (virHashAddEntry(mgr->labels, device, list) < 0) + goto cleanup; + list = NULL; + } + + ret = 0; + cleanup: + udevSeclabelFree(list, NULL); + return ret; +} + + +static int +virUdevMgrNewFromStrInternal(virUdevMgrPtr mgr, + const char *state) +{ + virJSONValuePtr object; + virJSONValuePtr child; + int ret = -1; + + if (!(object = virJSONValueFromString(state))) + goto cleanup; + + if (!(child = virJSONValueObjectGetArray(object, "labels"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing 'labels' object in JSON document")); + goto cleanup; + } + + if (virUdevRestoreLabels(mgr, child) < 0) + goto cleanup; + + ret = 0; + cleanup: + virJSONValueFree(object); + return ret; +} + + +virUdevMgrPtr +virUdevMgrNewFromStr(const char *str) +{ + virUdevMgrPtr mgr; + + if (!(mgr = virUdevMgrNew())) + goto error; + + if (virUdevMgrNewFromStrInternal(mgr, str) < 0) + goto error; + + return mgr; + error: + virObjectUnref(mgr); + return NULL; +} + + +virUdevMgrPtr +virUdevMgrNewFromFile(const char *filename) +{ + virUdevMgrPtr mgr; + char *state = NULL; + + if (!(mgr = virUdevMgrNew())) + goto error; + + if (virFileReadAll(filename, + 1024 * 1024 * 10, /* 10 MB */ + &state) < 0) + goto error; + + if (virUdevMgrNewFromStrInternal(mgr, state) < 0) + goto error; + + VIR_FREE(state); + + return mgr; + error: + virObjectUnref(mgr); + VIR_FREE(state); + return NULL; +} diff --git a/src/util/virudev.h b/src/util/virudev.h index 1d5a23e..82b2b4f 100644 --- a/src/util/virudev.h +++ b/src/util/virudev.h @@ -29,6 +29,8 @@ typedef struct _virUdevMgr virUdevMgr; typedef virUdevMgr *virUdevMgrPtr; virUdevMgrPtr virUdevMgrNew(void); +virUdevMgrPtr virUdevMgrNewFromStr(const char *str); +virUdevMgrPtr virUdevMgrNewFromFile(const char *filename); int virUdevMgrAddLabel(virUdevMgrPtr mgr, const char *device, diff --git a/tests/virudevtest.c b/tests/virudevtest.c index 883d751..c395741 100644 --- a/tests/virudevtest.c +++ b/tests/virudevtest.c @@ -93,6 +93,37 @@ testDump(const void *opaque) static int +testParse(const void *opaque) +{ + const struct testUdevData *data = opaque; + virUdevMgrPtr mgr = NULL; + char *filename = NULL; + char *state = NULL; + int ret = -1; + + if (virAsprintf(&filename, "%s/virudevtestdata/%s.json", + abs_srcdir, data->file) < 0) + goto cleanup; + + if (!(mgr = virUdevMgrNewFromFile(filename))) + goto cleanup; + + if (!(state = virUdevMgrDumpStr(mgr))) + goto cleanup; + + if (virTestCompareToFile(state, filename)) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(state); + VIR_FREE(filename); + virObjectUnref(mgr); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -107,6 +138,15 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_PARSE(filename) \ + do { \ + struct testUdevData data = { \ + .file = filename, \ + }; \ + if (virTestRun("Parse " filename, testParse, &data) < 0) \ + ret = -1; \ + } while (0) + DO_TEST_DUMP("empty", NULL); DO_TEST_DUMP("simple-selinux", "/dev/sda", "selinux", "someSELinuxLabel"); @@ -118,6 +158,11 @@ mymain(void) "/dev/sdb", "dac", "otherDACLabel", "/dev/sdb", "selinux", "otherSELinuxLabel"); + DO_TEST_PARSE("empty"); + DO_TEST_PARSE("simple-selinux"); + DO_TEST_PARSE("simple-dac"); + DO_TEST_PARSE("complex"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.8.4
This is the cherry on the top. For given device all its security labels are fetched. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virudev.c | 43 +++++++++++++++++++++++ src/util/virudev.h | 4 +++ tests/virudevtest.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 139 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3369600..a0de4e9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2577,6 +2577,7 @@ virTypedParamsValidate; virUdevMgrAddLabel; virUdevMgrDumpFile; virUdevMgrDumpStr; +virUdevMgrLookupLabels; virUdevMgrNew; virUdevMgrNewFromFile; virUdevMgrNewFromStr; diff --git a/src/util/virudev.c b/src/util/virudev.c index d60fbf9..9940f5f 100644 --- a/src/util/virudev.c +++ b/src/util/virudev.c @@ -300,6 +300,49 @@ virUdevMgrAddLabel(virUdevMgrPtr mgr, int +virUdevMgrLookupLabels(virUdevMgrPtr mgr, + const char *device, + virSecurityDeviceLabelDefPtr **seclabels, + size_t *nseclabels) +{ + int ret = -1; + udevSeclabelPtr list; + size_t i; + + virObjectLock(mgr); + + if (!(list = virHashLookup(mgr->labels, device))) { + *seclabels = NULL; + *nseclabels = 0; + ret = 0; + goto cleanup; + } + + if (VIR_ALLOC_N(*seclabels, list->nseclabels) < 0) + goto cleanup; + + *nseclabels = list->nseclabels; + + for (i = 0; i < list->nseclabels; i++) { + if (!((*seclabels)[i] = virSecurityDeviceLabelDefCopy(list->seclabels[i]))) + goto cleanup; + } + + ret = 0; + cleanup: + virObjectUnlock(mgr); + if (ret < 0) { + if (*seclabels) { + for (i = 0; i < *nseclabels; i++) + virSecurityDeviceLabelDefFree((*seclabels)[i]); + VIR_FREE(*seclabels); + } + } + return ret; +} + + +int virUdevMgrRemoveAllLabels(virUdevMgrPtr mgr, const char *device) { diff --git a/src/util/virudev.h b/src/util/virudev.h index 82b2b4f..4e286bb 100644 --- a/src/util/virudev.h +++ b/src/util/virudev.h @@ -37,6 +37,10 @@ int virUdevMgrAddLabel(virUdevMgrPtr mgr, const virSecurityDeviceLabelDef *seclabel); int virUdevMgrRemoveAllLabels(virUdevMgrPtr mgr, const char *device); +int virUdevMgrLookupLabels(virUdevMgrPtr mgr, + const char *device, + virSecurityDeviceLabelDefPtr **seclabels, + size_t *nseclabels); char *virUdevMgrDumpStr(virUdevMgrPtr mgr); diff --git a/tests/virudevtest.c b/tests/virudevtest.c index c395741..36a9077 100644 --- a/tests/virudevtest.c +++ b/tests/virudevtest.c @@ -124,6 +124,76 @@ testParse(const void *opaque) static int +testLookup(const void *opaque) +{ + const struct testUdevData *data = opaque; + virUdevMgrPtr mgr = NULL; + int ret = -1; + const char * const *tmp; + char *filename = NULL; + virSecurityDeviceLabelDefPtr *seclabels = NULL; + size_t i, nseclabels = 0; + + if (virAsprintf(&filename, "%s/virudevtestdata/%s.json", + abs_srcdir, data->file) < 0) + goto cleanup; + + if (!(mgr = virUdevMgrNewFromFile(filename))) + goto cleanup; + + tmp = data->labels; + while (*tmp) { + const char *device; + const char *model; + const char *label; + + device = *tmp; + if (!++tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Invalid seclabels list"); + goto cleanup; + } + model = *tmp; + if (!++tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Invalid seclabels list"); + goto cleanup; + } + label = *tmp; + tmp++; + + if (virUdevMgrLookupLabels(mgr, device, &seclabels, &nseclabels) < 0) + goto cleanup; + + for (i = 0; i < nseclabels; i++) { + virSecurityDeviceLabelDefPtr seclabel = seclabels[i]; + + if (STREQ(seclabel->model, model) && + STREQ(seclabel->label, label)) + break; + } + + if (i == nseclabels) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Label not found"); + goto cleanup; + } + + for (i = 0; i < nseclabels; i++) + virSecurityDeviceLabelDefFree(seclabels[i]); + VIR_FREE(seclabels); + nseclabels = 0; + } + + ret = 0; + cleanup: + for (i = 0; i < nseclabels; i++) + virSecurityDeviceLabelDefFree(seclabels[i]); + VIR_FREE(seclabels); + VIR_FREE(filename); + virObjectUnref(mgr); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -147,6 +217,16 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_LOOKUP(filename, ...) \ + do { \ + const char *labels[] = {__VA_ARGS__, NULL}; \ + struct testUdevData data = { \ + .file = filename, .labels = labels, \ + }; \ + if (virTestRun("Lookup " filename, testLookup, &data) < 0) \ + ret = -1; \ + } while (0) + DO_TEST_DUMP("empty", NULL); DO_TEST_DUMP("simple-selinux", "/dev/sda", "selinux", "someSELinuxLabel"); @@ -163,6 +243,17 @@ mymain(void) DO_TEST_PARSE("simple-dac"); DO_TEST_PARSE("complex"); + DO_TEST_LOOKUP("empty", NULL); + DO_TEST_LOOKUP("simple-selinux", + "/dev/sda", "selinux", "someSELinuxLabel"); + DO_TEST_LOOKUP("simple-dac", + "/dev/sda", "dac", "someDACLabel"); + DO_TEST_LOOKUP("complex", + "/dev/sda", "dac", "someDACLabel", + "/dev/sda", "selinux", "someSELinuxLabel", + "/dev/sdb", "dac", "otherDACLabel", + "/dev/sdb", "selinux", "otherSELinuxLabel"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.8.4
This is a small helper intended to be run by udev. On its input (either as the only command line argument or in DEVNODE environment vairable) it is given a device and on the output it will either put nothing (meaning the device is not used by any of the libvirt domains), or it will print out security labels in the following form: UID GID SELABEL Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- libvirt.spec.in | 1 + mingw-libvirt.spec.in | 2 + po/POTFILES.in | 1 + src/Makefile.am | 20 ++++++++ src/util/udevhelper.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+) create mode 100644 src/util/udevhelper.c diff --git a/libvirt.spec.in b/libvirt.spec.in index 545990c..856c702 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1630,6 +1630,7 @@ exit 0 %dir %attr(0700, root, root) %{_localstatedir}/log/libvirt/ %attr(0755, root, root) %{_libexecdir}/libvirt_iohelper +%attr(0755, root, root) %{_libexecdir}/libvirt_udevhelper %attr(0755, root, root) %{_sbindir}/libvirtd %attr(0755, root, root) %{_sbindir}/virtlogd diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in index c9bf503..015b06b 100644 --- a/mingw-libvirt.spec.in +++ b/mingw-libvirt.spec.in @@ -178,6 +178,8 @@ rm -rf $RPM_BUILD_ROOT%{mingw64_datadir}/gtk-doc/* rm -rf $RPM_BUILD_ROOT%{mingw32_libexecdir}/libvirt_iohelper.exe rm -rf $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt_iohelper.exe +rm -rf $RPM_BUILD_ROOT%{mingw32_libexecdir}/libvirt_udevhelper.exe +rm -rf $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt_udevhelper.exe rm -rf $RPM_BUILD_ROOT%{mingw32_libexecdir}/libvirt-guests.sh rm -rf $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh diff --git a/po/POTFILES.in b/po/POTFILES.in index dabc612..7a89cbd 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -178,6 +178,7 @@ src/test/test_driver.c src/uml/uml_conf.c src/uml/uml_driver.c src/util/iohelper.c +src/util/udevhelper.c src/util/viralloc.c src/util/viraudit.c src/util/virauth.c diff --git a/src/Makefile.am b/src/Makefile.am index 2ea6f2b..0c97728 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1007,6 +1007,9 @@ STORAGE_HELPER_DISK_SOURCES = \ UTIL_IO_HELPER_SOURCES = \ util/iohelper.c +UTIL_UDEV_HELPER_SOURCES = \ + util/udevhelper.c + NETWORK_LEASES_HELPER_SOURCES = \ network/leaseshelper.c @@ -2858,6 +2861,23 @@ libvirt_iohelper_CFLAGS = \ $(PIE_CFLAGS) \ $(NULL) +libexec_PROGRAMS += libvirt_udevhelper +libvirt_udevhelper_SOURCES = $(UTIL_UDEV_HELPER_SOURCES) +libvirt_udevhelper_CFLAGS = \ + $(AM_CFLAGS) \ + $(PIE_CFLAGS) \ + $(NULL) +libvirt_udevhelper_LDFLAGS = \ + $(AM_LDFLAGS) \ + $(PIE_LDFLAGS) \ + $(NULL) +libvirt_udevhelper_LDADD = \ + libvirt_util.la \ + ../gnulib/lib/libgnu.la +if WITH_DTRACE_PROBES +libvirt_udevhelper_LDADD += libvirt_probes.lo +endif WITH_DTRACE_PROBES + if WITH_NETWORK libexec_PROGRAMS += libvirt_leaseshelper libvirt_leaseshelper_SOURCES = $(NETWORK_LEASES_HELPER_SOURCES) diff --git a/src/util/udevhelper.c b/src/util/udevhelper.c new file mode 100644 index 0000000..e44e6f6 --- /dev/null +++ b/src/util/udevhelper.c @@ -0,0 +1,137 @@ +/* + * Copyright (C) 2016 Red Hat, 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: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> + +#include "configmake.h" +#include "viralloc.h" +#include "virgettext.h" +#include "virobject.h" +#include "virstring.h" +#include "virthread.h" +#include "virudev.h" +#include "virutil.h" + +#define VIR_FROM_THIS VIR_FROM_SECURITY + + +static void +usage(const char *progname) +{ + fprintf(stderr, + _("Usage: %s [device]\n" + "\n" + "This program is intended to be run from udev to\n" + "maintain proper security labels on devices used by\n" + "domains managed by libvirt.\n" + "\n" + "For given device (either passed as the only\n" + "command line argument or set by DEVNODE environment\n" + "variable) all the security labels are printed onto\n" + "standard output.\n"), progname); +} + + +static int +printLabels(const char *device) +{ + char *filename = NULL; + virUdevMgrPtr mgr = NULL; + int ret = -1; + virSecurityDeviceLabelDefPtr *labels = NULL; + size_t i, nlabels = 0; + const char *dacLabel = NULL; + const char *seLabel = NULL; + + if (virAsprintf(&filename, + "%s/run/libvirt/qemu/devices.udev", LOCALSTATEDIR) < 0) + goto cleanup; + + if (!(mgr = virUdevMgrNewFromFile(filename))) + goto cleanup; + + if (virUdevMgrLookupLabels(mgr, device, &labels, &nlabels) < 0) + goto cleanup; + + if (!labels) { + /* Device not found in our DB. Claim success. */ + ret = 0; + goto cleanup; + } + + for (i = 0; i < nlabels; i++) { + virSecurityDeviceLabelDefPtr tmp = labels[i]; + + if (STREQ(tmp->model, "dac")) + dacLabel = tmp->label; + else if (STREQ(tmp->model, "selinux")) + seLabel = tmp->label; + } + + if (dacLabel) + printf("%s", dacLabel); + if (seLabel) + printf(" %s", seLabel); + printf("\n"); + + ret = 0; + cleanup: + for (i = 0; i < nlabels; i++) + virSecurityDeviceLabelDefFree(labels[i]); + VIR_FREE(labels); + virObjectUnref(mgr); + return ret; +} + + +int +main(int argc, char *argv[]) +{ + const char *device = NULL; + int ret = EXIT_FAILURE; + + if (virGettextInitialize() < 0 || + virThreadInitialize() < 0 || + virErrorInitialize() < 0) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (argc > 1) + device = argv[1]; + if (!device) + device = virGetEnvBlockSUID("DEVNODE"); + if (!device || STREQ(device, "-h") || STREQ(device, "--help")) { + usage(argv[0]); + if (device) + ret = EXIT_SUCCESS; + goto cleanup; + } + + if (printLabels(device) < 0) + goto cleanup; + + ret = EXIT_SUCCESS; + cleanup: + return ret; +} -- 2.8.4
On Wed, Oct 26, 2016 at 02:36:58PM +0200, Michal Privoznik wrote:
This is a small helper intended to be run by udev. On its input (either as the only command line argument or in DEVNODE environment vairable) it is given a device and on the output it will either put nothing (meaning the device is not used by any of the libvirt domains), or it will print out security labels in the following form:
UID GID SELABEL
How is this intended to be actually used ? ie what udev rule are you creating along with this ? IMHO we just want the helper to indicate that udev should not do anything to the device - we should not need udev to ever set labels itself as libvirt has already set them - we just don't want udev to remove them. IOW, I don't see the need to print out this info at all. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Wed, Oct 26, 2016 at 17:39:35 +0200, Daniel P. Berrange wrote:
On Wed, Oct 26, 2016 at 02:36:58PM +0200, Michal Privoznik wrote:
This is a small helper intended to be run by udev. On its input (either as the only command line argument or in DEVNODE environment vairable) it is given a device and on the output it will either put nothing (meaning the device is not used by any of the libvirt domains), or it will print out security labels in the following form:
UID GID SELABEL
How is this intended to be actually used ? ie what udev rule are you creating along with this ?
Yeah, the rule should really be part of this series.
IMHO we just want the helper to indicate that udev should not do anything to the device - we should not need udev to ever set labels itself as libvirt has already set them - we just don't want udev to remove them. IOW, I don't see the need to print out this info at all.
That would be nice, but unfortunately there's no way to tell udev not to touch a specific device (I discussed this stuff with Michal Sekletar). Other udev rules might have already set UID/GID/SELABEL for the device and we can only change it to contain the required content; we can't reset them to "don't change any of these". And if you were thinking that our rule could be the first rule called on each device (rather than the last one), there's no way to tell udev to just skip all other rules and ignore the device. It will run through all rules and they were set their own UID/GID/SELABEL as they wish. Jirka
On Thu, Oct 27, 2016 at 08:37:02AM +0200, Jiri Denemark wrote:
On Wed, Oct 26, 2016 at 17:39:35 +0200, Daniel P. Berrange wrote:
On Wed, Oct 26, 2016 at 02:36:58PM +0200, Michal Privoznik wrote:
This is a small helper intended to be run by udev. On its input (either as the only command line argument or in DEVNODE environment vairable) it is given a device and on the output it will either put nothing (meaning the device is not used by any of the libvirt domains), or it will print out security labels in the following form:
UID GID SELABEL
How is this intended to be actually used ? ie what udev rule are you creating along with this ?
Yeah, the rule should really be part of this series.
IMHO we just want the helper to indicate that udev should not do anything to the device - we should not need udev to ever set labels itself as libvirt has already set them - we just don't want udev to remove them. IOW, I don't see the need to print out this info at all.
That would be nice, but unfortunately there's no way to tell udev not to touch a specific device (I discussed this stuff with Michal Sekletar). Other udev rules might have already set UID/GID/SELABEL for the device and we can only change it to contain the required content; we can't reset them to "don't change any of these".
I think we need to prevent those rules from running - any situation in which somes rules change permissions and our other rule needs to change them back is still very badly race prone. My expectation was that the standard udev provided rule which resets permissions on file close would be modified to have an extra condition in its match rule: ENV{EXTERNALLY_MANAGED}!="yes" libvirt would then drop in the rule runs libvirt_udevhelper and if that returns 1, then we set ENV{EXTERNALLY_MANAGED}. This provides a general solution that other (non-libvirt) apps can use to prevent the perms changing behind their back Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Thu, Oct 27, 2016 at 09:25:41AM +0200, Daniel P. Berrange wrote:
On Thu, Oct 27, 2016 at 08:37:02AM +0200, Jiri Denemark wrote:
On Wed, Oct 26, 2016 at 17:39:35 +0200, Daniel P. Berrange wrote:
On Wed, Oct 26, 2016 at 02:36:58PM +0200, Michal Privoznik wrote:
This is a small helper intended to be run by udev. On its input (either as the only command line argument or in DEVNODE environment vairable) it is given a device and on the output it will either put nothing (meaning the device is not used by any of the libvirt domains), or it will print out security labels in the following form:
UID GID SELABEL
How is this intended to be actually used ? ie what udev rule are you creating along with this ?
Yeah, the rule should really be part of this series.
IMHO we just want the helper to indicate that udev should not do anything to the device - we should not need udev to ever set labels itself as libvirt has already set them - we just don't want udev to remove them. IOW, I don't see the need to print out this info at all.
That would be nice, but unfortunately there's no way to tell udev not to touch a specific device (I discussed this stuff with Michal Sekletar). Other udev rules might have already set UID/GID/SELABEL for the device and we can only change it to contain the required content; we can't reset them to "don't change any of these".
I think we need to prevent those rules from running - any situation in which somes rules change permissions and our other rule needs to change them back is still very badly race prone.
My expectation was that the standard udev provided rule which resets permissions on file close would be modified to have an extra condition in its match rule:
ENV{EXTERNALLY_MANAGED}!="yes"
libvirt would then drop in the rule runs libvirt_udevhelper and if that returns 1, then we set ENV{EXTERNALLY_MANAGED}. This provides a general solution that other (non-libvirt) apps can use to prevent the perms changing behind their back
IIUC, we can possibly achieve our goal using GOTO, with two rules. In a 00-libvirt-early.rules have a rule that runs libvirt_udevhelper and adds a "GOTO=libvirt-end". Then in zzzzzz-libvirt-late.rules define the LABEL=libvirt-end. That should cause it to skip over all intermediate udev rules. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Thu, Oct 27, 2016 at 3:08 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
IIUC, we can possibly achieve our goal using GOTO, with two rules.
In a 00-libvirt-early.rules have a rule that runs libvirt_udevhelper and adds a "GOTO=libvirt-end". Then in zzzzzz-libvirt-late.rules define the LABEL=libvirt-end. That should cause it to skip over all intermediate udev rules.
Jumps to non-local labels are not supported. Michal
On Thu, Oct 27, 2016 at 15:08:37 +0200, Daniel P. Berrange wrote:
On Thu, Oct 27, 2016 at 09:25:41AM +0200, Daniel P. Berrange wrote:
On Thu, Oct 27, 2016 at 08:37:02AM +0200, Jiri Denemark wrote:
On Wed, Oct 26, 2016 at 17:39:35 +0200, Daniel P. Berrange wrote:
On Wed, Oct 26, 2016 at 02:36:58PM +0200, Michal Privoznik wrote:
This is a small helper intended to be run by udev. On its input (either as the only command line argument or in DEVNODE environment vairable) it is given a device and on the output it will either put nothing (meaning the device is not used by any of the libvirt domains), or it will print out security labels in the following form:
UID GID SELABEL
How is this intended to be actually used ? ie what udev rule are you creating along with this ?
Yeah, the rule should really be part of this series.
IMHO we just want the helper to indicate that udev should not do anything to the device - we should not need udev to ever set labels itself as libvirt has already set them - we just don't want udev to remove them. IOW, I don't see the need to print out this info at all.
That would be nice, but unfortunately there's no way to tell udev not to touch a specific device (I discussed this stuff with Michal Sekletar). Other udev rules might have already set UID/GID/SELABEL for the device and we can only change it to contain the required content; we can't reset them to "don't change any of these".
I think we need to prevent those rules from running - any situation in which somes rules change permissions and our other rule needs to change them back is still very badly race prone.
My expectation was that the standard udev provided rule which resets permissions on file close would be modified to have an extra condition in its match rule:
ENV{EXTERNALLY_MANAGED}!="yes"
libvirt would then drop in the rule runs libvirt_udevhelper and if that returns 1, then we set ENV{EXTERNALLY_MANAGED}. This provides a general solution that other (non-libvirt) apps can use to prevent the perms changing behind their back
IIUC, we can possibly achieve our goal using GOTO, with two rules.
In a 00-libvirt-early.rules have a rule that runs libvirt_udevhelper and adds a "GOTO=libvirt-end". Then in zzzzzz-libvirt-late.rules define the LABEL=libvirt-end. That should cause it to skip over all intermediate udev rules.
Hmm, sounds like a nice hack around it. I didn't realize goto can jump to a label defined in a different rule file... Jirka
On Thu, Oct 27, 2016 at 8:37 AM, Jiri Denemark <jdenemar@redhat.com> wrote:
Yeah, the rule should really be part of this series.
I am working on udev hook-up. I found out that SECLABEL key doesn't support substitutions (see %c in man 7 udev). I need to fix that first and then we can have very simple udev rule file that employs helper. This is my WIP version, # /etc/udev/rules.d/99-virt.rules ACTION!="add|change", GOTO="virt_rules_end" SUBSYSTEM!="block", GOTO="virt_rules_end" PROGRAM="/usr/local/libexec/libvirt_udevhelper", OWNER="%c{1}", GROUP="%c{2}", SECLABEL{selinux}="%c{3}", TAG+="libvirt" LABEL="virt_rules_end" SECLABEL part doesn't work yet. Also helper uses DEVNODE env variable to obtain path to work with. It should use DEVNAME. This is my fault, because I told Michal to use DEVNODE. I've patched it locally for now. I will send PR to Michal with the fix. One more thing I want to work on is to close the possibility of a race when execution of a change event is in-flight and user starts VM. Then libvirtd writes to devices.udev file and launches qemu driver, but execution of prior change event may finish and hence reset permissions to whatever was there before libvirtd wrote to devices.udev database. That is why I am adding TAG to the device. What we could do, is to write devices.udev database and then trigger change event from libvirtd and locally wait for its completion (no need for event-loop integration), that would be indicated by TAG being present in the event environment. However, even w/o this, proposed patch series very much improves current state. Michal
On Thu, Oct 27, 2016 at 11:13:28AM +0200, Michal Sekletar wrote:
On Thu, Oct 27, 2016 at 8:37 AM, Jiri Denemark <jdenemar@redhat.com> wrote:
Yeah, the rule should really be part of this series.
I am working on udev hook-up. I found out that SECLABEL key doesn't support substitutions (see %c in man 7 udev). I need to fix that first and then we can have very simple udev rule file that employs helper. This is my WIP version,
# /etc/udev/rules.d/99-virt.rules ACTION!="add|change", GOTO="virt_rules_end" SUBSYSTEM!="block", GOTO="virt_rules_end"
PROGRAM="/usr/local/libexec/libvirt_udevhelper", OWNER="%c{1}", GROUP="%c{2}", SECLABEL{selinux}="%c{3}", TAG+="libvirt"
LABEL="virt_rules_end"
SECLABEL part doesn't work yet. Also helper uses DEVNODE env variable to obtain path to work with. It should use DEVNAME. This is my fault, because I told Michal to use DEVNODE. I've patched it locally for now. I will send PR to Michal with the fix.
One more thing I want to work on is to close the possibility of a race when execution of a change event is in-flight and user starts VM. Then libvirtd writes to devices.udev file and launches qemu driver, but execution of prior change event may finish and hence reset permissions to whatever was there before libvirtd wrote to devices.udev database. That is why I am adding TAG to the device. What we could do, is to write devices.udev database and then trigger change event from libvirtd and locally wait for its completion (no need for event-loop integration), that would be indicated by TAG being present in the event environment. However, even w/o this, proposed patch series very much improves current state.
We absolutely don't want to be delegating permissions setting/labelling to udev & waiting for it to complete asychronously in the background. That leads to two completely different approaches for labelling files vs block devices making debugging harder and the overall system more complex & error prone. We want to maintain libvirt setting all labelling synchronously itself. The only integration we want with udev is to prevent it undoing what libvirt has set. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Thu, Oct 27, 2016 at 2:48 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
We absolutely don't want to be delegating permissions setting/labelling to udev & waiting for it to complete asychronously in the background. That leads to two completely different approaches for labelling files vs block devices making debugging harder and the overall system more complex & error prone.
libvirtd would chown and relabel but it would also trigger change event after it populates devices.udev. To make sure any change event that was possibly running before won't change permissions back. Waiting will be synchronous on libvirtd side. I admit it is a bit ugly but I think there is no other way how to solve this problem. Michal
On Thu, Oct 27, 2016 at 03:37:59PM +0200, Michal Sekletar wrote:
On Thu, Oct 27, 2016 at 2:48 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
We absolutely don't want to be delegating permissions setting/labelling to udev & waiting for it to complete asychronously in the background. That leads to two completely different approaches for labelling files vs block devices making debugging harder and the overall system more complex & error prone.
libvirtd would chown and relabel but it would also trigger change event after it populates devices.udev. To make sure any change event that was possibly running before won't change permissions back. Waiting will be synchronous on libvirtd side. I admit it is a bit ugly but I think there is no other way how to solve this problem.
We can not do any sychronous wait against udev doing work - that will add an unacceptable delay in the VM startup process - it is already too long and we don't want to design a system that will make it even slower. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Thu, Oct 27, 2016 at 3:45 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
We can not do any sychronous wait against udev doing work - that will add an unacceptable delay in the VM startup process - it is already too long and we don't want to design a system that will make it even slower.
Note that if implemented it would be very targeted wait for one specific event. It *won't* be an equivalent of udevadm settle. In "uncontented" case udev's response should be very quick. Otherwise, something (e.g. lv resize) is still setting up a block device because change events are still being processed for it, and if that is the case, then just spawning qemu and hoping for the best doesn't seem right to me (disregarding SELinux and permission issues). Is libvirt really not waiting for udev at all? Michal
On Thu, Oct 27, 2016 at 04:02:33PM +0200, Michal Sekletar wrote:
On Thu, Oct 27, 2016 at 3:45 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
We can not do any sychronous wait against udev doing work - that will add an unacceptable delay in the VM startup process - it is already too long and we don't want to design a system that will make it even slower.
Note that if implemented it would be very targeted wait for one specific event. It *won't* be an equivalent of udevadm settle. In "uncontented" case udev's response should be very quick. Otherwise, something (e.g. lv resize) is still setting up a block device because change events are still being processed for it, and if that is the case, then just spawning qemu and hoping for the best doesn't seem right to me (disregarding SELinux and permission issues).
We should never be spawning a VM using a disk at the same time as the underlying device is appearing - other unrelated devices may be appearing/disappearing at this time, but not the one the VM is using. The only reason we're hitting this problem with udev is not because the storage is changing, it is because some other process (eg a background monitoring app) has closed the device triggerring the udev to arbitrarily change the devices. If we can just stop udev resetting device permissions on FD close we would not have to deal with any of this problem.
Is libvirt really not waiting for udev at all?
We only wait for udev in cases where we are dealing with storage setup. eg when we login to an iSCSI target, or when we activate a LVM volume group, etc. None of these things are in the VM startup code path. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Thu, Oct 27, 2016 at 4:09 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
We should never be spawning a VM using a disk at the same time as the underlying device is appearing - other unrelated devices may be appearing/disappearing at this time, but not the one the VM is using.
The only reason we're hitting this problem with udev is not because the storage is changing, it is because some other process (eg a background monitoring app) has closed the device triggerring the udev to arbitrarily change the devices.
udev by default watches some device nodes with inotify because there is no generic kernel interface, i.e. ioctl() that block device management tools can call once they finish their job. For example, if a filesystem is initialized we need to pull in some fs related information to udev database (e.g. fs label). But AFAIK there is no block layer ioctl that mkfs could call and tell kernel to generate change event for the device. Hence udev watches devices and synthesizes change event from user-space.
If we can just stop udev resetting device permissions on FD close we would not have to deal with any of this problem.
Is libvirt really not waiting for udev at all?
We only wait for udev in cases where we are dealing with storage setup. eg when we login to an iSCSI target, or when we activate a LVM volume group, etc. None of these things are in the VM startup code path.
Ok, so possible race I am trying to address is non-existent in practice. In that case I will not do that. Thanks, Michal
On 27.10.2016 02:13, Michal Sekletar wrote:
On Thu, Oct 27, 2016 at 8:37 AM, Jiri Denemark <jdenemar@redhat.com> wrote:
Yeah, the rule should really be part of this series.
I am working on udev hook-up. I found out that SECLABEL key doesn't support substitutions (see %c in man 7 udev). I need to fix that first and then we can have very simple udev rule file that employs helper. This is my WIP version,
# /etc/udev/rules.d/99-virt.rules ACTION!="add|change", GOTO="virt_rules_end" SUBSYSTEM!="block", GOTO="virt_rules_end"
PROGRAM="/usr/local/libexec/libvirt_udevhelper", OWNER="%c{1}", GROUP="%c{2}", SECLABEL{selinux}="%c{3}", TAG+="libvirt"
LABEL="virt_rules_end"
So probably we should require a fixed version of udev then in our spec file. And also, once you finish the rule, it should be installed by libvirt rpm, so please post it here so that I can add it to my patch set.
SECLABEL part doesn't work yet. Also helper uses DEVNODE env variable to obtain path to work with. It should use DEVNAME. This is my fault, because I told Michal to use DEVNODE. I've patched it locally for now. I will send PR to Michal with the fix.
Thank you, I've fixed this locally. Michal
Whenever a security driver wants to change label of some path, it should let virUdevMgr module know so that it can update its internal database too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 ++ src/security/security_dac.c | 36 ++++++++++++++++++++++++++++--- src/security/security_manager.c | 16 ++++++++++++++ src/security/security_manager.h | 5 +++++ src/security/security_selinux.c | 47 ++++++++++++++++++++++++++++++++++++++--- 5 files changed, 100 insertions(+), 6 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a0de4e9..66df1f7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1143,6 +1143,7 @@ virSecurityManagerGetModel; virSecurityManagerGetMountOptions; virSecurityManagerGetNested; virSecurityManagerGetProcessLabel; +virSecurityManagerGetUdevManager; virSecurityManagerNew; virSecurityManagerNewDAC; virSecurityManagerNewStack; @@ -1167,6 +1168,7 @@ virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; +virSecurityManagerSetUdevManager; virSecurityManagerStackAddNested; virSecurityManagerVerify; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 7f17124..54e59c7 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -356,7 +356,11 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, gid_t gid) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virUdevMgrPtr udevMgr = virSecurityManagerGetUdevManager(mgr); + virSecurityDeviceLabelDefPtr seclabel = NULL; + char *label = NULL; struct stat sb; + int ret = -1; if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) @@ -365,14 +369,36 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, if (path) { if (stat(path, &sb) < 0) { virReportSystemError(errno, _("unable to stat: %s"), path); - return -1; + return ret; } if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) < 0) - return -1; + return ret; + + if (udevMgr) { + if (virAsprintf(&label, "%u %u", + (unsigned int) uid, + (unsigned int) gid) < 0) + goto cleanup; + + if (!(seclabel = virSecurityDeviceLabelDefNewLabel(SECURITY_DAC_NAME, label))) + goto cleanup; + VIR_FREE(label); + } } - return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + goto cleanup; + + if (udevMgr && path && + virUdevMgrAddLabel(udevMgr, path, seclabel) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(label); + virSecurityDeviceLabelDefFree(seclabel); + return ret; } @@ -382,6 +408,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, const char *path) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virUdevMgrPtr udevMgr = virSecurityManagerGetUdevManager(mgr); int rv; uid_t uid = 0; /* By default return to root:root */ gid_t gid = 0; @@ -399,6 +426,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, return -1; if (rv > 0) return 0; + + if (udevMgr) + virUdevMgrRemoveAllLabels(udevMgr, path); } return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); diff --git a/src/security/security_manager.c b/src/security/security_manager.c index ecb4a40..2f07d6a 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -39,6 +39,7 @@ struct _virSecurityManager { virSecurityDriverPtr drv; unsigned int flags; const char *virtDriver; + virUdevMgrPtr udevMgr; void *privateData; }; @@ -1001,3 +1002,18 @@ virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr, return 0; } + + +void +virSecurityManagerSetUdevManager(virSecurityManagerPtr mgr, + virUdevMgrPtr udevMgr) +{ + mgr->udevMgr = virObjectRef(udevMgr); +} + + +virUdevMgrPtr +virSecurityManagerGetUdevManager(virSecurityManagerPtr mgr) +{ + return mgr->udevMgr; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 4cbc2d8..8f565f7 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -26,6 +26,7 @@ # include "domain_conf.h" # include "vircommand.h" # include "virstoragefile.h" +# include "virudev.h" typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; @@ -164,4 +165,8 @@ int virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *path); +void virSecurityManagerSetUdevManager(virSecurityManagerPtr mgr, + virUdevMgrPtr udevMgr); +virUdevMgrPtr virSecurityManagerGetUdevManager(virSecurityManagerPtr mgr); + #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5dad22c..c85f500 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -947,7 +947,24 @@ virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, const char *path, char *tcon) { bool privileged = virSecurityManagerGetPrivileged(mgr); - return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged); + virUdevMgrPtr udevMgr = virSecurityManagerGetUdevManager(mgr); + virSecurityDeviceLabelDefPtr seclabel = NULL; + int rc; + + if (udevMgr && + !(seclabel = virSecurityDeviceLabelDefNewLabel(SECURITY_SELINUX_NAME, tcon))) + return -1; + + rc = virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged); + + if (udevMgr && + virUdevMgrAddLabel(udevMgr, path, seclabel) < 0) { + virSecurityDeviceLabelDefFree(seclabel); + return -1; + } + + virSecurityDeviceLabelDefFree(seclabel); + return rc; } static int @@ -955,7 +972,24 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, char *tcon) { bool privileged = virSecurityManagerGetPrivileged(mgr); - return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged); + virUdevMgrPtr udevMgr = virSecurityManagerGetUdevManager(mgr); + virSecurityDeviceLabelDefPtr seclabel = NULL; + int rc; + + if (udevMgr && + !(seclabel = virSecurityDeviceLabelDefNewLabel(SECURITY_SELINUX_NAME, tcon))) + return -1; + + rc = virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged); + + if (udevMgr && + virUdevMgrAddLabel(udevMgr, path, seclabel) < 0) { + virSecurityDeviceLabelDefFree(seclabel); + return -1; + } + + virSecurityDeviceLabelDefFree(seclabel); + return rc; } static int @@ -1018,6 +1052,8 @@ static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, const char *path) { + virUdevMgrPtr udevMgr = virSecurityManagerGetUdevManager(mgr); + bool privileged = virSecurityManagerGetPrivileged(mgr); struct stat buf; security_context_t fcon = NULL; int rc = -1; @@ -1038,6 +1074,11 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto err; } + if (udevMgr) { + virUdevMgrRemoveAllLabels(udevMgr, path); + virUdevMgrRemoveAllLabels(udevMgr, newpath); + } + if (stat(newpath, &buf) != 0) { VIR_WARN("cannot stat %s: %s", newpath, virStrerror(errno, ebuf, sizeof(ebuf))); @@ -1051,7 +1092,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, VIR_WARN("cannot lookup default selinux label for %s", newpath); rc = 0; } else { - rc = virSecuritySELinuxSetFilecon(mgr, newpath, fcon); + rc = virSecuritySELinuxSetFileconHelper(newpath, fcon, false, privileged); } err: -- 2.8.4
Not everybody is going to use the new virUdevMgr module. Some users have their own set of udev rules and they don't need libvirt to step in that area. Lets give users choice to enable or disable this feature. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 +++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/qemu_driver.c | 8 ++++++++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 23 insertions(+) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 73ebeda..08e0803 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -70,6 +70,7 @@ module Libvirtd_qemu = | str_array_entry "cgroup_controllers" | str_array_entry "cgroup_device_acl" | int_entry "seccomp_sandbox" + | bool_entry "write_udev" let save_entry = str_entry "save_image_format" | str_entry "dump_image_format" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index c4fcb6d..a34975a 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -293,6 +293,11 @@ # guests will be blocked. Defaults to 0. #security_require_confined = 1 +# In order to avoid races between libvirt and udev who also changes security +# labels on devices, libvirt can let know what devices belong a domain managed +# by libvirt and thus reason udev to not mangle security labels. +#write_udev = 1 + # The user for QEMU processes run by the system instance. It can be # specified as a user name or as a user id. The qemu driver will try to # parse this value first as a name and then, if the name doesn't exist, diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 109668b..c0be670 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -795,6 +795,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, } } + if (virConfGetValueBool(conf, "write_udev", &cfg->writeUdev) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 12b2661..b42eea7 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -192,6 +192,8 @@ struct _virQEMUDriverConfig { virFirmwarePtr *firmwares; size_t nfirmwares; + + bool writeUdev; }; /* Main driver state */ @@ -269,6 +271,9 @@ struct _virQEMUDriver { /* Immutable pointer, self-locking APIs */ virHashAtomicPtr migrationErrors; + + /* Immutable pointer, self-locking APIs*/ + virUdevMgrPtr udevMgr; }; typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3a51826..7dbbc25 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -378,6 +378,10 @@ qemuSecurityInit(virQEMUDriverPtr driver) virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); unsigned int flags = 0; + if (cfg->writeUdev && + !(driver->udevMgr = virUdevMgrNew())) + goto error; + if (cfg->allowDiskFormatProbing) flags |= VIR_SECURITY_MANAGER_ALLOW_DISK_PROBE; if (cfg->securityDefaultConfined) @@ -395,6 +399,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) QEMU_DRIVER_NAME, flags))) goto error; + virSecurityManagerSetUdevManager(mgr, driver->udevMgr); if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) goto error; @@ -410,6 +415,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) QEMU_DRIVER_NAME, flags))) goto error; + virSecurityManagerSetUdevManager(mgr, driver->udevMgr); if (!(stack = virSecurityManagerNewStack(mgr))) goto error; mgr = NULL; @@ -424,6 +430,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) flags, qemuSecurityChownCallback))) goto error; + virSecurityManagerSetUdevManager(mgr, driver->udevMgr); if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) goto error; @@ -1091,6 +1098,7 @@ qemuStateCleanup(void) VIR_FREE(qemu_driver->qemuImgBinary); virObjectUnref(qemu_driver->securityManager); + virObjectUnref(qemu_driver->udevMgr); ebtablesContextFree(qemu_driver->ebtables); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 805fa0e..112f343 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -33,6 +33,7 @@ module Test_libvirtd_qemu = { "security_driver" = "selinux" } { "security_default_confined" = "1" } { "security_require_confined" = "1" } +{ "write_udev" = "1" } { "user" = "root" } { "group" = "root" } { "dynamic_ownership" = "1" } -- 2.8.4
Now that security drivers are capable of writing into virUdevMgr module, we also need it to flush its internal database right after that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 12 +++++++++++- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_driver.c | 9 +++++++-- src/qemu/qemu_hotplug.c | 35 ++++++++++++++++++++++++++++------- src/qemu/qemu_process.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_process.h | 3 +++ 6 files changed, 96 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 838e838..4422fbc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -641,13 +641,15 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv) /* qemuDomainMasterKeyRemove: + * @driver: qemu driver data * @priv: Pointer to the domain private object * * Remove the traces of the master key, clear the heap, clear the file, * delete the file. */ void -qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv) +qemuDomainMasterKeyRemove(virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv) { char *path = NULL; @@ -660,6 +662,8 @@ qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv) /* Delete the master key file */ path = qemuDomainGetMasterKeyFilePath(priv->libDir); unlink(path); + if (driver->udevMgr) + virUdevMgrRemoveAllLabels(driver->udevMgr, path); VIR_FREE(path); } @@ -4935,6 +4939,9 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, vm->def, elem) < 0) VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); + if (qemuProcessFlushUdev(driver) < 0) + VIR_WARN("Unable to clean up udev rules"); + if (qemuTeardownImageCgroup(vm, elem) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(elem->path)); @@ -4974,6 +4981,9 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, elem) < 0) goto cleanup; + if (qemuProcessFlushUdev(driver) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2ee1829..a381b59 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -710,7 +710,8 @@ int qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver, int qemuDomainMasterKeyCreate(virDomainObjPtr vm); -void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv); +void qemuDomainMasterKeyRemove(virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv); void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7dbbc25..cc34782 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3864,8 +3864,11 @@ qemuDomainScreenshot(virDomainPtr dom, endjob: VIR_FORCE_CLOSE(tmp_fd); - if (unlink_tmp) + if (unlink_tmp) { + virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm->def, tmp); + qemuProcessFlushUdev(driver); unlink(tmp); + } VIR_FREE(tmp); qemuDomainObjEndJob(driver, vm); @@ -6682,6 +6685,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm->def, path) < 0) VIR_WARN("failed to restore save state label on %s", path); + qemuProcessFlushUdev(driver); virObjectUnref(cfg); return ret; } @@ -16071,7 +16075,8 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, disk) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, - disk) < 0)) + disk) < 0 || + qemuProcessFlushUdev(driver) < 0)) goto cleanup; disk->src = oldsrc; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9746a06..fdac464 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -113,6 +113,9 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, vm->def, disk) < 0) goto rollback_lock; + if (qemuProcessFlushUdev(driver) < 0) + goto rollback_label; + if (qemuSetupDiskCgroup(vm, disk) < 0) goto rollback_label; @@ -130,6 +133,9 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, VIR_WARN("Unable to restore security label on %s", virDomainDiskGetSource(disk)); + if (qemuProcessFlushUdev(driver) < 0) + VIR_WARN("Unable to clean up udev rules"); + rollback_lock: if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", @@ -1427,6 +1433,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, goto error; if (backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) teardownlabel = true; + if (qemuProcessFlushUdev(driver) < 0) + goto error; if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto error; @@ -1476,10 +1484,10 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); if (teardownlabel && - virSecurityManagerRestoreHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) + (virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0 || + qemuProcessFlushUdev(driver) < 0)) VIR_WARN("Unable to restore host device labelling on hotplug fail"); - if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); @@ -2262,6 +2270,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, vm->def, hostdev, NULL) < 0) goto cleanup; teardownlabel = true; + if (qemuProcessFlushUdev(driver) < 0) + goto cleanup; if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto cleanup; @@ -2289,8 +2299,9 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); if (teardownlabel && - virSecurityManagerRestoreHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) + (virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0 || + qemuProcessFlushUdev(driver) < 0)) VIR_WARN("Unable to restore host device labelling on hotplug fail"); if (added) qemuHostdevReAttachUSBDevices(driver, vm->def->name, &hostdev, 1); @@ -2364,6 +2375,9 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, goto cleanup; teardownlabel = true; + if (qemuProcessFlushUdev(driver) < 0) + goto cleanup; + if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto cleanup; @@ -2407,8 +2421,9 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); if (teardownlabel && - virSecurityManagerRestoreHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) + (virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0 || + qemuProcessFlushUdev(driver) < 0)) VIR_WARN("Unable to restore host device labelling on hotplug fail"); } VIR_FREE(drivealias); @@ -3315,6 +3330,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", src); + if (qemuProcessFlushUdev(driver) < 0) + VIR_WARN("Unable to clean up udev rules"); + if (qemuTeardownDiskCgroup(vm, disk) < 0) VIR_WARN("Failed to tear down cgroup for disk path %s", src); @@ -3493,6 +3511,9 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, vm->def, hostdev, NULL) < 0) VIR_WARN("Failed to restore host device labelling"); + if (qemuProcessFlushUdev(driver) < 0) + VIR_WARN("Unable to clean up udev rules"); + if (qemuTeardownHostdevCgroup(vm, hostdev) < 0) VIR_WARN("Failed to remove host device cgroup ACL"); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b67aee..10f29ff 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5499,6 +5499,9 @@ qemuProcessLaunch(virConnectPtr conn, */ ret = -2; + if (qemuProcessFlushUdev(driver) < 0) + goto cleanup; + if (incoming && incoming->fd != -1) { /* if there's an fd to migrate from, and it's a pipe, put the * proper security label on it @@ -5998,10 +6001,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, } /* Remove the master key */ - qemuDomainMasterKeyRemove(priv); + qemuDomainMasterKeyRemove(driver, priv); virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir); + if (driver->udevMgr) { + virUdevMgrRemoveAllLabels(driver->udevMgr, priv->libDir); + virUdevMgrRemoveAllLabels(driver->udevMgr, priv->channelTargetDir); + } qemuDomainClearPrivatePaths(vm); @@ -6033,10 +6040,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, } /* Reset Security Labels unless caller don't want us to */ - if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL)) + if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL)) { virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, !!(flags & VIR_QEMU_PROCESS_STOP_MIGRATED)); + qemuProcessFlushUdev(driver); + } virSecurityManagerReleaseLabel(driver->securityManager, vm->def); for (i = 0; i < vm->def->ndisks; i++) { @@ -6587,3 +6596,37 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver, virHashFree(table); return ret; } + + +char * +qemuProcessGetUdevPath(virQEMUDriverPtr driver) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + char *file; + + ignore_value(virAsprintf(&file, "%s/devices.udev", cfg->stateDir)); + virObjectUnref(cfg); + return file; +} + + +int +qemuProcessFlushUdev(virQEMUDriverPtr driver) +{ + char *file = NULL; + int ret = -1; + + if (!driver->udevMgr) + return 0; + + if (!(file = qemuProcessGetUdevPath(driver))) + goto cleanup; + + if (virUdevMgrDumpFile(driver->udevMgr, file) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(file); + return ret; +} diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 21f3b0c..8d1e937 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -191,4 +191,7 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob); +char * qemuProcessGetUdevPath(virQEMUDriverPtr driver); +int qemuProcessFlushUdev(virQEMUDriverPtr driver); + #endif /* __QEMU_PROCESS_H__ */ -- 2.8.4
If the daemon is restarted, the virUdevMgr loses its internal state. This is because entries to its internal DB are added whilst setting security labels. This obviously doesn't happen when the daemon is restarted. It's not wise to start with a fresh internal state and possibly leave behind previous one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cc34782..51122d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -377,10 +377,20 @@ qemuSecurityInit(virQEMUDriverPtr driver) virSecurityManagerPtr stack = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); unsigned int flags = 0; + char *file = NULL; - if (cfg->writeUdev && - !(driver->udevMgr = virUdevMgrNew())) - goto error; + if (cfg->writeUdev) { + if (!(file = qemuProcessGetUdevPath(driver))) + goto error; + + if (virFileExists(file)) { + if (!(driver->udevMgr = virUdevMgrNewFromFile(file))) + goto error; + } else { + if (!(driver->udevMgr = virUdevMgrNew())) + goto error; + } + } if (cfg->allowDiskFormatProbing) flags |= VIR_SECURITY_MANAGER_ALLOW_DISK_PROBE; @@ -442,12 +452,14 @@ qemuSecurityInit(virQEMUDriverPtr driver) } driver->securityManager = stack; + VIR_FREE(file); virObjectUnref(cfg); return 0; error: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to initialize security drivers")); + VIR_FREE(file); virObjectUnref(stack); virObjectUnref(mgr); virObjectUnref(cfg); -- 2.8.4
In some cases callers might want to filter what devices are stored in this module (esp. when used in combination with udev who cares about nothing but "/dev/" prefixed paths). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virudev.c | 25 ++++++++++++++++++++++ src/util/virudev.h | 13 ++++++++++++ tests/virudevtest.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 92 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 66df1f7..60dc16b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2584,6 +2584,7 @@ virUdevMgrNew; virUdevMgrNewFromFile; virUdevMgrNewFromStr; virUdevMgrRemoveAllLabels; +virUdevMgrSetFilter; # util/viruri.h diff --git a/src/util/virudev.c b/src/util/virudev.c index 9940f5f..c17682c 100644 --- a/src/util/virudev.c +++ b/src/util/virudev.c @@ -36,6 +36,9 @@ struct _virUdevMgr { virObjectLockable parent; virHashTablePtr labels; + + virUdevMgrFilter filter; + void *opaque; }; struct _udevSeclabel { @@ -274,6 +277,17 @@ virUdevMgrAddLabel(virUdevMgrPtr mgr, virObjectLock(mgr); + if (mgr->filter) { + int rc = mgr->filter(device, seclabel, mgr->opaque); + if (rc < 0) + goto cleanup; + if (rc == 0) { + /* Claim success. */ + ret = 0; + goto cleanup; + } + } + if ((list = virHashLookup(mgr->labels, device))) { virSecurityDeviceLabelDefPtr entry = udevSeclabelFindByModel(list, seclabel->model); @@ -561,3 +575,14 @@ virUdevMgrNewFromFile(const char *filename) VIR_FREE(state); return NULL; } + +void +virUdevMgrSetFilter(virUdevMgrPtr mgr, + virUdevMgrFilter filter, + void *opaque) +{ + virObjectLock(mgr); + mgr->filter = filter; + mgr->opaque = opaque; + virObjectUnlock(mgr); +} diff --git a/src/util/virudev.h b/src/util/virudev.h index 4e286bb..2f035e3 100644 --- a/src/util/virudev.h +++ b/src/util/virudev.h @@ -28,6 +28,15 @@ typedef struct _virUdevMgr virUdevMgr; typedef virUdevMgr *virUdevMgrPtr; +/* Filter some devices out in virUdevMgrAddLabel. + * Return 0 to NOT record label for device, + * 1 to record the label for device, + * -1 on error. + */ +typedef int (*virUdevMgrFilter)(const char *device, + const virSecurityDeviceLabelDef *seclabel, + void *opaque); + virUdevMgrPtr virUdevMgrNew(void); virUdevMgrPtr virUdevMgrNewFromStr(const char *str); virUdevMgrPtr virUdevMgrNewFromFile(const char *filename); @@ -47,4 +56,8 @@ char *virUdevMgrDumpStr(virUdevMgrPtr mgr); int virUdevMgrDumpFile(virUdevMgrPtr mgr, const char *filename); +void virUdevMgrSetFilter(virUdevMgrPtr mgr, + virUdevMgrFilter filter, + void *opaque); + #endif diff --git a/tests/virudevtest.c b/tests/virudevtest.c index 36a9077..cd5d136 100644 --- a/tests/virudevtest.c +++ b/tests/virudevtest.c @@ -29,6 +29,7 @@ struct testUdevData { const char *file; const char *const *labels; + virUdevMgrFilter filter; }; @@ -49,6 +50,8 @@ testDump(const void *opaque) if (!(mgr = virUdevMgrNew())) goto cleanup; + virUdevMgrSetFilter(mgr, data->filter, NULL); + tmp = data->labels; while (*tmp) { const char *device; @@ -194,6 +197,25 @@ testLookup(const void *opaque) static int +filterAll(const char *device ATTRIBUTE_UNUSED, + const virSecurityDeviceLabelDef *seclabel ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return 0; +} + + +static int +filterAllowSda(const char *device, + const virSecurityDeviceLabelDef *seclabel ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return STRPREFIX(device, "/dev/sda") ? 1 : 0; +} + + + +static int mymain(void) { int ret = 0; @@ -202,7 +224,7 @@ mymain(void) do { \ const char *labels[] = {__VA_ARGS__, NULL}; \ struct testUdevData data = { \ - .file = filename, .labels = labels, \ + .file = filename, .labels = labels, .filter = NULL, \ }; \ if (virTestRun("Dump " filename, testDump, &data) < 0) \ ret = -1; \ @@ -227,6 +249,17 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_FILTER(filename, fltr, ...) \ + do { \ + const char *labels[] = {__VA_ARGS__, NULL}; \ + struct testUdevData data = { \ + .file = filename, .labels = labels, .filter = fltr, \ + }; \ + if (virTestRun("Filter " filename, testDump, &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_DUMP("empty", NULL); DO_TEST_DUMP("simple-selinux", "/dev/sda", "selinux", "someSELinuxLabel"); @@ -254,6 +287,25 @@ mymain(void) "/dev/sdb", "dac", "otherDACLabel", "/dev/sdb", "selinux", "otherSELinuxLabel"); + DO_TEST_FILTER("empty", filterAll, + "/dev/sda", "dac", "someDACLabel", + "/dev/sda", "selinux", "someSELinuxLabel", + "/dev/sdb", "dac", "otherDACLabel", + "/dev/sdb", "selinux", "otherSELinuxLabel"); + DO_TEST_FILTER("simple-selinux", filterAllowSda, + "/dev/sda", "selinux", "someSELinuxLabel", + "/dev/sdb", "dac", "otherDACLabel", + "/dev/sdb", "selinux", "otherSELinuxLabel"); + DO_TEST_FILTER("simple-dac", filterAllowSda, + "/dev/sda", "dac", "someDACLabel", + "/dev/sdb", "dac", "otherDACLabel", + "/dev/sdb", "selinux", "otherSELinuxLabel"); + DO_TEST_FILTER("complex", NULL, + "/dev/sda", "dac", "someDACLabel", + "/dev/sda", "selinux", "someSELinuxLabel", + "/dev/sdb", "dac", "otherDACLabel", + "/dev/sdb", "selinux", "otherSELinuxLabel"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.8.4
In case of udev, it will never try to reset security label on say domain monitor socket, or some other channel. Therefore, it makes sense to filter those paths out and keep the state file on the disk small. The only paths that udev will handle are those prefixed with "/dev/". Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 51122d0..0fe91b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -370,6 +370,15 @@ qemuSecurityChownCallback(virStorageSourcePtr src, static int +qemuUdevFilter(const char *devpath, + const virSecurityDeviceLabelDef *seclabel ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return STRPREFIX(devpath, "/dev/") ? 1 : 0; +} + + +static int qemuSecurityInit(virQEMUDriverPtr driver) { char **names; @@ -390,6 +399,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) if (!(driver->udevMgr = virUdevMgrNew())) goto error; } + + virUdevMgrSetFilter(driver->udevMgr, qemuUdevFilter, NULL); } if (cfg->allowDiskFormatProbing) -- 2.8.4
Michal Privoznik <mprivozn@redhat.com> [2016-10-26, 02:52PM +0200]:
I've came across interesting bug recently. The problem was that user tried to start a domain, but qemu was denied access to some device. Even though we relabelled it initially. By debugging I found the root cause: while we were starting qemu, udev came and restored original security labels. Sigh. We have two options here:
a) write out series of udev rules so that whenever it tries to relabel something our rule will stop it from doing so
b) write a small helper binary that will udev call in order to: 1) detect whether device is in use by libvirt 2) get seclabel that was set by libvirt
These patches implement the latter approach. While these patches make life easier for us, there is still a race when udev might restore the device's seclabel before we had chance to flush internal database of seclabels for the helper binary. This is something I'm currently focusing on. But before I get there, here are patches that makes the problem much more bearable.
In case you want to try these patches, here are some scratch builds:
https://mprivozn.fedorapeople.org/udev/
Also, you can find them on my branch:
https://github.com/zippy2/libvirt/commits/udev_labels2
This beast is turned off by default, to turn it on you'll need to add:
write_udev=1
to qemu.conf.
Hello Michal, will this work (or can be made working) for file-based disk images as well? Currently, the user can provoke a race condition when defining two domains using the same file-based disk image and not (wrongly) specifying the <shareable/> element. What happens is that when the while the first domain is running, the second domain start will relabel disk image for its own usage, essentially cutting of the first running domain. This caused a crash of the first QEMU process until https://patchwork.ozlabs.org/patch/658163/, now the first domain still reports I/O errors. I have investigated this problem for a while now and have seen various bug reports (https://bugzilla.redhat.com/show_bug.cgi?id=1191802, https://bugzilla.redhat.com/show_bug.cgi?id=547546) and your attempt to fix this (https://www.redhat.com/archives/libvir-list/2015-October/msg00331.html) but unfortunately this disruptive behaviour still exists. Best regards, Bjoern -- IBM Systems Linux on z Systems & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Schönaicher Str. 220 71032 Böblingen Phone: +49 7031 16 1819 E-Mail: bwalk@de.ibm.com ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Oct 27, 2016 at 01:07:57PM +0200, Bjoern Walk wrote:
Michal Privoznik <mprivozn@redhat.com> [2016-10-26, 02:52PM +0200]:
I've came across interesting bug recently. The problem was that user tried to start a domain, but qemu was denied access to some device. Even though we relabelled it initially. By debugging I found the root cause: while we were starting qemu, udev came and restored original security labels. Sigh. We have two options here:
a) write out series of udev rules so that whenever it tries to relabel something our rule will stop it from doing so
b) write a small helper binary that will udev call in order to: 1) detect whether device is in use by libvirt 2) get seclabel that was set by libvirt
These patches implement the latter approach. While these patches make life easier for us, there is still a race when udev might restore the device's seclabel before we had chance to flush internal database of seclabels for the helper binary. This is something I'm currently focusing on. But before I get there, here are patches that makes the problem much more bearable.
In case you want to try these patches, here are some scratch builds:
https://mprivozn.fedorapeople.org/udev/
Also, you can find them on my branch:
https://github.com/zippy2/libvirt/commits/udev_labels2
This beast is turned off by default, to turn it on you'll need to add:
write_udev=1
to qemu.conf.
Hello Michal,
will this work (or can be made working) for file-based disk images as well? Currently, the user can provoke a race condition when defining two domains using the same file-based disk image and not (wrongly) specifying the <shareable/> element. What happens is that when the while the first domain is running, the second domain start will relabel disk image for its own usage, essentially cutting of the first running domain. This caused a crash of the first QEMU process until https://patchwork.ozlabs.org/patch/658163/, now the first domain still reports I/O errors.
That is a separate issue - that requires libvirt to be tracking the labelling it is doing. There was a series that could fix this, which was related to restore of originall labelling on guest shutdown. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On 27.10.2016 04:07, Bjoern Walk wrote:
Michal Privoznik <mprivozn@redhat.com> [2016-10-26, 02:52PM +0200]:
I've came across interesting bug recently. The problem was that user tried to start a domain, but qemu was denied access to some device. Even though we relabelled it initially. By debugging I found the root cause: while we were starting qemu, udev came and restored original security labels. Sigh. We have two options here:
a) write out series of udev rules so that whenever it tries to relabel something our rule will stop it from doing so
b) write a small helper binary that will udev call in order to: 1) detect whether device is in use by libvirt 2) get seclabel that was set by libvirt
These patches implement the latter approach. While these patches make life easier for us, there is still a race when udev might restore the device's seclabel before we had chance to flush internal database of seclabels for the helper binary. This is something I'm currently focusing on. But before I get there, here are patches that makes the problem much more bearable.
In case you want to try these patches, here are some scratch builds:
https://mprivozn.fedorapeople.org/udev/
Also, you can find them on my branch:
https://github.com/zippy2/libvirt/commits/udev_labels2
This beast is turned off by default, to turn it on you'll need to add:
write_udev=1
to qemu.conf.
Hello Michal,
will this work (or can be made working) for file-based disk images as well? Currently, the user can provoke a race condition when defining two domains using the same file-based disk image and not (wrongly) specifying the <shareable/> element. What happens is that when the while the first domain is running, the second domain start will relabel disk image for its own usage, essentially cutting of the first running domain. This caused a crash of the first QEMU process until https://patchwork.ozlabs.org/patch/658163/, now the first domain still reports I/O errors.
As Dan says, that's a separate issue. But if we have this helper in, we might turn it later into a small service that would remember seclabels (and refcount them). There was a patch set earlier to implement just this but for some reason it was abandoned. But I don't want to say this out loud and scare possible reviewers O:-) Michal
participants (5)
-
Bjoern Walk -
Daniel P. Berrange -
Jiri Denemark -
Michal Privoznik -
Michal Sekletar