Daniele Di Proietto <diproiet...@vmware.com> writes:
> Hi Aaron,
>
> I'm still a little bit nervous about calling chown on a (partially)
> user controlled file name.
I agree, that always seems scary.
> Before moving forward I wanted to discuss a couple of other options:
>
> * Ansis (in CC) suggested using -runas parameter in qemu. This way
> qemu can open the socket as root and drop privileges before starting
> guest execution.
I'm not sure how to do this with libvirt, or via the OpenStack Neutron
plugin. I also don't know if it would be an acceptable workaround for
users. Additionally, I recall there being something of a "don't even
know if this works" around it. Maybe Christian or Ansis (both in CC)
can expound on it.
Today, I know that users are manually going to directories which
contain the vhost-user sockets and running chown/chmod. Sometimes with
even worse protections than what ovs could provide (I know of someone
just doing `chown a+rw`..shudder..). Additionally, it definitely
doesn't survive reboot.
I think what attracted Christian and I to this feature set is that it
can be complementary to MAC, by using group whitelisting (ie: ovs is in
a mutual vhost-user group with the qemu group and can chown/chmod
without actually needing root).
There is a more complicated solution, which is to keep a part of OVS
running as root that can manually open the files, change permissions,
and pass the descriptors back. That solution feels a bit more invasive,
and just as dangerous to me.
> * Instead of changing the owner of the socket, we could create it as a
> different user using setfsgid() or seteuid() system call. I'm not
> sure whether this is doable, we need to make sure that
> rte_vhost_driver_register() can run with reduced privileges and that
> the system calls play nicely with other threads.
This is orthogonal, I think. I'm envisioning a scenario as I've
outlined above. Open vSwitch runs as the openvswitch user. The
openvswitch user belongs to both the openvswitch group, as well as the
vhost-user group. Additionally, the qemu user is a member of the
vhost-user group. Then there's a baked in whitelist - both have access
to vhost-user group chown'd files, and we can set the permissions on the
socket to be something like 0060 or 0660. This assumes that it is
possible to start ovs+dpdk without root access; I'll need some time to
investigate that.
> What do you guys think?
I think originally we quickly discussed 4 possible solutions (and
hopefully I captured them correctly):
1. OVS downgrades to the ovs user, and kvm runs under the ovs
group. I don't actually like this solution because kvm could then
pollute the ovs database.
2. OVS runs as some user and sets the user/group ownership of the socket
via chown/chmod where permissions come from the database (the
original context had ovs running as root - but as I described above
it doesn't need to be root provided ovs+DPDK can start without root).
3. OVS runs as some user, kvm starts as root, opens the socket and
downgrades. IIRC, this doesn't actually work, or it may have
implications on other projects. I don't remember exactly what was
not as great about this solution, TBH.
4. OVS and KVM run as whatever users; MAC is used to enforce the
layering between them.
I think solution 2 and solution 4 don't actually interfere with each
other, and can be used to a complementary effect (if implemented
properly) so that the MAC layer enforces access, but even without MAC,
the DAC layer can provide appropriate whitelisting behavior.
I hope that makes sense. Thanks for your input on this, as always,
Daniele!
-Aaron
> Thanks,
>
> Daniele
>
>
>
>
> On 20/05/2016 13:32, "Aaron Conole" <acon...@redhat.com> wrote:
>
>>Currently, when dpdkvhostuser devices are created, they inherit whatever the
>>running umask and uid/gid of the vswitchd process. This leads to difficulties
>>when using vhost_user consumers (such as qemu).
>>
>>This patch introduces two new database entries, 'vhost-sock-owner' to set the
>>ownership, and 'vhost-sock-perms' to set the permissions bits for the
>>vhost_user sockets.
>>
>>Signed-off-by: Aaron Conole <acon...@redhat.com>
>>---
>>v1->v2:
>>* Rebased to latest
>>
>> INSTALL.DPDK.md | 7 +++++++
>> NEWS | 2 ++
>> lib/netdev-dpdk.c | 26 +++++++++++++++++++++++---
>> vswitchd/vswitch.xml | 23 +++++++++++++++++++++++
>> 4 files changed, 55 insertions(+), 3 deletions(-)
>>
>>diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
>>index 93f92e4..5debcd1 100644
>>--- a/INSTALL.DPDK.md
>>+++ b/INSTALL.DPDK.md
>>@@ -180,6 +180,13 @@ Using the DPDK with ovs-vswitchd:
>> * vhost-sock-dir
>> Option to set the path to the vhost_user unix socket files.
>>
>>+ * vhost-sock-owner
>>+ Option to set the owner of the vhost_user unix socket files.
>>+
>>+ * vhost-sock-perms
>>+ Option to set the DAC permissions of the vhost_user unix socket
>>+ files.
>>+
>> NOTE: Changing any of these options requires restarting the ovs-vswitchd
>> application.
>>
>>diff --git a/NEWS b/NEWS
>>index 4e81cad..807d4bf 100644
>>--- a/NEWS
>>+++ b/NEWS
>>@@ -32,6 +32,8 @@ Post-v2.5.0
>> * DB entries have been added for many of the DPDK EAL command line
>> arguments. Additional arguments can be passed via the dpdk-extra
>> entry.
>>+ * New database options 'vhost-sock-owner' and 'vhost-sock-perms' for
>>+ setting vhost_user unix socket file ownership and permissions.
>> - ovs-benchmark: This utility has been removed due to lack of use and
>> bitrot.
>> - ovs-appctl:
>>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>index 87879d5..d405e66 100644
>>--- a/lib/netdev-dpdk.c
>>+++ b/lib/netdev-dpdk.c
>>@@ -31,6 +31,7 @@
>> #include <sys/stat.h>
>> #include <getopt.h>
>>
>>+#include "chutil.h"
>> #include "dirs.h"
>> #include "dp-packet.h"
>> #include "dpif-netdev.h"
>>@@ -140,6 +141,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
>>ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>> static char *cuse_dev_name = NULL; /* Character device cuse_dev_name. */
>> #endif
>> static char *vhost_sock_dir = NULL; /* Location of vhost-user sockets */
>>+static char *vhost_sock_def_owner = NULL; /* Default owner of vhost-user
>>+ sockets*/
>>+static char *vhost_sock_def_perms = NULL; /* Default permissions of
>>+ vhost-user sockets */
>>
>> /*
>> * Maximum amount of time in micro seconds to try and enqueue to vhost.
>>@@ -872,6 +877,16 @@ netdev_dpdk_vhost_user_construct(struct netdev *netdev)
>> }
>>
>> ovs_mutex_unlock(&dpdk_mutex);
>>+ if (!err && vhost_sock_def_owner &&
>>+ (err = ovs_chown(dev->vhost_id, vhost_sock_def_owner))) {
>>+ VLOG_ERR("vhost-user socket device ownership change failed.");
>>+ }
>>+
>>+ if (!err && vhost_sock_def_perms &&
>>+ (err = ovs_chmod(dev->vhost_id, vhost_sock_def_perms))) {
>>+ VLOG_ERR("vhost-user socket device permission change failed.");
>>+ }
>>+
>> return err;
>> }
>>
>>@@ -3098,8 +3113,8 @@ dpdk_init__(const struct smap *ovs_other_config)
>> VLOG_INFO("DPDK Enabled, initializing");
>>
>> #ifdef VHOST_CUSE
>>- if (process_vhost_flags("cuse-dev-name", xstrdup("vhost-net"),
>>- PATH_MAX, ovs_other_config, &cuse_dev_name)) {
>>+ process_vhost_flags("cuse-dev-name", xstrdup("vhost-net"),
>>+ PATH_MAX, ovs_other_config, &cuse_dev_name);
>> #else
>> if (process_vhost_flags("vhost-sock-dir", xstrdup(ovs_rundir()),
>> NAME_MAX, ovs_other_config,
>>@@ -3123,9 +3138,14 @@ dpdk_init__(const struct smap *ovs_other_config)
>> free(sock_dir_subcomponent);
>> } else {
>> vhost_sock_dir = sock_dir_subcomponent;
>>-#endif
>> }
>>
>>+ process_vhost_flags("vhost-sock-owner", NULL, NAME_MAX, ovs_other_config,
>>+ &vhost_sock_def_owner);
>>+ process_vhost_flags("vhost-sock-perms", NULL, NAME_MAX, ovs_other_config,
>>+ &vhost_sock_def_perms);
>>+#endif
>>+
>> /* Get the main thread affinity */
>> CPU_ZERO(&cpuset);
>> err = pthread_getaffinity_np(pthread_self(), sizeof(cpu_set_t),
>>diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>>index 944d8ec..0553d5a 100644
>>--- a/vswitchd/vswitch.xml
>>+++ b/vswitchd/vswitch.xml
>>@@ -311,6 +311,29 @@
>> </p>
>> </column>
>>
>>+ <column name="other_config" key="vhost-sock-owner"
>>+ type='{"type": "string"}'>
>>+ <p>
>>+ Specifies the owner of the vhost-user unix domain socket files.
>>+ </p>
>>+ <p>
>>+ The default is to inherit from the running user and group id's. The
>>+ argument is specified in the same form as the 'chown' unix utility.
>>+ </p>
>>+ </column>
>>+
>>+ <column name="other_config" key="vhost-sock-perms"
>>+ type='{"type": "string"}'>
>>+ <p>
>>+ Specifies the permissions for the vhost-user unix domain socket
>>+ files.
>>+ </p>
>>+ <p>
>>+ The default is derived from the running mask. The argument is
>>+ specified in the same form as the 'chmod' unix utility.
>>+ </p>
>>+ </column>
>>+
>> <column name="other_config" key="n-handler-threads"
>> type='{"type": "integer", "minInteger": 1}'>
>> <p>
>>--
>>2.5.5
>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
