Hi Aaron, apologies for the delay, I took another look at the series, and I'm not convinced this is the right solution to the problem.
This obviously has nothing to do with the quality of the code. While the hardness amplification might help against symlinks, it doesn't protect against some type of race conditions. There's still a window between 'rte_vhost_driver_register()' and 'vhost_set_permissions()' that can be exploited to change permissions of other sockets in the ovs rundir (which is the default vhostuser socket directory). ovs_kchown() doesn't guarantee that the socket is the one we intended. By introducing a sleep between 'rte_vhost_driver_register()' and 'vhost_set_permissions()' (and restarting some daemons in between) I was able to trick ovs-vswitchd into changing the permissions of the ovn nb database socket. I suspect someone more expert than me could find similar and more dangerous exploits. Alternative approaches: 1) vhostuser client mode. It prevents this (and other) problem(s) and it is IMHO the best way to deploy vhostuser in production. I know that it requires a recent version of QEMU, but this whole stack is still pretty new, I don't see many ways around this. 2) Do we want to do chmod/chown in the lower layers of the software stack? Then we need some support in DPDK. Using fchmod() and fchown() in DPDK seems better to me, because it's possible to do it without any race conditions. The same thing is not possible in OVS (it would be possible if DPDK gave us access to the fd before binding). 3) If it's not possible to do it in the lower layers without any races, I think it's best not to hide the hack, but to do it in outside of Open vSwitch, where there might be more context about the other running daemons, or the vhost-sock-dir. As mentioned before, a manual intervention will not survive an ovs-vswitchd restart, but an ovs restart will also break the connection to the existing VM (that's why I think client mode is a much better approach). Again, this is just my opinion, but I'm really not comfortable maintaining this. Daniele 2016-08-19 16:48 GMT-07:00 Aaron Conole <acon...@redhat.com>: > Currently, when using Open vSwitch with DPDK and qemu guests, the > recommended > method for joining the guests is via the dpdkvhostuser interface. This > interface uses Unix Domain sockets to communicate. When these sockets are > created, they inherit the permissions and ownership from the vswitchd > process. > This can lead to an undesirable state where the QEMU process cannot use the > socket file until manual intervention is performed (via `chown` and/or > `chmod` > calls). > > > This patchset gives the ability to set the permissions and ownership of all > dpdkvhostuser sockets from the database, avoiding the manual intervention > required to connect QEMU and OVS via DPDK. > > > The first patch adds chmod and chown calls to lib, with unit tests. The > second patch adds a hardness amplification version as described in the > paper "Portably Solving File TOCTTOU Races with Hardness Amplification" > found at > https://www.usenix.org/legacy/event/fast08/tech/full_papers/ > tsafrir/tsafrir_html/index.html, while the third patch hooks those calls > into the > netdev_dpdk_vhost_user_construct function, after the socket is created. > > > Changes from v3: > * Replaced patch 2/3 with hardness amplification version. Retested on > RHEL7 > and validated the travis builds. > > Changes from v2: > * Added a new 2nd patch to series for chmod/chown on already opened files. > There exist known implementations for other systems, including FreeBSD, > but > only linux is implemented. ENOTSUP is set when these calls fail on > non-linux > systems. > > Aaron Conole (3): > chutil: introduce a new change-utils lib > chutil: Add hardness amplification versions of chmod/chown > netdev-dpdk: Support user-defined socket attribs > > INSTALL.DPDK.md | 8 + > configure.ac | 2 +- > lib/automake.mk | 2 + > lib/chutil-unix.c | 652 ++++++++++++++++++++++++++++++ > +++++++++++++++++++++ > lib/chutil.h | 36 +++ > lib/daemon-unix.c | 149 +----------- > lib/netdev-dpdk.c | 37 +++ > tests/automake.mk | 2 + > tests/library.at | 5 + > tests/test-chutil.c | 297 +++++++++++++++++++++++ > vswitchd/vswitch.xml | 23 ++ > 11 files changed, 1068 insertions(+), 145 deletions(-) > create mode 100644 lib/chutil-unix.c > create mode 100644 lib/chutil.h > create mode 100644 tests/test-chutil.c > > -- > 2.5.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
