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

[Aaron Conole]

Daniele Di Proietto  writes:

> 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.

Okay.  

> 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).

Agreed.  Let's drop this effort.

> 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 :
>
>  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 

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

[Daniele Di Proietto]

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 :

> 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

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

[Christian Ehrhardt]

On Tue, Sep 20, 2016 at 3:40 PM, Aaron Conole  wrote:

>
> `other_config:dpdk-extra'


Thank you a lot, but even with my immediate issue being ok by that I'd
still like the change to be integrated in OVS.


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Aaron Conole]

Christian Ehrhardt  writes:
...
> But now even worse than having to have a delta for such a long time it seems 
> we have to add
> openvswitch delta as well.
> This is due to the EAL commandline no more being "reachable" via 
> "ovs-vswitchd [...] --dpdk
> [DPDK-EAL-OPTIONS]" by the (otherwise great) changes to move dpdk 
> configuration into ovsdb.
> (BTW - If I missed that one can still insert arbitrary EAL command line 
> options via other_config
> please let me know).

`other_config:dpdk-extra'

:-)

-Aaron
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Christian Ehrhardt]

On Sun, Sep 4, 2016 at 4:33 PM, Aaron Conole  wrote:

> Sorry for the top-post and don't want to be a pest - ping?


You are not a pest at all Aaron.
I already thank you for driving this all the time since we started looking
at it about a year ago.

Just as of now I face the question if I have to maintain my delta to DPDK (
https://git.launchpad.net/~ubuntu-server/dpdk/tree/
debian/patches/fix-vhost-user-socket-permission.patch?h=
ubuntu-yakkety-dpdk16.07).
But now even worse than having to have a delta for such a long time it
seems we have to add openvswitch delta as well.
This is due to the EAL commandline no more being "reachable" via
"ovs-vswitchd [...] --dpdk [DPDK-EAL-OPTIONS]" by the (otherwise great)
changes to move dpdk configuration into ovsdb.
(BTW - If I missed that one can still insert arbitrary EAL command line
options via other_config please let me know).

So therefore a last try to jointly "pest-vote" for the ability to control
ownership/permission before OSV 2.6 release ...
... Ping ...



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Aaron Conole]

Sorry for the top-post and don't want to be a pest - ping?

Aaron Conole  writes:

> 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
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Aaron Conole]

"Mooney, Sean K"  writes:

>> -Original Message-
>> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Aaron Conole
>> Sent: Saturday, August 20, 2016 12:48 AM
>> To: dev@openvswitch.org; Ben Pfaff ; Daniele Di Proietto
>> 
>> Subject: [ovs-dev] [PATCH v4 0/3] vhost-user: Add the ability to control
>> ownership/permissions
>> 
>> 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.
> [Mooney, Sean K] technically you don’t need to do any manual
> intervention today if you
> Start the ovs-vswitchd process with sudo sg  -c "umask
> 200; ovs-vswitchd .."
> i.e. start it with the same group as qemu process and allow read write
> acess to members of the
> same group.
> This is how we have deployed ovs with dpdk in the networking-ovs-dpdk
> devstack plug for more then 2 years.

Well, I consider that manual intervention anyway.  With that approach,
you can't start using ovs-ctl and you have lots of permissions weirdness
on logfiles and non-dpdk sockets so there is still steps that have to be
taken - even if sure you can wrap them in a script.  OvS isn't setting
the permissions to be "usable values" for qemu user, which is what I
consider the issue (for some definitions of usable values).

> The new parameters make this simpler though as you no longer need to
> use the linux sg and umask command
> To adjust the socket permissions of all files created by the vswitchd
> process. Its also likely more secure
> As the permission change is limited to the vhost-user socket files.
>
>> 
>> 
>> 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/i
>> ndex.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

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

[Mooney, Sean K]

> -Original Message-
> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Aaron Conole
> Sent: Saturday, August 20, 2016 12:48 AM
> To: dev@openvswitch.org; Ben Pfaff ; Daniele Di Proietto
> 
> Subject: [ovs-dev] [PATCH v4 0/3] vhost-user: Add the ability to control
> ownership/permissions
> 
> 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.
[Mooney, Sean K] technically you don’t need to do any manual intervention today 
if you
Start the ovs-vswitchd process with sudo sg   -c "umask 200; 
ovs-vswitchd .."
i.e. start it with the same group as qemu process and allow read write acess to 
members of the
same group.
This is how we have deployed ovs with dpdk in the networking-ovs-dpdk devstack 
plug for more then 2 years.

The new parameters make this simpler though as you no longer need to use the 
linux sg and umask command
To adjust the socket permissions of all files created by the vswitchd process. 
Its also likely more secure
As the permission change is limited to the vhost-user socket files.

> 
> 
> 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/i
> ndex.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