12:21 p.m.
On 01/06/2011 05:35 AM, Daniel P. Berrange wrote:
The current security driver usage requires horrible code like
if (driver->securityDriver &&
driver->securityDriver->domainSetSecurityHostdevLabel &&
driver->securityDriver->domainSetSecurityHostdevLabel(driver->securityDriver,
vm, hostdev) < 0)
This is a v2 of the earlier
https://www.redhat.com/archives/libvir-list/2010-November/msg00984.html,
but omits the rest of the 8-patch series that v1 was included with.
That's okay, but I'm a bit curious on the progress of the rest of that
series (in part because it involved adding virCommand handshaking, where
I'm not sure if I need to lend a hand at getting that in) :)
This pair of checks for NULL clutters up the code, making the driver
calls 2 lines longer than they really need to be. The goal of the
patchset is to change the calling convention to simply
if (virSecurityManagerSetHostdevLabel(driver->securityDriver,
vm, hostdev) < 0)
-qemudSecurityInit(struct qemud_driver *qemud_drv)
+qemudSecurityInit(struct qemud_driver *driver)
{
+ virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName,
+
driver->allowDiskFormatProbing);
+ if (!mgr)
return -1;
- }
- /* No primary security driver wanted to be enabled: just setup
- * the DAC driver on its own */
- if (ret == -2) {
- qemud_drv->securityDriver = &qemuDACSecurityDriver;
- VIR_INFO0(_("No security driver available"));
+ if (driver->privileged) {
+ virSecurityManagerPtr dac = virSecurityManagerNewDAC(driver->user,
+ driver->group,
+
driver->allowDiskFormatProbing,
+
driver->dynamicOwnership);
+ if (!dac)
+ return -1;
Does this leak mgr?
+
+ if (!(driver->securityManager = virSecurityManagerNewStack(mgr,
+ dac)))
+ return -1;
Likewise.
@@ -1555,7 +1540,6 @@ qemudShutdown(void) {
VIR_FREE(qemu_driver->spicePassword);
VIR_FREE(qemu_driver->hugetlbfs_mount);
VIR_FREE(qemu_driver->hugepage_path);
- VIR_FREE(qemu_driver->securityDriverName);
VIR_FREE(qemu_driver->saveImageFormat);
VIR_FREE(qemu_driver->dumpImageFormat);
Is there any state in a virSecurityManagerPtr that might necessitate a
cleanup function (and even if there isn't right now, what happens if we
extend virSecurityManager in the future), such that you are missing a
call here to virSecurityManagerFree(qemu_driver->securityManager)?
diff --git a/src/security/security_apparmor.c
b/src/security/security_apparmor.c
index 468d0a3..d82ba73 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -1,4 +1,3 @@
-
/*
* AppArmor security driver for libvirt
* Copyright (C) 2009-2010 Canonical Ltd.
Add 2011 and/or Red Hat copyright annotations?
diff --git a/src/security/security_dac.c
b/src/security/security_dac.c
new file mode 100644
index 0000000..edecaf9
--- /dev/null
+++ b/src/security/security_dac.c
@@ -0,0 +1,703 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
2010-2011
+ *
+ * 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.
+ *
+ * QEMU POSIX DAC security driver
Do we still need the term QEMU here, or should it be dropped now that
it's generic?
+static int
+virSecurityDACSetOwnership(const char *path, int uid, int gid)
+{
+ VIR_INFO("Setting DAC user and group on '%s' to '%d:%d'",
path, uid, gid);
%d and uid/gid are not compatible on cygwin; in util/util.c, we use %u,
(unsigned int)uid for a portable alternative. (I'm not sure if this
file would end up being compiled on cygwin, even though the old qemu
version has been skipped to date). Multiple instances, but worth a
separate cleanup patch, similar to commit c685993d7, so that this
(already large) patch remains as a straight code motion with no hidden
cleanup.
+
+static int
+virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
+ const char *path,
+ size_t depth ATTRIBUTE_UNUSED,
+ void *opaque)
+{
+ virSecurityManagerPtr mgr = opaque;
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
Rather than passing mgr as opaque, and recomputing priv each time
through the loop, ...
+
+ return virSecurityDACSetOwnership(path, priv->user, priv->group);
+}
+
+
+static int
+virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainDiskDefPtr disk)
+
+{
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+
+ if (!priv->dynamicOwnership)
+ return 0;
+
+ return virDomainDiskDefForeachPath(disk,
+
virSecurityManagerGetAllowDiskFormatProbing(mgr),
+ false,
+ virSecurityDACSetSecurityFileLabel,
+ mgr);
why not just pass priv as opaque in the first place?
+static int
+virSecurityDACSetSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED,
+ const char *file,
+ void *opaque)
+{
+ virSecurityManagerPtr mgr = opaque;
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+static int
+virSecurityDACSetSecurityUSBLabel(usbDevice *dev ATTRIBUTE_UNUSED,
+ const char *file,
+ void *opaque)
+{
+ virSecurityManagerPtr mgr = opaque;
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+static int
+virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainHostdevDefPtr dev)
+{
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ ret = usbDeviceFileIterate(usb,
virSecurityDACSetSecurityUSBLabel, mgr);
+ ret = pciDeviceFileIterate(pci,
virSecurityDACSetSecurityPCILabel, mgr);
Likewise.
+
+static int
+virSecurityDACRestoreSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED,
+ const char *file,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+ return virSecurityDACRestoreSecurityFileLabel(file);
+static int
+virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ virDomainHostdevDefPtr dev)
+
+{
+ ret = usbDeviceFileIterate(usb,
virSecurityDACRestoreSecurityUSBLabel, mgr);
Why pass mgr, when it is unused, and when the pre-code-motion version
passed NULL?
+ /* XXX fixme - we need to recursively label the entriy tree
:-( */
Faithful copy of the typo (s/entriy/entire/)
diff --git a/src/security/security_dac.h
b/src/security/security_dac.h
new file mode 100644
index 0000000..b690236
--- /dev/null
+++ b/src/security/security_dac.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
2010-2011
+
+void virSecurityDACSetUser(virSecurityManagerPtr mgr,
+ uid_t user);
+void virSecurityDACSetGroup(virSecurityManagerPtr mgr,
+ gid_t group);
+
+void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
+ bool dynamic);
Setters, but no getters. I guess that's okay for now.
diff --git a/src/security/security_manager.c
b/src/security/security_manager.c
new file mode 100644
index 0000000..7ab6e37
--- /dev/null
+++ b/src/security/security_manager.c
@@ -0,0 +1,291 @@
No copyright header?
+virSecurityManagerPtr
virSecurityManagerNewStack(virSecurityManagerPtr primary,
+ virSecurityManagerPtr secondary)
+{
+ virSecurityManagerPtr mgr =
+ virSecurityManagerNewDriver(&virSecurityDriverStack,
+
virSecurityManagerGetAllowDiskFormatProbing(primary));
Should this be
virSecurityManagerGetAllowDiskFormatProbing(primary) ||
virSecurityManagerGetAllowDiskFormatProbing(secondary)
or is the intent that creating a stack allows you to override probing
permitted in the secondary with a primary that disables probing?
+
+ virSecurityStackSetPrimary(mgr, primary);
+ virSecurityStackSetSecondary(mgr, secondary);
You need an early exit if mgr is NULL, since virSecurityStackSetPrimary
isn't too happy with a NULL mgr.
+virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user,
+ gid_t group,
+ bool allowDiskFormatProbing,
+ bool dynamicOwnership)
+{
+ virSecurityManagerPtr mgr =
+ virSecurityManagerNewDriver(&virSecurityDriverDAC,
+ allowDiskFormatProbing);
+
+ virSecurityDACSetUser(mgr, user);
Likewise about needing an early exit if mgr is NULL.
+void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
+{
+ return mgr + sizeof(mgr);
Won't work. You meant to do:
return (char *) mgr + sizeof(mgr);
otherwise, you are doing pointer arithmetic on pointers that are
sizeof(mgr) large, and accessing (WAAAY) beyond array bounds. (I'm
surprised it didn't dump core on you during testing.) And yes, I meant
to cast to (char*), even though the return type is (void*), since
pointer arithmetic on (void*) is a gcc extension not required by C99.
+void virSecurityManagerFree(virSecurityManagerPtr mgr)
This function was not exported. (Hmm, it answers my earlier question
about qemu cleanup failing to call this function).
diff --git a/src/security/security_manager.h
b/src/security/security_manager.h
new file mode 100644
index 0000000..c0ef84f
--- /dev/null
+++ b/src/security/security_manager.h
@@ -0,0 +1,74 @@
+
+#ifndef VIR_SECURITY_MANAGER_H__
No copyright header?
+#define VIR_SECURITY_MANAGER_H__
+
+# define virSecurityReportError(code, ...) \
+ virReportErrorHelper(NULL, VIR_FROM_SECURITY, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
Line up those \.
diff --git a/src/security/security_nop.c
b/src/security/security_nop.c
new file mode 100644
index 0000000..947cf55
--- /dev/null
+++ b/src/security/security_nop.c
@@ -0,0 +1,168 @@
+
+
+#include <config.h>
No copyright header?
diff --git a/src/security/security_nop.h
b/src/security/security_nop.h
new file mode 100644
index 0000000..714e646
--- /dev/null
+++ b/src/security/security_nop.h
@@ -0,0 +1,17 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
s/2010/2011/ (unlike other headers where you are doing code motion and
preserving the old copyright years, this file is completely new)
diff --git a/src/security/security_stack.c
b/src/security/security_stack.c
new file mode 100644
index 0000000..9b3753a
--- /dev/null
+++ b/src/security/security_stack.c
@@ -0,0 +1,383 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
2010-2011
+
+static const char * virSecurityStackGetModel(virSecurityManagerPtr mgr)
Unusual formatting; I'd expect either:
static const char *virSecurityStackGetModel(virSecurityManagerPtr mgr)
or
static const char *
virSecurityStackGetModel(virSecurityManagerPtr mgr)
+static int
+virSecurityStackVerify(virSecurityManagerPtr mgr,
+ virDomainDefPtr def)
+{
+ virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ int rc = 0;
+
+ if (virSecurityManagerVerify(priv->primary, def) < 0)
+ rc = -1;
+
+#if 0
+ if (virSecurityManagerVerify(priv->secondary, def) < 0)
+ rc = -1;
+#endif
What happened here? The original code verified the secondary driver
first, not second, and here, you aren't even verifying the secondary
driver. Are you really fixing a bug in the original code? If so, can
we separate the bug fix from the code motion into two commits?
+static int
+virSecurityStackGenLabel(virSecurityManagerPtr mgr,
+ virDomainObjPtr vm)
+{
+ virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ int rc = 0;
+
+ if (virSecurityManagerGenLabel(priv->primary, vm) < 0)
+ rc = -1;
+#if 0
+ if (virSecurityManagerGenLabel(priv->secondary, vm) < 0)
+ rc = -1;
+#endif
Likewise. Here, it makes a bit more sense that you only need to
generate one label to be shared among both stacks, rather than two. But
what if the primary doesn't care about labels while the secondary does -
shouldn't you have a fallback in that case?
The next couple of functions have similar #ifdef 0 questions.
+
+
+static int
+virSecurityStackSetSecurityImageLabel(virSecurityManagerPtr mgr,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+ virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ int rc = 0;
+
+ if (virSecurityManagerSetImageLabel(priv->secondary, vm, disk) < 0)
+ rc = -1;
+ if (virSecurityManagerSetImageLabel(priv->primary, vm, disk) < 0)
+ rc = -1;
The rest of this file better matches my expectations of just code motion.
diff --git a/src/security/security_stack.h
b/src/security/security_stack.h
new file mode 100644
index 0000000..c924842
--- /dev/null
+++ b/src/security/security_stack.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
2010-2011
+
+void virSecurityStackSetPrimary(virSecurityManagerPtr mgr,
+ virSecurityManagerPtr primary);
+void virSecurityStackSetSecondary(virSecurityManagerPtr mgr,
+ virSecurityManagerPtr secondary);
Again, setters with no getters, but I'm okay with that for now.
I had enough comments (including real bugs) that it's probably worth a v3.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org