Re: [ovs-dev] [PATCH] selinux: Allow creating tap devices.

[Aaron Conole <aconole@redhat.com>]

Daniele Di Proietto <diproiettod@vmware.com> writes:

> On 26/01/2017 12:35, "Ansis Atteka" <ansisatteka@gmail.com> wrote:
>>
>>
>>On 26 January 2017 at 21:24, Aaron Conole 
>><aconole@redhat.com> wrote:
>>
>>Daniele Di Proietto <diproiettod@vmware.com> writes:
>>
>>> On 25/01/2017 00:01, "Ansis Atteka" <ansisatteka@gmail.com> wrote:
>>>
>>>>On Jan 25, 2017 4:22 AM, "Daniele Di Proietto" <diproiettod@vmware.com> wrote:
>>>>
>>>>Current SELinux policy in RHEL and Fedora doesn't allow the creation of
>>>>TAP devices.
>>>>
>>>>A tap device is used by dpif-netdev to create internal devices.
>>>>
>>>>Without this patch, adding any bridge backed by the userspace datapath
>>>>would fail.
>>>>
>>>>This doesn't mean that we can run Open vSwitch with DPDK under SELinux
>>>>yet, but at least we can use the userspace datapath.
>>>>
>>>>Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
>>
>>I just noticed this, sorry for jumping in late.
>>
>>>>Acked-by: Ansis Atteka <aatteka@ovn.org>
>>>>
>>>>
>>>>I saw that other open source projects like OpenVPN use rw_file_perms
>>>> shortcut macro. Not sure how relevant that is for OVS but that macro
>>>> expands to a little more function calls than what you have
>>>> below. Maybe we don't need it, if what you have
>>>> just worked.
>>>
>>> Thanks a lot for the review.
>>>
>>> I cooked this up using audit2allow and I tested it on fedora 25.  I'm
>>> now able to create and delete userspace bridges, without any further
>>> complaints from selinux
>>
>>I have the following openvswitch-custom.te that did work to run
>>ovs+dpdk under selinux and pass traffic:
>>
>>
>>Thanks for posting this. I think that this is really helpful to
>> gather all necessary OVS+DPDK rules from different sources to make
>> sure that nothing is missed.
> +1, thanks a lot
>> 
>>
>>
>>-------------------- 8< --------------------
>>
>>require {
>>        type openvswitch_t;
>>        type openvswitch_tmp_t;
>>        type openvswitch_var_run_t;
>>        type ifconfig_exec_t;
>>        type hostname_exec_t;
>>        type vfio_device_t;
>>        type kernel_t;
>>        type tun_tap_device_t;
>>        type hugetlbfs_t;
>>        type init_t;
>>        class netlink_socket { setopt getopt create connect getattr write read };
>>        class file { write getattr read open execute execute_no_trans create unlink };
>>        class chr_file { write getattr read open ioctl };
>>        class unix_stream_socket { write getattr read connectto connect setopt getopt sendto accept bind recvfrom acceptfrom };
>>        class dir { write remove_name add_name lock read };
>>}
>>
>>#============= openvswitch_t ==============
>>allow openvswitch_t self:netlink_socket { setopt getopt create connect getattr write read };
>>allow openvswitch_t hostname_exec_t:file { read getattr open execute execute_no_trans };
>>allow openvswitch_t ifconfig_exec_t:file { read getattr open execute execute_no_trans };
>>allow openvswitch_t openvswitch_tmp_t:file { execute execute_no_trans };
>>allow openvswitch_t openvswitch_tmp_t:unix_stream_socket { write getattr read connectto connect setopt getopt sendto accept bind recvfrom acceptfrom };
>>allow openvswitch_t vfio_device_t:chr_file { read write open ioctl getattr };
>>allow openvswitch_t tun_tap_device_t:chr_file { read write getattr open ioctl };
>>allow openvswitch_t hugetlbfs_t:dir { write remove_name add_name lock read };
>>allow openvswitch_t hugetlbfs_t:file { create unlink };
>>allow openvswitch_t kernel_t:unix_stream_socket { write getattr read connectto connect setopt getopt sendto accept bind recvfrom acceptfrom };
>>allow openvswitch_t init_t:file { read open };
>>
>>-------------------- >8 --------------------
>>
>>You'll note that this change gives the openvswitch complete access to
>>hugetlbfs label, which might be the biggest scary part.
>>
>>
>>There is also option to use SELinux switches that allow to activate only subset of SElinux rules on a "per OVS feature basis" if there is risk that because of DPDK whitelise we could be unconditionally loosening up SElinux policy too much for non-DPDK
>> cases. See [https://wiki.centos.org/TipsAndTricks/SelinuxBooleans] for more details.
> Ok, so perhaps we should require tun_tap_device_t permissions only if
> we enable userspace support with a boolean.
> I just posted this piece because the corresponding code is in openvswitch source tree.
> The rest of the permissions (hugepages, vfio) are required because of
> code that's in the dpdk library.  Is there a way to put these in DPDK
> and then just call a macro here, like
> dpdk_perms(openvswitch_t)

Below is an example of the macro:

-------------------- 8< --------------------

define(`dpdk_perms', `
	gen_require(`
		type vfio_device_t;
		type kernel_t;
		type hugetlbfs_t;
		class file { write getattr read open execute execute_no_trans
			create unlink };
		class chr_file { write getattr read open ioctl };
		class unix_stream_socket { write getattr read connectto connect
			 setopt getopt sendto accept bind recvfrom acceptfrom };
		class dir { write remove_name add_name lock read };
	')

	allow $1_t vfio_device_t:chr_file { read write open ioctl getattr };
	allow $1_t hugetlbfs_t:dir { write remove_name add_name lock read };
	allow $1_t hugetlbfs_t:file { create unlink };
	allow $1_t kernel_t:unix_stream_socket { write getattr read connectto
		connect setopt getopt sendto accept bind recvfrom acceptfrom };
')

-------------------- >8 --------------------

And then it can be called at the end of the .te file as:

  dpdk_perms(openvswitch)

I am not sure how best to install this in the end system to make sure
that it gets included properly - I'll ask around here and maybe get an
answer (or even post a patch to the dpdk mailing list).  I did try
making a .te file with this macro and a policy definition, but I wasn't
able to reference it from within openvswitch-custom.te; most likely I
will need to figure out where my configuration is wrong.

> I'm a little bit concerned because there are different drivers in DPDK
> and they require different permissions (uio, igb_uio).
> Perhaps we should try to work with upstream
> https://github.com/TresysTechnology/refpolicy-contrib
> I'm not sure if what I'm saying make total sense, but I'm glad we're discussing this
> Thanks,
> Daniele
>> 
>>
>>
>>> I'm definitely not an expert in SELinux, so I'm not sure if it's
>>> better to use the macro and ask for extra permission, or to hardcode
>>> the list.
>>>
>>> What do you think?
>>
>>
>>
>>
>>For macro usage?  I haven't messed with them at all.  I'll note that
>>
>>I guess I don't have a strong opinion about macros. I think that as
>> long as nothing is missed and given OVS features works as expected
>> we are good. Anyway, probably Linux distribution maintainers should
>> have final say about this if this gets up-streamed
>> to the main SELinux policy repository at
>> https://github.com/TresysTechnology/refpolicy/wiki
>> 
>>
>>
>>https://github.com/redhat-openstack/openstack-selinux/pull/5/commits/67606ffa6ea85db73e1955868f53848e05096bf0
>>
>>has what appear to be these macros in a .te file, but I'm going to echo
>>what is previously written: I'm not an selinux expert. 
>>
>>
>>
>>-Aaron
>>
>>
>>
>>
>>
>>