[ovs-dev] [PATCH v4 0/3] vhost-user: Add the ability to control ownership/permissions

Daniele Di Proietto diproiettod at ovn.org
Tue Oct 4 02:15:48 UTC 2016


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 <aconole at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list