On Wednesday, December 05, 2012 01:43:39 PM Michael S. Tsirkin wrote:

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

quoted

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?

Currently, we do the uid & gid check in tun_not_capable() also for 
TUNSETQUEUE. So the fd can't be attach to the device if current process is not 
the owner. And if it's the owner, is it harm to allow the fd to be attached to 
another device?

There's no state information of the tun stored in the tun_file, and it was 
something like after you open a fd of tun, you can call TUNSETIFF to any tun 
if the owner is current process.

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.

It's better to extend current TUNSETQUEUE if needed, for controling 
transmitting and receiving, we can add more parameters to this ioctl (e.g. 
IFF_TX_ENABLE, IFF_TX_DISABLE)

quoted

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.

But what if step 1-4 is done by one process, step 5-6 is done by another? The 
issuse only existed for persistent device with no fd attached. 

I agree we can add checks and let TUNSETQUEUE and let it to differ those 
conditions. But for simplicity, I suggest is to let userspace call TUNSETIFF 
first just like what we did when only single queue is supported in tun, and 
relabel the security of tun_struct there. And we can use different check (e.g. 
selinux for TUNSETQUEUE and TUNSETIFF) to enable the qemu to does TUNSETQUEUE 
itself.

quoted

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.

Well, we can return -EPERM when userspace call TUNSETQUEUE to a tun_struct 
with zero fd/queues.

The rest is implementation detail.

quoted

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html