tun: avoid owner checks on IFF_ATTACH_QUEUE

PATCH Applied Michael S. Tsirkin 4 participants 10 messages Jan 10, 2013

Subsystems (3)

Networking Network drivers TUN/TAP driver

lore

Share & tools

lore

Michael S. Tsirkin

author

2013-01-10 11:27 UTC #0 lore raw

PATCH 1/1 tun: avoid owner checks on IFF_ATTACH_QUEUE

At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Note: this is unrelated to Stefan's bugfix.

drivers/net/tun.c +0 -2

index fbd106e..78e3225 100644
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());

Signature · 1 line

MST

Stefan Hajnoczi

Reviewed-by on v1 1/1

2013-01-10 11:55 UTC #1 lore raw

Quoted 7 lines · Michael S. Tsirkin

At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.

Quoted 5 lines

Signed-off-by Michael S. Tsirkin <[email protected]>
---
Note: this is unrelated to Stefan's bugfix.

This should go into 3.8-rc.

Reviewed-by Stefan Hajnoczi <[email protected]>

Jason Wang

2013-01-10 14:08 UTC #3 lore raw

In reply to Michael S. Tsirkin

Quoted 6 lines · Michael S. Tsirkin

At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.

It also change the number of queues of the tuntap, maybe we should limit this. Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly.

Quoted 21 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Note: this is unrelated to Stefan's bugfix.

drivers/net/tun.c +0 -2

index fbd106e..78e3225 100644
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());

Michael S. Tsirkin

author

2013-01-10 14:15 UTC #4 lore raw

Quoted 10 lines · Jason Wang

Quoted 6 lines · Michael S. Tsirkin
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.
It also change the number of queues of the tuntap, maybe we should limit this.

Number of active queues? Why does it matter? Max number of queues is already limited by SETIFF.

Quoted 3 lines

Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly.

The point is that management limits tun to allow SETIFF from libvirt only, then passes the fds to qemu.

Quoted 22 lines

Quoted 21 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Note: this is unrelated to Stefan's bugfix.

drivers/net/tun.c +0 -2

index fbd106e..78e3225 100644
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());

Jason Wang

2013-01-10 14:27 UTC #5 lore raw

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Jason Wang
Quoted 6 lines · Michael S. Tsirkin
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.

Quoted 4 lines

Quoted 2 lines
It also change the number of queues of the tuntap, maybe we should limit this.
Number of active queues? Why does it matter? Max number of queues is already limited by SETIFF.

Yes the number of active(real) queues in the kernel net device and this changing may introduce other events such uevent. With this patch, even if a owner is set for tap, every user could change the number of real queues which I don't think is not expected. Without this patch, we can limit a user that just do read and write.

Quoted 6 lines

Quoted 4 lines
Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly.
The point is that management limits tun to allow SETIFF from libvirt only, then passes the fds to qemu.

Yes, but looks like libvirt does not call TUNSETOWNER before passing it to qemu, so we're ok even without this patch. And if libvirt want to do this, it can just call TUNSETOWNER to the user of qemu.

Quoted 25 lines

Quoted 6 lines
Quoted 6 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Note: this is unrelated to Stefan's bugfix.
Quoted 15 lines
Quoted 5 lines
drivers/net/tun.c +0 -0
index fbd106e..78e3225 100644
drivers/net/tun.c static int tun_set_queue(struct file *file, struct ifreq *ifr) · line 1799
Quoted 10 lines
drivers/net/tun.c +0 -2
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael S. Tsirkin

author

2013-01-10 14:37 UTC #6 lore raw

Quoted 9 lines · Jason Wang

Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Jason Wang
Quoted 6 lines · Michael S. Tsirkin
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
It also change the number of queues of the tuntap, maybe we should limit this.
Number of active queues? Why does it matter? Max number of queues is already limited by SETIFF.
Yes the number of active(real) queues in the kernel net device and this changing may introduce other events such uevent.

How can it trigger a uevent?

Quoted 4 lines

With this patch, even
if a owner is set for tap, every user could change the number of real queues which I don't think is not expected. Without this patch, we can limit a user that just do read and write.

In the end if you want very fine tuned security policy you have to use an LSM.

Here we are talking about the expected usage without an LSM. There, enabling/disabling queues is just an optimization:
if an application wants to process data from a single thread it's better off getting it through a single fd. Having to channel threading changes through a priveledged proxy would be very awkward.

Quoted 10 lines

Quoted 6 lines
Quoted 4 lines
Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly.
The point is that management limits tun to allow SETIFF from libvirt only, then passes the fds to qemu.
Yes, but looks like libvirt does not call TUNSETOWNER before passing it to qemu, so we're ok even without this patch. And if libvirt want to do this, it can just call TUNSETOWNER to the user of qemu.

No, that would allow qemu to do SETIFF which we don't want.

Quoted 25 lines

Quoted 25 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Note: this is unrelated to Stefan's bugfix.
Quoted 15 lines
Quoted 5 lines
drivers/net/tun.c +0 -0
index fbd106e..78e3225 100644
drivers/net/tun.c static int tun_set_queue(struct file *file, struct ifreq *ifr) · line 1799
Quoted 10 lines
drivers/net/tun.c +0 -2
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jason Wang

2013-01-10 14:47 UTC #7 lore raw

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Jason Wang
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Jason Wang
Quoted 6 lines · Michael S. Tsirkin
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.

Quoted 7 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
It also change the number of queues of the tuntap, maybe we should limit this.
Number of active queues? Why does it matter? Max number of queues is already limited by SETIFF.
Yes the number of active(real) queues in the kernel net device and this changing may introduce other events such uevent.
How can it trigger a uevent?

netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which may trigger an uevent.

Quoted 8 lines

Quoted 5 lines
With this patch, even
if a owner is set for tap, every user could change the number of real queues which I don't think is not expected. Without this patch, we can limit a user that just do read and write.
In the end if you want very fine tuned security policy you have to use an LSM.

Quoted 6 lines

Here we are talking about the expected usage without an LSM. There, enabling/disabling queues is just an optimization:
if an application wants to process data from a single thread it's better off getting it through a single fd. Having to channel threading changes through a priveledged proxy would be very awkward.

Yes, but we have something similar like bridge-helper in qemu which create devices through a privileged proxy.

Quoted 9 lines

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly.
The point is that management limits tun to allow SETIFF from libvirt only, then passes the fds to qemu.
Yes, but looks like libvirt does not call TUNSETOWNER before passing it to qemu, so we're ok even without this patch. And if libvirt want to do this, it can just call TUNSETOWNER to the user of qemu.
No, that would allow qemu to do SETIFF which we don't want.

True, I was wrong.

Quoted 26 lines

Quoted 26 lines
Quoted 26 lines
Quoted 7 lines
Quoted 7 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Note: this is unrelated to Stefan's bugfix.
Quoted 15 lines
Quoted 5 lines
drivers/net/tun.c +0 -0
index fbd106e..78e3225 100644
drivers/net/tun.c static int tun_set_queue(struct file *file, struct ifreq *ifr) · line 1799
Quoted 10 lines
drivers/net/tun.c +0 -2
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael S. Tsirkin

author

2013-01-10 15:06 UTC #8 lore raw

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Jason Wang
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Jason Wang
Quoted 6 lines · Michael S. Tsirkin
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.
It also change the number of queues of the tuntap, maybe we should limit this.

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Number of active queues? Why does it matter? Max number of queues is already limited by SETIFF.
Yes the number of active(real) queues in the kernel net device and this changing may introduce other events such uevent.
How can it trigger a uevent?
netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which may trigger an uevent.

Look SETOWNER is a tool intended mostly for persistent taps, where you give a specific user the rights to attach to specific taps but not others.

The uevent issue is preventing a DOS by a uevent flood? Then it applies to persistent and non persistent as one. So if one cares about this one should use an LSM or we can add a separate capability to limit this if we care enough.

Quoted 15 lines

Quoted 8 lines
Quoted 5 lines
With this patch, even
if a owner is set for tap, every user could change the number of real queues which I don't think is not expected. Without this patch, we can limit a user that just do read and write.
In the end if you want very fine tuned security policy you have to use an LSM.
Quoted 6 lines
Here we are talking about the expected usage without an LSM. There, enabling/disabling queues is just an optimization:
if an application wants to process data from a single thread it's better off getting it through a single fd. Having to channel threading changes through a priveledged proxy would be very awkward.

Quoted 2 lines

Yes, but we have something similar like bridge-helper in qemu which create devices through a privileged proxy.

This only happens on startup. Threading changes can happen at any time.

Quoted 37 lines

Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly.
The point is that management limits tun to allow SETIFF from libvirt only, then passes the fds to qemu.
Yes, but looks like libvirt does not call TUNSETOWNER before passing it to qemu, so we're ok even without this patch. And if libvirt want to do this, it can just call TUNSETOWNER to the user of qemu.
No, that would allow qemu to do SETIFF which we don't want.
True, I was wrong.
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Quoted 7 lines
Quoted 7 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Note: this is unrelated to Stefan's bugfix.
Quoted 15 lines
Quoted 5 lines
drivers/net/tun.c +0 -0
index fbd106e..78e3225 100644
drivers/net/tun.c static int tun_set_queue(struct file *file, struct ifreq *ifr) · line 1799
Quoted 10 lines
drivers/net/tun.c +0 -2
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jason Wang

2013-01-10 15:27 UTC #9 lore raw

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · Jason Wang
Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Jason Wang
Quoted 12 lines · Michael S. Tsirkin
Quoted 9 lines · Jason Wang
Quoted 6 lines · Michael S. Tsirkin
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.
It also change the number of queues of the tuntap, maybe we should limit this.
Number of active queues? Why does it matter? Max number of queues is already limited by SETIFF.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
Yes the number of active(real) queues in the kernel net device and this changing may introduce other events such uevent.
How can it trigger a uevent?
netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which may trigger an uevent.
Look SETOWNER is a tool intended mostly for persistent taps, where you give a specific user the rights to attach to specific taps but not others.

True.

Quoted 5 lines

The uevent issue is preventing a DOS by a uevent flood? Then it applies to persistent and non persistent as one. So if one cares about this one should use an LSM or we can add a separate capability to limit this if we care enough.

Ok.

Quoted 13 lines

Quoted 13 lines
Quoted 13 lines
Quoted 4 lines
With this patch, even
if a owner is set for tap, every user could change the number of real queues which I don't think is not expected. Without this patch, we can limit a user that just do read and write.
In the end if you want very fine tuned security policy you have to use an LSM.
Here we are talking about the expected usage without an LSM. There, enabling/disabling queues is just an optimization:
if an application wants to process data from a single thread it's better off getting it through a single fd. Having to channel threading changes through a priveledged proxy would be very awkward.

Quoted 4 lines

Quoted 2 lines
Yes, but we have something similar like bridge-helper in qemu which create devices through a privileged proxy.
This only happens on startup. Threading changes can happen at any time.

Yes. So no objection from my side. Thanks for the explanation.

Quoted 35 lines

Quoted 35 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly.
The point is that management limits tun to allow SETIFF from libvirt only, then passes the fds to qemu.
Yes, but looks like libvirt does not call TUNSETOWNER before passing it to qemu, so we're ok even without this patch. And if libvirt want to do this, it can just call TUNSETOWNER to the user of qemu.
No, that would allow qemu to do SETIFF which we don't want.
True, I was wrong.
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Note: this is unrelated to Stefan's bugfix.
Quoted 15 lines
Quoted 5 lines
drivers/net/tun.c +0 -0
index fbd106e..78e3225 100644
drivers/net/tun.c static int tun_set_queue(struct file *file, struct ifreq *ifr) · line 1799
Quoted 10 lines
drivers/net/tun.c +0 -2
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
 		tun = tfile->detached;
 		if (!tun)
 			ret = -EINVAL;
-		else if (tun_not_capable(tun))
-			ret = -EPERM;
 		else
 			ret = tun_attach(tun, file);
 	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
 		tun = rcu_dereference_protected(tfile->tun,
 						lockdep_rtnl_is_held());
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer Applied

2013-01-10 22:38 UTC #2 lore raw

In reply to Stefan Hajnoczi

From: Stefan Hajnoczi <[email protected]>
Date: Thu, 10 Jan 2013 12:55:03 +0100

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write.
Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 7 lines

Quoted 3 lines
---
Note: this is unrelated to Stefan's bugfix.
This should go into 3.8-rc.
Reviewed-by Stefan Hajnoczi <[email protected]>

Applied.