On Wed, Dec 05, 2012 at 02:17:30PM +0800, Jason Wang wrote:

On 12/04/2012 11:24 PM, Michael S. Tsirkin wrote:

quoted

On Tue, Dec 04, 2012 at 09:24:43PM +0800, Jason Wang wrote:

quoted

On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:

quoted

On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:

quoted

On 11/30/2012 06:06 AM, Paul Moore wrote:

quoted

This patch corrects some problems with LSM/SELinux that were introduced
with the multiqueue patchset.  The problem stems from the fact that the
multiqueue work changed the relationship between the tun device and its
associated socket; before the socket persisted for the life of the
device, however after the multiqueue changes the socket only persisted
for the life of the userspace connection (fd open).  For non-persistent
devices this is not an issue, but for persistent devices this can cause
the tun device to lose its SELinux label.

We correct this problem by adding an opaque LSM security blob to the
tun device struct which allows us to have the LSM security state, e.g.
SELinux labeling information, persist for the lifetime of the tun
device.

...

quoted

quoted

-static int selinux_tun_dev_attach(struct sock *sk)
+static int selinux_tun_dev_attach(struct sock *sk, void *security)

 {

+	struct tun_security_struct *tunsec = security;

 	struct sk_security_struct *sksec = sk->sk_security;
 	u32 sid = current_sid();
 	int err;

+	/* we don't currently perform any NetLabel based labeling here ...

 	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
 	
 			   TUN_SOCKET__RELABELFROM, NULL);
 	
 	if (err)
 	
 		return err;

-	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
+	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,

 			   TUN_SOCKET__RELABELTO, NULL);
 	
 	if (err)
 	
 		return err;

-	sksec->sid = sid;
+	sksec->sid = tunsec->sid;
+	sksec->sclass = SECCLASS_TUN_SOCKET;

I'm not sure whether this is correct, looks like we need to differ between
TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent
device, looks like we need change the sid of tunsec like in the past.

It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
elaborate as to why they should be different?

If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:

1) Create the tun/tap network device
2) For persistent device, re-attach the fd to the network device / socket. In 
this case, we call selinux_tun_dev_attch() to relabel the socket sid (in fact 
also the device's since the socket were persistent also) to the sid of process 
that calls TUNSETIFF.

So, after the changes of multiqueue, we need try to preserve those policy. The 
interesting part is the introducing of TUNSETQUEUE, it's used to attach more 
file descriptors/sockets to a tun/tap device after at least one file descriptor 
were attached to the tun/tap device through TUNSETIFF. So I think maybe we 
need differ those two ioctls. This patch looks fine for TUNSETQUEUE, but for 
TUNSETIFF, we need relabel the tunsec to the process that calling TUNSETIFF 
for persistent device?

Basically, it looks like currently once you get a tun fd,
you can attach it to any device even if normally
selinux would prevent you from accessing it.

Yes some checking during TUNSETQUEUE is missed.

quoted

If we reuse selinux_tun_dev_attach, we won't need to
change selinux policy, with a new capability we will need to change it
to allow libvirt to do TUNSETQUEUE.

Also needed for qemu too since it may call TUNSETQUEUE when guest wants
to change the number of queues.

Hmm that's nasty. If you allow qemu to do TUNSETQUEUE then how
do you prevent it from attaching to some other tun?
Maybe we can extend TUNSETQUEUE or add another ioctl to
mark a queue active/inactive? Probably control transmit
and receive being active separately as well.

quoted

quoted

btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap 
device with no file attached. It should be a bug and need to be fixed.

Is this a problem? You can always
attach
set queue
detach

and it would be hard to prevent this ...

Currently, the following steps is allowed:

1. fd1 = open("/dev/net/tun");
2. tunsetiff(fd1, "tap0");
3. tunsetpersistent("tap0");
4. close(fd1);
5. fd2 = open("/dev/net/tun");
6. tunsetqueue(fd2, "tap0);

Allowed for libvirt, right? Not for qemu.

Looks like step 6 should be forbidden since:

- no fd/sockets were attached to the device, we need use TUNSETIFF
instead to keep the API as we used do in single queue tun
- we need update the security information in tun_struct just like what
we discussed in this mail
- it may also miss checks in TUNSETIFF

We need to fix the checks anyway. Basically if you allow
a queue to be the only fd that is attached
(and I don't see how you can prevent this since
userspace does not expect close to fail),
I don't see why one way to get to this state
should be legal and another illegal.

The rest is implementation detail.

quoted

quoted

quoted

One thing that I think we probably should change is the relabelto/from
permissions in the function above (selinux_tun_dev_attach()); in the case
where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
should probably skip the relabel permissions since we want to assign the
TUN device label regardless in this case.

I'm not familiar with the selinux, have a quick glance of the code, looks like 
the label has been initialized to SECINITSID_KERNEL in 
selinux_socket_post_create().

Thanks