1:25 a.m.
Hi Osier-san, Daniel-san, and all,
This patchset adds an option for KVM guests to retain arbitrary capabilities.
The previous versions are here:
http://www.redhat.com/archives/libvir-list/2011-December/msg00857.html
http://www.redhat.com/archives/libvir-list/2011-December/msg00950.html
v2 -> v3
I updated my patch according to Osier-san's comment.
However,:
- retain caps name (depending on cap-ng lib)
- no test code is added
*[PATCH v3 1/5] conf: add XML schema for capability XML
*[PATCH v3 2/5] conf: add XML schema for domain XML
*[PATCH v3 3/5] util: add functions to keep capabilities
*[PATCH v3 4/5] util: extend virExecWithHook()
*[PATCH v3 5/5] qemu: make qemu processes to retain capabilities
--
Best regards,
Taku Izumi <izumi.taku(a)jp.fujitsu.com>
2:01 a.m.
New subject: [libvirt] [PATCH v3 1/5] conf: add XML schema for capability XML
This patch introduces XML schema for capability XML.
"process" and "cap" element are added.
The list of "cap" elements represents process capabilities host supports.
<capabilities>
<host>
...
<process>
<cap name='chown'/>
<cap name='dac_override'/>
...
</process>
</host>
...
</capabilities>
Signed-off-by: Taku Izumi <izumi.taku(a)jp.fujitsu.com>
---
docs/formatcaps.html.in | 36 +++++++++++++++++++++++++
docs/schemas/capability.rng | 50 +++++++++++++++++++++++++++++++++++
include/libvirt/libvirt.h.in | 45 +++++++++++++++++++++++++++++++
src/conf/capabilities.c | 61 +++++++++++++++++++++++++++++++++++++++++++
src/conf/capabilities.h | 5 +++
5 files changed, 197 insertions(+)
Index: libvirt/src/conf/capabilities.h
===================================================================
--- libvirt.orig/src/conf/capabilities.h
+++ libvirt/src/conf/capabilities.h
@@ -119,6 +119,10 @@ struct _virCapsHost {
virCapsHostSecModel secModel;
virCPUDefPtr cpu;
unsigned char host_uuid[VIR_UUID_BUFLEN];
+
+ unsigned long long processCaps; /* Bitmask of the Process capabilities
+ * see enum virCapsProcessCaps
+ */
};
typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr,
@@ -263,5 +267,6 @@ virCapabilitiesDefaultGuestEmulator(virC
extern char *
virCapabilitiesFormatXML(virCapsPtr caps);
+VIR_ENUM_DECL(virCapsProcessCaps)
#endif /* __VIR_CAPABILITIES_H */
Index: libvirt/src/conf/capabilities.c
===================================================================
--- libvirt.orig/src/conf/capabilities.c
+++ libvirt/src/conf/capabilities.c
@@ -33,6 +33,9 @@
#include "cpu_conf.h"
#include "virterror_internal.h"
+#if HAVE_CAPNG
+# include <cap-ng.h>
+#endif
#define VIR_FROM_THIS VIR_FROM_CAPABILITIES
@@ -40,6 +43,48 @@ VIR_ENUM_DECL(virCapsHostPMTarget)
VIR_ENUM_IMPL(virCapsHostPMTarget, VIR_NODE_SUSPEND_TARGET_LAST,
"suspend_mem", "suspend_disk",
"suspend_hybrid");
+VIR_ENUM_IMPL(virCapsProcessCaps, VIR_PROCESS_CAPABILITY_LAST,
+ "chown",
+ "dac_override",
+ "dac_read_search",
+ "fowner",
+ "fsetid",
+ "kill",
+ "setgid",
+ "setuid",
+ "setpcap",
+ "linux_immutable",
+ "net_bind_service",
+ "net_broadcast",
+ "net_admin",
+ "net_raw",
+ "ipc_lock",
+ "ipc_owner",
+ "sys_module",
+ "sys_rawio",
+ "sys_chroot",
+ "sys_ptrace",
+ "sys_pacct",
+ "sys_admin",
+ "sys_boot",
+ "sys_nice",
+ "sys_resource",
+ "sys_time",
+ "sys_tty_config",
+ "mknod",
+ "lease",
+ "audit_write",
+ "audit_control",
+ "setfcap",
+ "mac_override",
+ "mac_admin")
+
+static void
+virCapabilitiesInitProcessCaps(virCapsPtr caps)
+{
+ caps->host.processCaps |= (1ULL << (CAP_LAST_CAP + 1)) - 1;
+}
+
/**
* virCapabilitiesNew:
* @arch: host machine architecture
@@ -63,6 +108,10 @@ virCapabilitiesNew(const char *arch,
caps->host.offlineMigrate = offlineMigrate;
caps->host.liveMigrate = liveMigrate;
+#ifdef HAVE_CAPNG
+ virCapabilitiesInitProcessCaps(caps);
+#endif
+
return caps;
no_memory:
@@ -754,6 +803,18 @@ virCapabilitiesFormatXML(virCapsPtr caps
virBufferAddLit(&xml, " </secmodel>\n");
}
+ if (caps->host.processCaps) {
+ virBufferAddLit(&xml, " <process>\n");
+ for (i = 0; i < VIR_PROCESS_CAPABILITY_LAST; i++) {
+ if (caps->host.processCaps & (1ULL << i)) {
+ const char *name = virCapsProcessCapsTypeToString(i);
+ if (name)
+ virBufferAsprintf(&xml, " <cap
name='%s'/>\n", name);
+ }
+ }
+ virBufferAddLit(&xml, " </process>\n");
+ }
+
virBufferAddLit(&xml, " </host>\n\n");
Index: libvirt/docs/schemas/capability.rng
===================================================================
--- libvirt.orig/docs/schemas/capability.rng
+++ libvirt/docs/schemas/capability.rng
@@ -46,6 +46,56 @@
<optional>
<ref name='secmodel'/>
</optional>
+ <optional>
+ <ref name='process'/>
+ </optional>
+ </element>
+ </define>
+
+ <define name='process'>
+ <element name='process'>
+ <zeroOrMore>
+ <element name='cap'>
+ <attribute name='name'>
+ <choice>
+ <value>chown</value>
+ <value>dac_override</value>
+ <value>dac_read_search</value>
+ <value>fowner</value>
+ <value>fsetid</value>
+ <value>kill</value>
+ <value>setgid</value>
+ <value>setuid</value>
+ <value>setpcap</value>
+ <value>linux_immutable</value>
+ <value>net_bind_service</value>
+ <value>net_broadcast</value>
+ <value>net_admin</value>
+ <value>net_raw</value>
+ <value>ipc_lock</value>
+ <value>ipc_owner</value>
+ <value>sys_module</value>
+ <value>sys_rawio</value>
+ <value>sys_chroot</value>
+ <value>sys_ptrace</value>
+ <value>sys_pacct</value>
+ <value>sys_admin</value>
+ <value>sys_boot</value>
+ <value>sys_nice</value>
+ <value>sys_resource</value>
+ <value>sys_time</value>
+ <value>sys_tty_config</value>
+ <value>mknod</value>
+ <value>lease</value>
+ <value>audit_write</value>
+ <value>audit_control</value>
+ <value>setfcap</value>
+ <value>mac_override</value>
+ <value>mac_admin</value>
+ </choice>
+ </attribute>
+ </element>
+ </zeroOrMore>
</element>
</define>
Index: libvirt/include/libvirt/libvirt.h.in
===================================================================
--- libvirt.orig/include/libvirt/libvirt.h.in
+++ libvirt/include/libvirt/libvirt.h.in
@@ -3606,6 +3606,51 @@ int virConnectSetKeepAlive(virConnectPtr
int interval,
unsigned int count);
+
+/*
+ * virProcessCapabilityType
+ *
+ * A process capability Type
+ */
+typedef enum {
+ VIR_PROCESS_CAPABILITY_CHOWN,
+ VIR_PROCESS_CAPABILITY_DAC_OVERRIDE,
+ VIR_PROCESS_CAPABILITY_DAC_READ_SEARCH,
+ VIR_PROCESS_CAPABILITY_FOWNER,
+ VIR_PROCESS_CAPABILITY_FSETID,
+ VIR_PROCESS_CAPABILITY_KILL,
+ VIR_PROCESS_CAPABILITY_SETGID,
+ VIR_PROCESS_CAPABILITY_SETUID,
+ VIR_PROCESS_CAPABILITY_SETPCAP,
+ VIR_PROCESS_CAPABILITY_LINUX_IMMUTABLE,
+ VIR_PROCESS_CAPABILITY_NET_BIND_SERVICE,
+ VIR_PROCESS_CAPABILITY_NET_BROADCAST,
+ VIR_PROCESS_CAPABILITY_NET_ADMIN,
+ VIR_PROCESS_CAPABILITY_NET_RAW,
+ VIR_PROCESS_CAPABILITY_IPC_LOCK,
+ VIR_PROCESS_CAPABILITY_IPC_OWNER,
+ VIR_PROCESS_CAPABILITY_SYS_MODULE,
+ VIR_PROCESS_CAPABILITY_SYS_RAWIO,
+ VIR_PROCESS_CAPABILITY_SYS_CHROOT,
+ VIR_PROCESS_CAPABILITY_SYS_PTRACE,
+ VIR_PROCESS_CAPABILITY_SYS_PACCT,
+ VIR_PROCESS_CAPABILITY_SYS_ADMIN,
+ VIR_PROCESS_CAPABILITY_SYS_BOOT,
+ VIR_PROCESS_CAPABILITY_SYS_NICE,
+ VIR_PROCESS_CAPABILITY_SYS_RESOURCE,
+ VIR_PROCESS_CAPABILITY_SYS_TIME,
+ VIR_PROCESS_CAPABILITY_SYS_TTY_CONFIG,
+ VIR_PROCESS_CAPABILITY_MKNOD,
+ VIR_PROCESS_CAPABILITY_LEASE,
+ VIR_PROCESS_CAPABILITY_AUDIT_WRITE,
+ VIR_PROCESS_CAPABILITY_AUDIT_CONTROL,
+ VIR_PROCESS_CAPABILITY_SETFCAP,
+ VIR_PROCESS_CAPABILITY_MAC_OVERRIDE,
+ VIR_PROCESS_CAPABILITY_MAC_ADMIN,
+
+ VIR_PROCESS_CAPABILITY_LAST
+} virProcessCapabilityType;
+
#ifdef __cplusplus
}
#endif
Index: libvirt/docs/formatcaps.html.in
===================================================================
--- libvirt.orig/docs/formatcaps.html.in
+++ libvirt/docs/formatcaps.html.in
@@ -33,6 +33,42 @@ BIOS you will see</p>
<suspend_disk/>
<suspend_hybrid/>
<power_management/>
+ <process>
+ <cap name="chown">
+ <cap name="dac_override">
+ <cap name="dac_read_search">
+ <cap name="fowner">
+ <cap name="fsetid">
+ <cap name="kill">
+ <cap name="setgid">
+ <cap name="setuid">
+ <cap name="setpcap">
+ <cap name="linux_immutable">
+ <cap name="net_bind_service">
+ <cap name="net_broadcast">
+ <cap name="net_admin">
+ <cap name="net_raw">
+ <cap name="ipc_lock">
+ <cap name="ipc_owner">
+ <cap name="sys_module">
+ <cap name="sys_rawio">
+ <cap name="sys_chroot">
+ <cap name="sys_ptrace">
+ <cap name="sys_pacct">
+ <cap name="sys_admin">
+ <cap name="sys_boot">
+ <cap name="sys_nice">
+ <cap name="sys_resource">
+ <cap name="sys_time">
+ <cap name="sys_tty_config">
+ <cap name="mknod">
+ <cap name="lease">
+ <cap name="audit_write">
+ <cap name="audit_control">
+ <cap name="setfcap">
+ <cap name="mac_override">
+ <cap name="mac_admin">
+ </process>
</host></span>
<!-- xen-3.0-x86_64 -->
2:02 a.m.
New subject: [libvirt] [PATCH v3 3/5] util: add functions to keep capabilities
This patch introduces virKeepCapabilities() function and implements
virCommandAllowCap() function.
Existing virClearCapabilities() is function to clear all capabilities.
Instead virKeepCapabilities() is function to keep arbitrary capabilities.
Signed-off-by: Taku Izumi <izumi.taku(a)jp.fujitsu.com>
Signed-off-by: Shota Hirae <m11g1401(a)hibikino.ne.jp>
---
src/util/command.c | 45 ++++++++++++++++++++++++++++++++++++++-------
src/util/command.h | 4 +---
2 files changed, 39 insertions(+), 10 deletions(-)
Index: libvirt/src/util/command.c
===================================================================
--- libvirt.orig/src/util/command.c
+++ libvirt/src/util/command.c
@@ -103,6 +103,8 @@ struct _virCommand {
pid_t pid;
char *pidfile;
bool reap;
+
+ unsigned long long capabilities;
};
/*
@@ -182,6 +184,33 @@ static int virClearCapabilities(void)
return 0;
}
+
+/**
+ * virKeepCapabilities:
+ * @capabilities - capability flag to keep.
+ * In case of 0, this function is identical to
+ * virClearCapabilities()
+ *
+ */
+static int virKeepCapabilities(unsigned long long capabilities)
+{
+ int ret, i;
+
+ capng_clear(CAPNG_SELECT_BOTH);
+
+ for (i = 0; i <= CAP_LAST_CAP; i++) {
+ if (capabilities & (1ULL << i))
+ capng_update(CAPNG_ADD, CAPNG_BOUNDING_SET, i);
+ }
+
+ if (ret = capng_apply(CAPNG_SELECT_BOTH) < 0) {
+ virCommandError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot apply process capabilities %d"), ret);
+ return -1;
+ }
+
+ return 0;
+}
# else
static int virClearCapabilities(void)
{
@@ -189,6 +218,11 @@ static int virClearCapabilities(void)
// "capabilities");
return 0;
}
+
+static int virKeepCapabilities(unsigned long long capabilities)
+{
+ return 0;
+}
# endif
/**
@@ -883,26 +917,23 @@ virCommandClearCaps(virCommandPtr cmd)
cmd->flags |= VIR_EXEC_CLEAR_CAPS;
}
-#if 0 /* XXX Enable if we have a need for capability management. */
-
/**
* virCommandAllowCap:
* @cmd: the command to modify
- * @capability: what to allow
+ * @capabilities: what to allow
*
- * Re-allow a specific capability
+ * Allow specific capabilities
*/
void
virCommandAllowCap(virCommandPtr cmd,
- int capability ATTRIBUTE_UNUSED)
+ unsigned long long capabilities)
{
if (!cmd || cmd->has_error)
return;
- /* XXX ? */
+ cmd->capabilities = capabilities;
}
-#endif /* 0 */
/**
Index: libvirt/src/util/command.h
===================================================================
--- libvirt.orig/src/util/command.h
+++ libvirt/src/util/command.h
@@ -60,10 +60,8 @@ void virCommandSetPidFile(virCommandPtr
void virCommandClearCaps(virCommandPtr cmd);
-# if 0
void virCommandAllowCap(virCommandPtr cmd,
- int capability);
-# endif
+ unsigned long long capabilities);
void virCommandDaemonize(virCommandPtr cmd);
2:03 a.m.
New subject: [libvirt] [PATCH v3 4/5] util: extend virExecWithHook()
This patch extends virExecWithHook() to receive
capability information.
Signed-off-by: Taku Izumi <izumi.taku(a)jp.fujitsu.com>
Signed-off-by: Shota Hirae <m11g1401(a)hibikino.ne.jp>
---
src/util/command.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
Index: libvirt/src/util/command.c
===================================================================
--- libvirt.orig/src/util/command.c
+++ libvirt/src/util/command.c
@@ -393,6 +393,7 @@ prepareStdFd(int fd, int std)
* @hook optional virExecHook function to call prior to exec
* @data data to pass to the hook function
* @pidfile path to use as pidfile for daemonized process (needs DAEMON flag)
+ * @capabilities capabilities to keep
*/
static int
virExecWithHook(const char *const*argv,
@@ -404,7 +405,8 @@ virExecWithHook(const char *const*argv,
unsigned int flags,
virExecHook hook,
void *data,
- char *pidfile)
+ char *pidfile,
+ unsigned long long capabilities)
{
pid_t pid;
int null = -1, i, openmax;
@@ -633,9 +635,9 @@ virExecWithHook(const char *const*argv,
/* The steps above may need todo something privileged, so
* we delay clearing capabilities until the last minute */
- if ((flags & VIR_EXEC_CLEAR_CAPS) &&
- virClearCapabilities() < 0)
- goto fork_error;
+ if (capabilities || (flags & VIR_EXEC_CLEAR_CAPS))
+ if (virKeepCapabilities(capabilities) < 0)
+ goto fork_error;
/* Close logging again to ensure no FDs leak to child */
virLogReset();
@@ -723,7 +725,8 @@ virExecWithHook(const char *const*argv A
int flags_unused ATTRIBUTE_UNUSED,
virExecHook hook ATTRIBUTE_UNUSED,
void *data ATTRIBUTE_UNUSED,
- char *pidfile ATTRIBUTE_UNUSED)
+ char *pidfile ATTRIBUTE_UNUSED,
+ unsigned long long capabilities ATTRIBUTE_UNUSED)
{
/* XXX: Some day we can implement pieces of virCommand/virExec on
* top of _spawn() or CreateProcess(), but we can't implement
@@ -2184,7 +2187,8 @@ virCommandRunAsync(virCommandPtr cmd, pi
cmd->flags,
virCommandHook,
cmd,
- cmd->pidfile);
+ cmd->pidfile,
+ cmd->capabilities);
VIR_DEBUG("Command result %d, with PID %d",
ret, (int)cmd->pid);
2:04 a.m.
New subject: [libvirt] [PATCH v3 5/5] qemu: make qemu processes to retain capabilities
This patch revises qemuProcessStart() function for qemu
processes to retain arbitrary capabilities.
Signed-off-by: Taku Izumi <izumi.taku(a)jp.fujitsu.com>
Signed-off-by: Shota Hirae <m11g1401(a)hibikino.ne.jp>
---
src/qemu/qemu_process.c | 1 +
1 file changed, 1 insertion(+)
Index: libvirt/src/qemu/qemu_process.c
===================================================================
--- libvirt.orig/src/qemu/qemu_process.c
+++ libvirt/src/qemu/qemu_process.c
@@ -3168,6 +3168,7 @@ int qemuProcessStart(virConnectPtr conn,
driver->clearEmulatorCapabilities);
if (driver->clearEmulatorCapabilities)
virCommandClearCaps(cmd);
+ virCommandAllowCap(cmd, vm->def->process_caps);
virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
8:46 a.m.
On Thu, Jan 12, 2012 at 04:25:27PM +0900, Taku Izumi wrote:
Hi Osier-san, Daniel-san, and all,
This patchset adds an option for KVM guests to retain arbitrary capabilities.
The previous versions are here:
http://www.redhat.com/archives/libvir-list/2011-December/msg00857.html
http://www.redhat.com/archives/libvir-list/2011-December/msg00950.html
Can you give me a little more detail on why you need to make KVM
keep certain capabilities.
In your first message you mention that you need to have 'rawio' for
QEMU ?
I am now wondering if we should do this in a different way. ie if
there is some XML configuration parameter for the <disk> that
indicates the need for rawio, then libvirt could automatically
ensures that we add CAP_SYS_RAWIO when starting QEMU.
Likewise with type=ethernet, we could probably give QEMU the
CAP_SYS_NETADMIN capability.
To me this feels nicer directly exposing capabilities in the
XML, since these are really an internal impl detail, not a
guest configuration thing.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
1:38 a.m.
Sorry for late response.
On Fri, 13 Jan 2012 14:46:08 +0000
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Thu, Jan 12, 2012 at 04:25:27PM +0900, Taku Izumi wrote:
> Hi Osier-san, Daniel-san, and all,
>
> This patchset adds an option for KVM guests to retain arbitrary capabilities.
> The previous versions are here:
> http://www.redhat.com/archives/libvir-list/2011-December/msg00857.html
> http://www.redhat.com/archives/libvir-list/2011-December/msg00950.html
Can you give me a little more detail on why you need to make KVM
keep certain capabilities.
In your first message you mention that you need to have 'rawio' for
QEMU ?
Yes.
I am now wondering if we should do this in a different way. ie if
there is some XML configuration parameter for the <disk> that
indicates the need for rawio, then libvirt could automatically
ensures that we add CAP_SYS_RAWIO when starting QEMU.
I see.
You say if a guest has the following XML configuration,
"CAP_SYS_RAWIO" capability is automatically added to it, right?
<disk type='block' device='lun'>
Likewise with type=ethernet, we could probably give QEMU the
CAP_SYS_NETADMIN capability.
To me this feels nicer directly exposing capabilities in the
XML, since these are really an internal impl detail, not a
guest configuration thing.
--
Best regards,
Taku Izumi <izumi.taku(a)jp.fujitsu.com>
2:32 p.m.
On 01/18/2012 12:38 AM, Taku Izumi wrote:
> I am now wondering if we should do this in a different way. ie
if
> there is some XML configuration parameter for the <disk> that
> indicates the need for rawio, then libvirt could automatically
> ensures that we add CAP_SYS_RAWIO when starting QEMU.
I see.
You say if a guest has the following XML configuration,
"CAP_SYS_RAWIO" capability is automatically added to it, right?
<disk type='block' device='lun'>
Yes, that actually seems reasonable, especially since device='lun' is
brand new, and so far, is really the only reason that we'd need
CAP_SYS_RAWIO.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:10 p.m.
On Thu, Jan 19, 2012 at 01:32:08PM -0700, Eric Blake wrote:
On 01/18/2012 12:38 AM, Taku Izumi wrote:
>> I am now wondering if we should do this in a different way. ie if
>> there is some XML configuration parameter for the <disk> that
>> indicates the need for rawio, then libvirt could automatically
>> ensures that we add CAP_SYS_RAWIO when starting QEMU.
>
> I see.
> You say if a guest has the following XML configuration,
> "CAP_SYS_RAWIO" capability is automatically added to it, right?
>
> <disk type='block' device='lun'>
Yes, that actually seems reasonable, especially since device='lun' is
brand new, and so far, is really the only reason that we'd need
CAP_SYS_RAWIO.
Does device='lun' really require CAP_SYS_RAWIO ? I hadn't seen that
mentioned before now
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:48 p.m.
On 01/19/2012 02:10 PM, Daniel P. Berrange wrote:
On Thu, Jan 19, 2012 at 01:32:08PM -0700, Eric Blake wrote:
> On 01/18/2012 12:38 AM, Taku Izumi wrote:
>>> I am now wondering if we should do this in a different way. ie if
>>> there is some XML configuration parameter for the <disk> that
>>> indicates the need for rawio, then libvirt could automatically
>>> ensures that we add CAP_SYS_RAWIO when starting QEMU.
>>
>> I see.
>> You say if a guest has the following XML configuration,
>> "CAP_SYS_RAWIO" capability is automatically added to it, right?
>>
>> <disk type='block' device='lun'>
>
> Yes, that actually seems reasonable, especially since device='lun' is
> brand new, and so far, is really the only reason that we'd need
> CAP_SYS_RAWIO.
Does device='lun' really require CAP_SYS_RAWIO ? I hadn't seen that
mentioned before now
My understanding (and hopefully Paolo corrects me if I'm wrong) is that
some, but not all, SG_IO ioctl usage patterns require CAP_SYS_RAWIO;
using device='lun' is what allows any SG_IO ioctl to pass through in the
first place, but then there is further validation based on the
capabilities. So maybe this calls for an additional xml attribute, only
needed for device='lun', that controls whether the cap is also desired
when accessing the pass-through block device:
<disk type='block' device='lun' rawio='enabled'>
rather than always blindly granting CAP_SYS_RAWIO merely because of the
presence of device='lun'.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:25 a.m.
On Thu, 19 Jan 2012 14:48:41 -0700
Eric Blake <eblake(a)redhat.com> wrote:
On 01/19/2012 02:10 PM, Daniel P. Berrange wrote:
> On Thu, Jan 19, 2012 at 01:32:08PM -0700, Eric Blake wrote:
>> On 01/18/2012 12:38 AM, Taku Izumi wrote:
>>>> I am now wondering if we should do this in a different way. ie if
>>>> there is some XML configuration parameter for the <disk> that
>>>> indicates the need for rawio, then libvirt could automatically
>>>> ensures that we add CAP_SYS_RAWIO when starting QEMU.
>>>
>>> I see.
>>> You say if a guest has the following XML configuration,
>>> "CAP_SYS_RAWIO" capability is automatically added to it,
right?
>>>
>>> <disk type='block' device='lun'>
>>
>> Yes, that actually seems reasonable, especially since device='lun' is
>> brand new, and so far, is really the only reason that we'd need
>> CAP_SYS_RAWIO.
>
> Does device='lun' really require CAP_SYS_RAWIO ? I hadn't seen that
> mentioned before now
My understanding (and hopefully Paolo corrects me if I'm wrong) is that
some, but not all, SG_IO ioctl usage patterns require CAP_SYS_RAWIO;
using device='lun' is what allows any SG_IO ioctl to pass through in the
first place, but then there is further validation based on the
capabilities. So maybe this calls for an additional xml attribute, only
needed for device='lun', that controls whether the cap is also desired
when accessing the pass-through block device:
<disk type='block' device='lun' rawio='enabled'>
rather than always blindly granting CAP_SYS_RAWIO merely because of the
presence of device='lun'.
OK. I'll try to implement like this way.
--
Taku Izumi <izumi.taku(a)jp.fujitsu.com>
12:01 p.m.
On 01/20/2012 07:25 AM, Taku Izumi wrote:
OK. I'll try to implement like this way.
No, I think your current patch is fine. Perhaps in the future we can
try to implement cgroup-based whitelists in the kernel.
In any case adding rawio (which is a per-process capability) to a <disk>
element would be wrong.
Paolo
1:18 a.m.
On Sat, 21 Jan 2012 19:01:35 +0100
Paolo Bonzini <pbonzini(a)redhat.com> wrote:
Thank you for your comment.
On 01/20/2012 07:25 AM, Taku Izumi wrote:
> OK. I'll try to implement like this way.
No, I think your current patch is fine. Perhaps in the future we can
try to implement cgroup-based whitelists in the kernel.
In any case adding rawio (which is a per-process capability) to a <disk>
element would be wrong.
It is true that process capability affects not per disk but a domain.
It's a bit strange, but it is OK in my personal opinion.
Which do you think is better, Eric?
--
Taku Izumi <izumi.taku(a)jp.fujitsu.com>
2:38 a.m.
On 01/27/2012 08:18 AM, Taku Izumi wrote:
> In any case adding rawio (which is a per-process capability) to
a<disk>
> element would be wrong.
It is true that process capability affects not per disk but a domain.
It's a bit strange, but it is OK in my personal opinion.
No, this must be made very clear in the XML! Remember that rawio lets
you send dangerous commands such as WRITE BUFFER and any vendor specific
thing. I absolutely don't think it's okay to enable them on disks just
because _another_ disk gets a rawio="yes" attribute.
If you want to add it to the <disk> element, you should first add
support for an arbitrary whitelist in the kernel (e.g. by extending the
devices cgroups). The whitelisting code is in the kernel, just not the
cgroups interface.
Paolo
7:30 a.m.
On Fri, Jan 27, 2012 at 09:38:48AM +0100, Paolo Bonzini wrote:
On 01/27/2012 08:18 AM, Taku Izumi wrote:
>> In any case adding rawio (which is a per-process capability) to a<disk>
>> element would be wrong.
>
>It is true that process capability affects not per disk but a domain.
>It's a bit strange, but it is OK in my personal opinion.
No, this must be made very clear in the XML! Remember that rawio
lets you send dangerous commands such as WRITE BUFFER and any vendor
specific thing. I absolutely don't think it's okay to enable them
on disks just because _another_ disk gets a rawio="yes" attribute.
If you want to add it to the <disk> element, you should first add
support for an arbitrary whitelist in the kernel (e.g. by extending
the devices cgroups). The whitelisting code is in the kernel, just
not the cgroups interface.
Yep, I tend to agree. We should have
1. rawio="yes|nmo" on the <disk> element somewhere
2. Give the QEMU process CAP_SYS_RAWIO
3. Use the devices cgroup to specify which individual disks
can use rawio.
That said I don't think we need to block the entire patch, just waiting
for #3. I think it is acceptable to implement #1 & #2 right now,
provided that we mark the domain as tainted. After all if we don't do
#1 & #2, then people are just going to set clear_emulator_capabilities=0
which is even more insecure.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
7:55 a.m.
On 01/27/2012 02:30 PM, Daniel P. Berrange wrote:
Yep, I tend to agree. We should have
1. rawio="yes|nmo" on the<disk> element somewhere
2. Give the QEMU process CAP_SYS_RAWIO
3. Use the devices cgroup to specify which individual disks
can use rawio.
That said I don't think we need to block the entire patch, just waiting
for #3. I think it is acceptable to implement #1& #2 right now,
provided that we mark the domain as tainted. After all if we don't do
#1& #2, then people are just going to set clear_emulator_capabilities=0
which is even more insecure.
Yeah, tainting makes sense. Once we implement #3 we can remove the taint.
Paolo
1:56 a.m.
On 01/19/2012 10:48 PM, Eric Blake wrote:
On 01/19/2012 02:10 PM, Daniel P. Berrange wrote:
> On Thu, Jan 19, 2012 at 01:32:08PM -0700, Eric Blake wrote:
>> On 01/18/2012 12:38 AM, Taku Izumi wrote:
>>>> I am now wondering if we should do this in a different way. ie if
>>>> there is some XML configuration parameter for the<disk> that
>>>> indicates the need for rawio, then libvirt could automatically
>>>> ensures that we add CAP_SYS_RAWIO when starting QEMU.
>>>
>>> I see.
>>> You say if a guest has the following XML configuration,
>>> "CAP_SYS_RAWIO" capability is automatically added to it,
right?
>>>
>>> <disk type='block' device='lun'>
>>
>> Yes, that actually seems reasonable, especially since device='lun' is
>> brand new, and so far, is really the only reason that we'd need
>> CAP_SYS_RAWIO.
>
> Does device='lun' really require CAP_SYS_RAWIO ? I hadn't seen that
> mentioned before now
My understanding (and hopefully Paolo corrects me if I'm wrong) is that
some, but not all, SG_IO ioctl usage patterns require CAP_SYS_RAWIO;
Indeed, and using CAP_SYS_RAWIO might let guests brick your disks by
doing firmware updates.
using device='lun' is what allows any SG_IO ioctl to pass
through in the
first place, but then there is further validation based on the
capabilities. So maybe this calls for an additional xml attribute, only
needed for device='lun', that controls whether the cap is also desired
when accessing the pass-through block device:
<disk type='block' device='lun' rawio='enabled'>
rather than always blindly granting CAP_SYS_RAWIO merely because of the
presence of device='lun'.
I don't even see the need to add an attribute to the <disk> element. I
think it's wrong, because you cannot set CAP_SYS_RAWIO for only one
disk. It's a per process decision.
I think Taku's current approach is the right one.
Paolo
4659
days inactive
4674
days old
16 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Paolo Bonzini -
Paolo Bonzini -
Taku Izumi